From b2e2b51239a3f76630de7a4d413fe0d8e260ad69 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Tue, 7 May 2024 08:12:16 +0800 Subject: [PATCH] module: cache synchronous module jobs before linking MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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: https://github.com/nodejs/node/pull/52868 Fixes: https://github.com/nodejs/node/issues/52864 Reviewed-By: Moshe Atlow Reviewed-By: Vinícius Lourenço Claro Cardoso Reviewed-By: Antoine du Hamel Reviewed-By: Chengzhong Wu --- lib/internal/modules/esm/module_job.js | 39 ++++++++++++------- lib/internal/modules/esm/module_map.js | 6 +++ .../test-require-module-cycle-cjs-esm-esm.js | 8 ++++ .../es-modules/cjs-esm-esm-cycle/a.mjs | 1 + .../es-modules/cjs-esm-esm-cycle/b.mjs | 2 + .../es-modules/cjs-esm-esm-cycle/c.cjs | 1 + 6 files changed, 43 insertions(+), 14 deletions(-) create mode 100644 test/es-module/test-require-module-cycle-cjs-esm-esm.js create mode 100644 test/fixtures/es-modules/cjs-esm-esm-cycle/a.mjs create mode 100644 test/fixtures/es-modules/cjs-esm-esm-cycle/b.mjs create mode 100644 test/fixtures/es-modules/cjs-esm-esm-cycle/c.cjs diff --git a/lib/internal/modules/esm/module_job.js b/lib/internal/modules/esm/module_job.js index f752e9ed1d35f4..abdd96673f72b3 100644 --- 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 index ab1171eaa47b02..247bde93cabd70 100644 --- 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 new file mode 100644 index 00000000000000..a83d7ee7a71bb2 --- /dev/null +++ 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 new file mode 100644 index 00000000000000..798e86506da2a9 --- /dev/null +++ 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 new file mode 100644 index 00000000000000..9e909cd6856455 --- /dev/null +++ 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 new file mode 100644 index 00000000000000..f9361ecd59d11e --- /dev/null +++ b/test/fixtures/es-modules/cjs-esm-esm-cycle/c.cjs @@ -0,0 +1 @@ +module.exports = require('./a.mjs');