From 3707d0051fbba829cadb2fccac2a7929eae04bad Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Wed, 19 Apr 2023 14:14:01 +0200 Subject: [PATCH 1/7] loader: use default loader as cascaded loader in the in loader worker Use the default loader as the cascaded loader in the loader worker. Otherwise we spawn loader workers in the loader workers indefinitely. --- lib/internal/main/worker_thread.js | 7 ++++++- lib/internal/modules/esm/hooks.js | 3 ++- lib/internal/modules/esm/loader.js | 3 ++- lib/internal/modules/esm/utils.js | 12 +++++++++++- lib/internal/modules/esm/worker.js | 3 +-- lib/internal/process/pre_execution.js | 8 ++++---- test/es-module/test-loader-worker.js | 27 +++++++++++++++++++++++++++ 7 files changed, 53 insertions(+), 10 deletions(-) create mode 100644 test/es-module/test-loader-worker.js diff --git a/lib/internal/main/worker_thread.js b/lib/internal/main/worker_thread.js index 4d8938e58bdf33..b12eb8329696d8 100644 --- a/lib/internal/main/worker_thread.js +++ b/lib/internal/main/worker_thread.js @@ -133,7 +133,12 @@ port.on('message', (message) => { if (manifestSrc) { require('internal/process/policy').setup(manifestSrc, manifestURL); } - setupUserModules(); + let isLoaderWorker = false; + if (doEval === 'internal' && + filename === require('internal/modules/esm/utils').loaderWorkerId) { + isLoaderWorker = true; + } + setupUserModules(isLoaderWorker); if (!hasStdin) process.stdin.push(null); diff --git a/lib/internal/modules/esm/hooks.js b/lib/internal/modules/esm/hooks.js index cde399d97f974f..2aae389c314f55 100644 --- a/lib/internal/modules/esm/hooks.js +++ b/lib/internal/modules/esm/hooks.js @@ -52,6 +52,7 @@ const { } = require('internal/modules/esm/resolve'); const { getDefaultConditions, + loaderWorkerId, } = require('internal/modules/esm/utils'); const { deserializeError } = require('internal/error_serdes'); const { @@ -494,7 +495,7 @@ class HooksProxy { const lock = new SharedArrayBuffer(SHARED_MEMORY_BYTE_LENGTH); this.#lock = new Int32Array(lock); - this.#worker = new InternalWorker('internal/modules/esm/worker', { + this.#worker = new InternalWorker(loaderWorkerId, { stderr: false, stdin: false, stdout: false, diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index a42319dba892c8..4c4ef76bf9fbfa 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -411,7 +411,8 @@ let emittedExperimentalWarning = false; * @returns {DefaultModuleLoader | CustomizedModuleLoader} */ function createModuleLoader(useCustomLoadersIfPresent = true) { - if (useCustomLoadersIfPresent) { + if (useCustomLoadersIfPresent && + !require('internal/modules/esm/utils').isLoaderWorker()) { const userLoaderPaths = getOptionValue('--experimental-loader'); if (userLoaderPaths.length > 0) { if (!emittedExperimentalWarning) { diff --git a/lib/internal/modules/esm/utils.js b/lib/internal/modules/esm/utils.js index 1c01dd13b3ab21..a615d4ecbbbdcf 100644 --- a/lib/internal/modules/esm/utils.js +++ b/lib/internal/modules/esm/utils.js @@ -91,7 +91,11 @@ async function importModuleDynamicallyCallback(wrap, specifier, assertions) { throw new ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING(); } -function initializeESM() { +// This is configured during pre-execution. Specifically it's set to true for +// the loader worker in internal/main/worker_thread.js. +let _isLoaderWorker = false; +function initializeESM(isLoaderWorker = false) { + _isLoaderWorker = isLoaderWorker; initializeDefaultConditions(); // Setup per-isolate callbacks that locate data or callbacks that we keep // track of for different ESM modules. @@ -99,6 +103,10 @@ function initializeESM() { setImportModuleDynamicallyCallback(importModuleDynamicallyCallback); } +function isLoaderWorker() { + return _isLoaderWorker; +} + async function initializeHooks() { const customLoaderPaths = getOptionValue('--experimental-loader'); @@ -165,4 +173,6 @@ module.exports = { initializeHooks, getDefaultConditions, getConditionsSet, + loaderWorkerId: 'internal/modules/esm/worker', + isLoaderWorker, }; diff --git a/lib/internal/modules/esm/worker.js b/lib/internal/modules/esm/worker.js index 8a1ef9add7060f..bba39abdf83661 100644 --- a/lib/internal/modules/esm/worker.js +++ b/lib/internal/modules/esm/worker.js @@ -29,7 +29,7 @@ const { receiveMessageOnPort } = require('internal/worker/io'); const { WORKER_TO_MAIN_THREAD_NOTIFICATION, } = require('internal/modules/esm/shared_constants'); -const { initializeESM, initializeHooks } = require('internal/modules/esm/utils'); +const { initializeHooks } = require('internal/modules/esm/utils'); function transferArrayBuffer(hasError, source) { @@ -67,7 +67,6 @@ async function customizedModuleWorker(lock, syncCommPort, errorHandler) { let hasInitializationError = false; try { - initializeESM(); const initResult = await initializeHooks(); hooks = initResult.hooks; preloadScripts = initResult.preloadScripts; diff --git a/lib/internal/process/pre_execution.js b/lib/internal/process/pre_execution.js index 53cda5737f07c0..fc6d9ccf4f517b 100644 --- a/lib/internal/process/pre_execution.js +++ b/lib/internal/process/pre_execution.js @@ -119,9 +119,9 @@ function prepareExecution(options) { } } -function setupUserModules() { +function setupUserModules(isLoaderWorker = false) { initializeCJSLoader(); - initializeESMLoader(); + initializeESMLoader(isLoaderWorker); const CJSLoader = require('internal/modules/cjs/loader'); assert(!CJSLoader.hasLoadedAnyUserCJSModule); loadPreloadModules(); @@ -600,9 +600,9 @@ function initializeCJSLoader() { initializeCJS(); } -function initializeESMLoader() { +function initializeESMLoader(isLoaderWorker) { const { initializeESM } = require('internal/modules/esm/utils'); - initializeESM(); + initializeESM(isLoaderWorker); // Patch the vm module when --experimental-vm-modules is on. // Please update the comments in vm.js when this block changes. diff --git a/test/es-module/test-loader-worker.js b/test/es-module/test-loader-worker.js new file mode 100644 index 00000000000000..3352cf8246dfa6 --- /dev/null +++ b/test/es-module/test-loader-worker.js @@ -0,0 +1,27 @@ +'use strict'; + +require('../common'); +const { spawnSync } = require('child_process'); +const tmpdir = require('../common/tmpdir'); +const assert = require('assert'); +const { execPath } = require('process'); +const { writeFileSync } = require('fs'); +const { join } = require('path'); + +tmpdir.refresh(); +writeFileSync(join(tmpdir.path, 'hello.cjs'), 'console.log("hello")', 'utf8'); +writeFileSync(join(tmpdir.path, 'loader.mjs'), 'import "./hello.cjs"', 'utf8'); +writeFileSync(join(tmpdir.path, 'main.cjs'), 'setTimeout(() => {}, 1000)', 'utf8'); + +const child = spawnSync(execPath, ['--experimental-loader', './loader.mjs', 'main.cjs'], { + cwd: tmpdir.path +}); + +const stdout = child.stdout.toString().trim(); +const stderr = child.stderr.toString().trim(); +const hellos = [...stdout.matchAll(/hello/g)]; +const warnings = [...stderr.matchAll(/ExperimentalWarning: Custom ESM Loaders/g)]; + +assert.strictEqual(child.status, 0); +assert.strictEqual(hellos.length, 1); +assert.strictEqual(warnings.length, 1); From 4c0fbfda3baffda9ac4581bc0d20622a316f351c Mon Sep 17 00:00:00 2001 From: Geoffrey Booth Date: Wed, 19 Apr 2023 17:28:16 -0700 Subject: [PATCH 2/7] Add tests, reimplement new test in current loaders tests style --- test/es-module/test-loader-worker.js | 27 ------- .../test-loaders-workers-spawned.mjs | 75 +++++++++++++++++++ test/fixtures/print-delayed.js | 3 + 3 files changed, 78 insertions(+), 27 deletions(-) delete mode 100644 test/es-module/test-loader-worker.js create mode 100644 test/es-module/test-loaders-workers-spawned.mjs create mode 100644 test/fixtures/print-delayed.js diff --git a/test/es-module/test-loader-worker.js b/test/es-module/test-loader-worker.js deleted file mode 100644 index 3352cf8246dfa6..00000000000000 --- a/test/es-module/test-loader-worker.js +++ /dev/null @@ -1,27 +0,0 @@ -'use strict'; - -require('../common'); -const { spawnSync } = require('child_process'); -const tmpdir = require('../common/tmpdir'); -const assert = require('assert'); -const { execPath } = require('process'); -const { writeFileSync } = require('fs'); -const { join } = require('path'); - -tmpdir.refresh(); -writeFileSync(join(tmpdir.path, 'hello.cjs'), 'console.log("hello")', 'utf8'); -writeFileSync(join(tmpdir.path, 'loader.mjs'), 'import "./hello.cjs"', 'utf8'); -writeFileSync(join(tmpdir.path, 'main.cjs'), 'setTimeout(() => {}, 1000)', 'utf8'); - -const child = spawnSync(execPath, ['--experimental-loader', './loader.mjs', 'main.cjs'], { - cwd: tmpdir.path -}); - -const stdout = child.stdout.toString().trim(); -const stderr = child.stderr.toString().trim(); -const hellos = [...stdout.matchAll(/hello/g)]; -const warnings = [...stderr.matchAll(/ExperimentalWarning: Custom ESM Loaders/g)]; - -assert.strictEqual(child.status, 0); -assert.strictEqual(hellos.length, 1); -assert.strictEqual(warnings.length, 1); diff --git a/test/es-module/test-loaders-workers-spawned.mjs b/test/es-module/test-loaders-workers-spawned.mjs new file mode 100644 index 00000000000000..9bc7950ecac817 --- /dev/null +++ b/test/es-module/test-loaders-workers-spawned.mjs @@ -0,0 +1,75 @@ +import { spawnPromisified } from '../common/index.mjs'; +import * as fixtures from '../common/fixtures.mjs'; +import assert from 'node:assert'; +import { execPath } from 'node:process'; +import { describe, it } from 'node:test'; + +describe('Worker threads do not spawn infinitely', { concurrency: true }, () => { + it('should not trigger an infinite loop when using a loader exports no recognized hooks', async () => { + const { code, signal, stdout, stderr } = await spawnPromisified(execPath, [ + '--no-warnings', + '--experimental-loader', + fixtures.fileURL('empty.js'), + '--eval', + 'setTimeout(() => console.log("hello"),99)', + ]); + + assert.strictEqual(stderr, ''); + assert.match(stdout, /^hello\r?\n$/); + assert.strictEqual(code, 0); + assert.strictEqual(signal, null); + }); + + it('should support a CommonJS entry point and a loader that imports a CommonJS module', async () => { + const { code, signal, stdout, stderr } = await spawnPromisified(execPath, [ + '--no-warnings', + '--experimental-loader', + fixtures.fileURL('es-module-loaders/loader-with-dep.mjs'), + fixtures.path('print-delayed.js'), + ]); + + assert.strictEqual(stderr, ''); + assert.match(stdout, /^delayed\r?\n$/); + assert.strictEqual(code, 0); + assert.strictEqual(signal, null); + }); + + it('should support --require and --import along with using a loader written in CommonJS', async () => { + const { code, signal, stdout, stderr } = await spawnPromisified(execPath, [ + '--no-warnings', + '--require', + fixtures.path('printA.js'), + '--import', + fixtures.fileURL('printB.js'), + '--experimental-loader', + fixtures.fileURL('empty.js'), + '--eval', + 'setTimeout(() => console.log("C"),99)', + ]); + + assert.strictEqual(stderr, ''); + assert.match(stdout, /^A\r?\nA\r?\nB\r?\nC\r?\n$/); + assert.strictEqual(code, 0); + assert.strictEqual(signal, null); + }); + + it('should support --require and --import along with using a loader written in ESM', async () => { + const { code, signal, stdout, stderr } = await spawnPromisified(execPath, [ + '--no-warnings', + '--require', + fixtures.path('printA.js'), + '--import', + fixtures.fileURL('printB.js'), + '--experimental-loader', + fixtures.fileURL('empty.js'), + '--input-type=module', + '--eval', + 'setTimeout(() => console.log("C"),99)', + ]); + + assert.strictEqual(stderr, ''); + assert.match(stdout, /^A\r?\nA\r?\nB\r?\nC\r?\n$/); + assert.strictEqual(code, 0); + assert.strictEqual(signal, null); + }); +}); diff --git a/test/fixtures/print-delayed.js b/test/fixtures/print-delayed.js new file mode 100644 index 00000000000000..cf9e4a45e92938 --- /dev/null +++ b/test/fixtures/print-delayed.js @@ -0,0 +1,3 @@ +setTimeout(() => { + console.log('delayed'); +}, 1000); From bb939a5cdd7f3457c014d246436dfe32b589039a Mon Sep 17 00:00:00 2001 From: Geoffrey Booth Date: Wed, 19 Apr 2023 17:46:03 -0700 Subject: [PATCH 3/7] Add comment --- lib/internal/modules/esm/loader.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index 4c4ef76bf9fbfa..5a5b544d73f66f 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -412,6 +412,8 @@ let emittedExperimentalWarning = false; */ function createModuleLoader(useCustomLoadersIfPresent = true) { if (useCustomLoadersIfPresent && + // Don't spawn a new worker if we're already in a worker thread created by instantiating CustomizedModuleLoader; + // doing so would cause an infinite loop. !require('internal/modules/esm/utils').isLoaderWorker()) { const userLoaderPaths = getOptionValue('--experimental-loader'); if (userLoaderPaths.length > 0) { From 954e02e07ea87bdd9c9a058766992cedcbe0127c Mon Sep 17 00:00:00 2001 From: Geoffrey Booth Date: Wed, 19 Apr 2023 17:49:40 -0700 Subject: [PATCH 4/7] Clarity --- lib/internal/main/worker_thread.js | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/lib/internal/main/worker_thread.js b/lib/internal/main/worker_thread.js index b12eb8329696d8..12ae4a9b23212d 100644 --- a/lib/internal/main/worker_thread.js +++ b/lib/internal/main/worker_thread.js @@ -133,11 +133,9 @@ port.on('message', (message) => { if (manifestSrc) { require('internal/process/policy').setup(manifestSrc, manifestURL); } - let isLoaderWorker = false; - if (doEval === 'internal' && - filename === require('internal/modules/esm/utils').loaderWorkerId) { - isLoaderWorker = true; - } + const isLoaderWorker = + doEval === 'internal' && + filename === require('internal/modules/esm/utils').loaderWorkerId; setupUserModules(isLoaderWorker); if (!hasStdin) From b691d1447ae1bd47a3cc96ac705d1edcbc3b72de Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Thu, 20 Apr 2023 15:55:09 +0200 Subject: [PATCH 5/7] Apply suggestions from code review --- .../test-loaders-workers-spawned.mjs | 28 +++++++++---------- test/fixtures/print-delayed.js | 2 +- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/test/es-module/test-loaders-workers-spawned.mjs b/test/es-module/test-loaders-workers-spawned.mjs index 9bc7950ecac817..5a63ac99bd0e2d 100644 --- a/test/es-module/test-loaders-workers-spawned.mjs +++ b/test/es-module/test-loaders-workers-spawned.mjs @@ -34,41 +34,41 @@ describe('Worker threads do not spawn infinitely', { concurrency: true }, () => assert.strictEqual(signal, null); }); - it('should support --require and --import along with using a loader written in CommonJS', async () => { + it('should support --require and --import along with using a loader written in CJS and CJS entry point', async () => { const { code, signal, stdout, stderr } = await spawnPromisified(execPath, [ '--no-warnings', - '--require', - fixtures.path('printA.js'), + '--eval', + 'setTimeout(() => console.log("D"),99)', '--import', - fixtures.fileURL('printB.js'), + fixtures.fileURL('printC.js'), '--experimental-loader', - fixtures.fileURL('empty.js'), - '--eval', - 'setTimeout(() => console.log("C"),99)', + fixtures.fileURL('printB.js'), + '--require', + fixtures.path('printA.js'), ]); assert.strictEqual(stderr, ''); - assert.match(stdout, /^A\r?\nA\r?\nB\r?\nC\r?\n$/); + assert.match(stdout, /^A\r?\nA\r?\nB\r?\nC\r?\nD\r?\n$/); assert.strictEqual(code, 0); assert.strictEqual(signal, null); }); - it('should support --require and --import along with using a loader written in ESM', async () => { + it('should support --require and --import along with using a loader written in ESM and ESM entry point', async () => { const { code, signal, stdout, stderr } = await spawnPromisified(execPath, [ '--no-warnings', '--require', fixtures.path('printA.js'), - '--import', - fixtures.fileURL('printB.js'), '--experimental-loader', - fixtures.fileURL('empty.js'), + 'data:text/javascript,console.log("B")', + '--import', + fixtures.fileURL('printC.js'), '--input-type=module', '--eval', - 'setTimeout(() => console.log("C"),99)', + 'setTimeout(() => console.log("D"),99)', ]); assert.strictEqual(stderr, ''); - assert.match(stdout, /^A\r?\nA\r?\nB\r?\nC\r?\n$/); + assert.match(stdout, /^A\r?\nA\r?\nB\r?\nC\r?\nD\r?\n$/); assert.strictEqual(code, 0); assert.strictEqual(signal, null); }); diff --git a/test/fixtures/print-delayed.js b/test/fixtures/print-delayed.js index cf9e4a45e92938..42eb45615b2a67 100644 --- a/test/fixtures/print-delayed.js +++ b/test/fixtures/print-delayed.js @@ -1,3 +1,3 @@ setTimeout(() => { console.log('delayed'); -}, 1000); +}, 100); From 36f9c62d92b9043cf25268026df781750f039595 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Thu, 20 Apr 2023 16:59:42 +0200 Subject: [PATCH 6/7] Update test/es-module/test-loaders-workers-spawned.mjs --- test/es-module/test-loaders-workers-spawned.mjs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/es-module/test-loaders-workers-spawned.mjs b/test/es-module/test-loaders-workers-spawned.mjs index 5a63ac99bd0e2d..83f02374667137 100644 --- a/test/es-module/test-loaders-workers-spawned.mjs +++ b/test/es-module/test-loaders-workers-spawned.mjs @@ -59,7 +59,7 @@ describe('Worker threads do not spawn infinitely', { concurrency: true }, () => '--require', fixtures.path('printA.js'), '--experimental-loader', - 'data:text/javascript,console.log("B")', + 'data:text/javascript,import{writeFileSync}from"node:fs";writeFileSync(1, "B\n")', '--import', fixtures.fileURL('printC.js'), '--input-type=module', From 3db9ee13cc0ac74f96deb76ccdba45636468d193 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Thu, 20 Apr 2023 17:21:46 +0200 Subject: [PATCH 7/7] Apply suggestions from code review --- test/es-module/test-loaders-workers-spawned.mjs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/test/es-module/test-loaders-workers-spawned.mjs b/test/es-module/test-loaders-workers-spawned.mjs index 83f02374667137..a04d8edb041e36 100644 --- a/test/es-module/test-loaders-workers-spawned.mjs +++ b/test/es-module/test-loaders-workers-spawned.mjs @@ -48,7 +48,8 @@ describe('Worker threads do not spawn infinitely', { concurrency: true }, () => ]); assert.strictEqual(stderr, ''); - assert.match(stdout, /^A\r?\nA\r?\nB\r?\nC\r?\nD\r?\n$/); + // The worker code should always run before the --import, but the console.log might arrive late. + assert.match(stdout, /^A\r?\nA\r?\n(B\r?\nC|C\r?\nB)\r?\nD\r?\n$/); assert.strictEqual(code, 0); assert.strictEqual(signal, null); }); @@ -59,7 +60,7 @@ describe('Worker threads do not spawn infinitely', { concurrency: true }, () => '--require', fixtures.path('printA.js'), '--experimental-loader', - 'data:text/javascript,import{writeFileSync}from"node:fs";writeFileSync(1, "B\n")', + 'data:text/javascript,console.log("B")', '--import', fixtures.fileURL('printC.js'), '--input-type=module', @@ -68,7 +69,8 @@ describe('Worker threads do not spawn infinitely', { concurrency: true }, () => ]); assert.strictEqual(stderr, ''); - assert.match(stdout, /^A\r?\nA\r?\nB\r?\nC\r?\nD\r?\n$/); + // The worker code should always run before the --import, but the console.log might arrive late. + assert.match(stdout, /^A\r?\nA\r?\n(B\r?\nC|C\r?\nB)\r?\nD\r?\n$/); assert.strictEqual(code, 0); assert.strictEqual(signal, null); });