From eb35469ea48c355966f10b68d4032ab75ab079c6 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Thu, 15 Oct 2026 18:11:22 +0200 Subject: [PATCH] module: fix require(esm) on the module.register() hooks thread On the async loader hooks thread, resolve kImportInRequiredESM and kRequireInImportedCJS requests synchronously again, and always use ModuleJobSync for kImportInRequiredESM. Otherwise require() of an ES module with imports from a hooks module hit an internal assertion in ModuleJobSync.syncLink(). Fixes: https://github.com/nodejs/node/issues/66767 Refs: https://github.com/nodejs/node/pull/62530 Signed-off-by: Matteo Collina --- lib/internal/modules/esm/loader.js | 14 +++++++---- .../require-esm-in-hooks-thread/dep.mjs | 1 + .../esm-with-import.mjs | 4 ++++ .../require-esm-in-hooks-thread/hooks.mjs | 9 ++++++++ ...ader-hooks-require-esm-in-hooks-thread.mjs | 23 +++++++++++++++++++ 5 files changed, 46 insertions(+), 5 deletions(-) create mode 100644 test/fixtures/module-hooks/require-esm-in-hooks-thread/dep.mjs create mode 100644 test/fixtures/module-hooks/require-esm-in-hooks-thread/esm-with-import.mjs create mode 100644 test/fixtures/module-hooks/require-esm-in-hooks-thread/hooks.mjs create mode 100644 test/module-hooks/test-async-loader-hooks-require-esm-in-hooks-thread.mjs diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index 489d8ff91e35..2e0b0fef8e9d 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -613,12 +613,13 @@ class ModuleLoader { const isMain = (parentURL === undefined); const inspectBrk = (isMain && getOptionValue('--inspect-brk')); - // Use ModuleJobSync whenever we're on the main thread (not the async loader hook worker), + // Use ModuleJobSync for kImportInRequiredESM, which must always be linked synchronously, + // and whenever we're on the main thread (not the async loader hook worker), // except for kRequireInImportedCJS (TODO: consolidate that case too). // TODO(joyeecheung): use ModuleJobSync for kRequireInImportedCJS too. - const ModuleJobCtor = (this.isForAsyncLoaderHookWorker || - requestType === kRequireInImportedCJS) ? - ModuleJob : ModuleJobSync; + const ModuleJobCtor = (requestType === kImportInRequiredESM || + (!this.isForAsyncLoaderHookWorker && requestType !== kRequireInImportedCJS)) ? + ModuleJobSync : ModuleJob; job = new ModuleJobCtor( this, url, @@ -645,9 +646,12 @@ class ModuleLoader { */ getOrCreateModuleJob(parentURL, request, requestType) { let maybePromise; - if (!this.isForAsyncLoaderHookWorker) { + if (!this.isForAsyncLoaderHookWorker || + requestType === kRequireInImportedCJS || requestType === kImportInRequiredESM) { // On the main thread, always resolve synchronously; // `resolveSync` coordinates with the async loader hook worker if needed. + // On the async loader hook worker, require(esm) and require() in imported CJS + // must still be resolved synchronously. maybePromise = this.resolveSync(parentURL, request); assert(!isPromise(maybePromise)); } else { diff --git a/test/fixtures/module-hooks/require-esm-in-hooks-thread/dep.mjs b/test/fixtures/module-hooks/require-esm-in-hooks-thread/dep.mjs new file mode 100644 index 000000000000..3d34ee667697 --- /dev/null +++ b/test/fixtures/module-hooks/require-esm-in-hooks-thread/dep.mjs @@ -0,0 +1 @@ +export const value = 'dep:'; diff --git a/test/fixtures/module-hooks/require-esm-in-hooks-thread/esm-with-import.mjs b/test/fixtures/module-hooks/require-esm-in-hooks-thread/esm-with-import.mjs new file mode 100644 index 000000000000..1aad2b7a98c1 --- /dev/null +++ b/test/fixtures/module-hooks/require-esm-in-hooks-thread/esm-with-import.mjs @@ -0,0 +1,4 @@ +import { sep } from 'node:path'; +import { value } from './dep.mjs'; + +export const result = `${value}${typeof sep}`; diff --git a/test/fixtures/module-hooks/require-esm-in-hooks-thread/hooks.mjs b/test/fixtures/module-hooks/require-esm-in-hooks-thread/hooks.mjs new file mode 100644 index 000000000000..b80b4ef959b7 --- /dev/null +++ b/test/fixtures/module-hooks/require-esm-in-hooks-thread/hooks.mjs @@ -0,0 +1,9 @@ +// Loaded on the module.register() hooks thread. It defines no hooks: require()-ing +// an ES module with imports while the hooks module is being loaded is enough. +import { createRequire } from 'node:module'; + +const require = createRequire(import.meta.url); +const { result } = require('./esm-with-import.mjs'); +if (result !== 'dep:string') { + throw new Error(`Unexpected result: ${result}`); +} diff --git a/test/module-hooks/test-async-loader-hooks-require-esm-in-hooks-thread.mjs b/test/module-hooks/test-async-loader-hooks-require-esm-in-hooks-thread.mjs new file mode 100644 index 000000000000..8a647568ffee --- /dev/null +++ b/test/module-hooks/test-async-loader-hooks-require-esm-in-hooks-thread.mjs @@ -0,0 +1,23 @@ +// Test that require(esm) of a module with imports works while loading a module.register() hooks module. +// Refs: https://github.com/nodejs/node/issues/66767 +import '../common/index.mjs'; +import { execPath } from 'node:process'; +import fixtures from '../common/fixtures.js'; +import { spawnSyncAndAssert } from '../common/child_process.js'; + +spawnSyncAndAssert( + execPath, + [ + '--no-warnings', + '--input-type=module', + '--eval', + `import { register } from 'node:module'; + register(${JSON.stringify(fixtures.fileURL('module-hooks/require-esm-in-hooks-thread/hooks.mjs'))}); + console.log('ok');`, + ], + { + stdout: 'ok', + stderr: '', + trim: true, + }, +);