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.

Sign up for GitHub

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

lib: send the argv to the ESM worker thread loader #50916

Closed
wants to merge 1 commit into from
Closed

lib: send the argv to the ESM worker thread loader #50916

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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/modules/esm/hooks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -497,14 +497,17 @@ class HooksProxy {

#isReady = false;

constructor() {
constructor(execArgv) {
const { InternalWorker } = require('internal/worker');
MessageChannel ??= require('internal/worker/io').MessageChannel;

const lock = new SharedArrayBuffer(SHARED_MEMORY_BYTE_LENGTH);
this.#lock = new Int32Array(lock);

// This needs to inherits the parent's argc/argv
// See: https://github.com/nodejs/node/issues/50885
this.#worker = new InternalWorker(loaderWorkerId, {
execArgv,
stderr: false,
stdin: false,
stdout: false,
Expand Down
13 changes: 8 additions & 5 deletions lib/internal/modules/esm/loader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,8 +338,11 @@ class ModuleLoader {
// `CustomizedModuleLoader` is defined at the bottom of this file and
// available well before this line is ever invoked. This is here in
// order to preserve the git diff instead of moving the class.

// Send the `process.execArgv`
// See: https://github.com/nodejs/node/issues/50885
// eslint-disable-next-line no-use-before-define
this.setCustomizations(new CustomizedModuleLoader());
this.setCustomizations(new CustomizedModuleLoader(process.execArgv));
}
return this.#customizations.register(`${specifier}`, `${parentURL}`, data, transferList);
}
Expand Down Expand Up @@ -460,8 +463,8 @@ class CustomizedModuleLoader {
/**
* Instantiate a module loader that uses user-provided custom loader hooks.
*/
constructor() {
getHooksProxy();
constructor(execArgv) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you update all jsdoc declarations?

getHooksProxy(execArgv);
}

/**
Expand Down Expand Up @@ -565,10 +568,10 @@ function createModuleLoader() {
* Get the HooksProxy instance. If it is not defined, then create a new one.
* @returns {HooksProxy}
*/
function getHooksProxy() {
function getHooksProxy(execArgv) {
if (!hooksProxy) {
const { HooksProxy } = require('internal/modules/esm/hooks');
hooksProxy = new HooksProxy();
hooksProxy = new HooksProxy(execArgv);
}

return hooksProxy;
Expand Down
1 change: 1 addition & 0 deletions lib/internal/worker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,7 @@ class Worker extends EventEmitter {
setupPortReferencing(this[kPublicPort], this, 'message');
this[kPort].postMessage({
argv,
execArgv: options.execArgv,
Copy link
Member

Choose a reason for hiding this comment

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

Does this affect all workers, not just the one for hooks?

Copy link
Member Author

Choose a reason for hiding this comment

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

If InternalWorker is only created for hooks, the answer is yes

Copy link
Contributor

Choose a reason for hiding this comment

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

This does seem odd, especially when execArgv is supposed to default to the parent for all Worker cases. I would also have expected a change to the consumption API too. Where is this consumed internally that this change is needed?

type: messageTypes.LOAD_SCRIPT,
filename,
doEval,
Expand Down
5 changes: 3 additions & 2 deletions test/parallel/test-worker-execargv.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,12 @@ if (!process.env.HAS_STARTED_WORKER) {
);
}));

// Refs: https://github.com/nodejs/node/issues/50885
new Worker(
"require('worker_threads').parentPort.postMessage(process.execArgv)",
{ eval: true, execArgv: ['--trace-warnings'] })
{ eval: true, execArgv: ['--trace-warnings', '--conditions', 'meow'] })
.on('message', common.mustCall((data) => {
assert.deepStrictEqual(data, ['--trace-warnings']);
assert.deepStrictEqual(data, ['--trace-warnings', '--conditions', 'meow']);
}));
} else {
process.emitWarning('some warning');
Expand Down