Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

loader: use default loader as cascaded loader in the in loader worker #47620

Merged
merged 7 commits into from
Apr 21, 2023
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion lib/internal/main/worker_thread.js
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,10 @@ port.on('message', (message) => {
if (manifestSrc) {
require('internal/process/policy').setup(manifestSrc, manifestURL);
}
setupUserModules();
const isLoaderWorker =
doEval === 'internal' &&
filename === require('internal/modules/esm/utils').loaderWorkerId;
setupUserModules(isLoaderWorker);

if (!hasStdin)
process.stdin.push(null);
Expand Down
3 changes: 2 additions & 1 deletion lib/internal/modules/esm/hooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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,
Expand Down
5 changes: 4 additions & 1 deletion lib/internal/modules/esm/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -411,7 +411,10 @@ let emittedExperimentalWarning = false;
* @returns {DefaultModuleLoader | CustomizedModuleLoader}
*/
function createModuleLoader(useCustomLoadersIfPresent = true) {
if (useCustomLoadersIfPresent) {
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.
aduh95 marked this conversation as resolved.
Show resolved Hide resolved
!require('internal/modules/esm/utils').isLoaderWorker()) {
GeoffreyBooth marked this conversation as resolved.
Show resolved Hide resolved
GeoffreyBooth marked this conversation as resolved.
Show resolved Hide resolved
const userLoaderPaths = getOptionValue('--experimental-loader');
if (userLoaderPaths.length > 0) {
if (!emittedExperimentalWarning) {
Expand Down
12 changes: 11 additions & 1 deletion lib/internal/modules/esm/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -91,14 +91,22 @@ 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.
setInitializeImportMetaObjectCallback(initializeImportMetaObject);
setImportModuleDynamicallyCallback(importModuleDynamicallyCallback);
}

function isLoaderWorker() {
return _isLoaderWorker;
}

async function initializeHooks() {
const customLoaderPaths = getOptionValue('--experimental-loader');

Expand Down Expand Up @@ -165,4 +173,6 @@ module.exports = {
initializeHooks,
getDefaultConditions,
getConditionsSet,
loaderWorkerId: 'internal/modules/esm/worker',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: id seems misleading to me, like it's a PID or similar, rather than a path.

Suggested change
loaderWorkerId: 'internal/modules/esm/worker',
loaderWorkerSpecifier: 'internal/modules/esm/worker',

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's definitely not a path (because the internal module loader doesn't use paths), I think calling it an ID is more correct.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

O.o it is literally the path to the file (but with CJS's whacked exclusion of the file extension) and is even consumed as such:

this.#worker = new InternalWorker(loaderWorkerId, {

Copy link
Contributor

@aduh95 aduh95 Apr 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not the path to a file, because internal modules are not in the FS, it's more correct to think of them as an offset in the node binary. InternalWorker constructor doesn't take a path, it only accepts internal module IDs – you can try to replace loaderWorkerId with a path at the line you linked to to convince yourself if you want.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it’s whatever we call the string that’s the input to internalBinding(). The signature of that function is internalBinding(module), so I guess we could use loaderWorkerModule. Not sure if that’s any better than loaderWorkerId. It’s not really a specifier because we don’t use this with import.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

loaderWorkerModuleId?

isLoaderWorker,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: just make this a getter?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would assume a function call would be slower than just mapping directly to a primitive?

};
3 changes: 1 addition & 2 deletions lib/internal/modules/esm/worker.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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;
Expand Down
8 changes: 4 additions & 4 deletions lib/internal/process/pre_execution.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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.
Expand Down
75 changes: 75 additions & 0 deletions test/es-module/test-loaders-workers-spawned.mjs
Original file line number Diff line number Diff line change
@@ -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 CJS and CJS entry point', async () => {
const { code, signal, stdout, stderr } = await spawnPromisified(execPath, [
'--no-warnings',
'--eval',
'setTimeout(() => console.log("D"),99)',
'--import',
fixtures.fileURL('printC.js'),
'--experimental-loader',
fixtures.fileURL('printB.js'),
'--require',
fixtures.path('printA.js'),
]);

assert.strictEqual(stderr, '');
assert.match(stdout, /^A\r?\nA\r?\nB\r?\nC\r?\nD\r?\n$/);
aduh95 marked this conversation as resolved.
Show resolved Hide resolved
assert.strictEqual(code, 0);
assert.strictEqual(signal, null);
});

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'),
'--experimental-loader',
'data:text/javascript,import{writeFileSync}from"node:fs";writeFileSync(1, "B\n")',
aduh95 marked this conversation as resolved.
Show resolved Hide resolved
'--import',
fixtures.fileURL('printC.js'),
'--input-type=module',
'--eval',
'setTimeout(() => console.log("D"),99)',
]);

assert.strictEqual(stderr, '');
assert.match(stdout, /^A\r?\nA\r?\nB\r?\nC\r?\nD\r?\n$/);
aduh95 marked this conversation as resolved.
Show resolved Hide resolved
assert.strictEqual(code, 0);
assert.strictEqual(signal, null);
});
});
3 changes: 3 additions & 0 deletions test/fixtures/print-delayed.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
setTimeout(() => {
console.log('delayed');
}, 100);