module: cache synchronous module jobs before linking

joyeecheung · marco-ippolito · commit 30ed93db12bf · 2025-02-11T11:55:49.000+01:00
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: #52868 Backport-PR-URL: #56927 Fixes: #52864 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Refs: #52697
diff --git a/lib/internal/modules/esm/module_job.js b/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() {
diff --git a/lib/internal/modules/esm/module_map.js b/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 = {
diff --git a/test/es-module/test-require-module-cycle-cjs-esm-esm.js b/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 });
diff --git a/test/fixtures/es-modules/cjs-esm-esm-cycle/a.mjs b/test/fixtures/es-modules/cjs-esm-esm-cycle/a.mjs
@@ -0,0 +1 @@
+export { b } from './b.mjs';
diff --git a/test/fixtures/es-modules/cjs-esm-esm-cycle/b.mjs b/test/fixtures/es-modules/cjs-esm-esm-cycle/b.mjs
@@ -0,0 +1,2 @@
+import './a.mjs'
+export const b = 5;
diff --git a/test/fixtures/es-modules/cjs-esm-esm-cycle/c.cjs b/test/fixtures/es-modules/cjs-esm-esm-cycle/c.cjs
@@ -0,0 +1 @@
+module.exports = require('./a.mjs');

Original file line number	Diff line number	Diff line change
`@@ -114,6 +114,12 @@ class LoadCache extends SafeMap {`
`114`	`114`	`validateString(type, 'type');`
`115`	`115`	`return super.get(url)?.[type] !== undefined;`
`116`	`116`	`}`
	`117`	`+ delete(url, type = kImplicitAssertType) {`
	`118`	`+ const cached = super.get(url);`
	`119`	`+ if (cached) {`
	`120`	`+ cached[type] = undefined;`
	`121`	`+ }`
	`122`	`+ }`
`117`	`123`	`}`
`118`	`124`
`119`	`125`	`module.exports = {`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,2 @@`
	`1`	`+import './a.mjs'`
	`2`	`+export const b = 5;`