module: cache synchronous module jobs before linking

So that if there are circular dependencies in the synchronous
module graph, they could be resolved using the cached jobs.
In case linking fails and the error gets caught, reset the
cache right after linking. If it succeeds, the caller will
cache it again. Otherwise the error bubbles up to users,
and since we unset the cache for the unlinkable module
the next attempt would still fail.

PR-URL: nodejs#52868
Fixes: nodejs#52864
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>

Author: joyeecheung

lib/internal/modules/esm/module_job.js

@@ -287,22 +287,33 @@ class ModuleJobSync extends ModuleJobBase {
   #loader = null;
   constructor(loader, url, importAttributes, moduleWrap, isMain, inspectBrk) {
     super(url, importAttributes, moduleWrap, isMain, inspectBrk, true);
-    assert(this.module instanceof ModuleWrap);
     this.#loader = loader;
-    const moduleRequests = this.module.getModuleRequests();
-    // Specifiers should be aligned with the moduleRequests array in order.
-    const specifiers = Array(moduleRequests.length);
-    const modules = Array(moduleRequests.length);
-    const jobs = Array(moduleRequests.length);
-    for (let i = 0; i < moduleRequests.length; ++i) {
-      const { specifier, attributes } = moduleRequests[i];
-      const job = this.#loader.getModuleJobForRequire(specifier, url, attributes);
-      specifiers[i] = specifier;
-      modules[i] = job.module;
-      jobs[i] = job;
+
+    assert(this.module instanceof ModuleWrap);
+    // Store itself into the cache first before linking in case there are circular
+    // references in the linking.
+    loader.loadCache.set(url, importAttributes.type, this);
+
+    try {
+      const moduleRequests = this.module.getModuleRequests();
+      // Specifiers should be aligned with the moduleRequests array in order.
+      const specifiers = Array(moduleRequests.length);
+      const modules = Array(moduleRequests.length);
+      const jobs = Array(moduleRequests.length);
+      for (let i = 0; i < moduleRequests.length; ++i) {
+        const { specifier, attributes } = moduleRequests[i];
+        const job = this.#loader.getModuleJobForRequire(specifier, url, attributes);
+        specifiers[i] = specifier;
+        modules[i] = job.module;
+        jobs[i] = job;
+      }
+      this.module.link(specifiers, modules);
+      this.linked = jobs;
+    } finally {
+      // Restore it - if it succeeds, we'll reset in the caller; Otherwise it's
+      // not cached and if the error is caught, subsequent attempt would still fail.
+      loader.loadCache.delete(url, importAttributes.type);
     }
-    this.module.link(specifiers, modules);
-    this.linked = jobs;
   }
 
   get modulePromise() {

lib/internal/modules/esm/module_map.js

@@ -114,6 +114,12 @@ class LoadCache extends SafeMap {
     validateString(type, 'type');
     return super.get(url)?.[type] !== undefined;
   }
+  delete(url, type = kImplicitAssertType) {
+    const cached = super.get(url);
+    if (cached) {
+      cached[type] = undefined;
+    }
+  }
 }
 
 module.exports = {

test/es-module/test-require-module-cycle-cjs-esm-esm.js

@@ -0,0 +1,8 @@
+// Flags: --experimental-require-module
+'use strict';
+
+// This tests that ESM <-> ESM cycle is allowed in a require()-d graph.
+const common = require('../common');
+const cycle = require('../fixtures/es-modules/cjs-esm-esm-cycle/c.cjs');
+
+common.expectRequiredModule(cycle, { b: 5 });

test/fixtures/es-modules/cjs-esm-esm-cycle/a.mjs

@@ -0,0 +1 @@
+export { b } from './b.mjs';

test/fixtures/es-modules/cjs-esm-esm-cycle/b.mjs

@@ -0,0 +1,2 @@
+import './a.mjs'
+export const b = 5;

test/fixtures/es-modules/cjs-esm-esm-cycle/c.cjs

@@ -0,0 +1 @@
+module.exports = require('./a.mjs');