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

Jump to bottom

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

wants to merge 1 commit into from

Conversation

juanarbol
Copy link
Member

When the --conditions is sent to a worker thread, it should affect the worker thread with the ESM loader on it. This patch sends the execArgv to the ESM worker.

Fixes: #50885

@juanarbol
lib: send the argv to the ESM worker thread loader
95155c4 
When the `--conditions` is sent to a worker thread, it should affect
the worker thread with the ESM loader on it. This patch sends the
`execArgv` to the ESM worker.

Signed-off-by: Juan José Arboleda <[email protected]>
Fixes: nodejs#50885
@juanarbol juanarbol requested a review from himself65 November 26, 2023 04:55
@juanarbol juanarbol self-assigned this Nov 26, 2023
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders

@nodejs-github-bot nodejs-github-bot added esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. worker Issues and PRs related to Worker support. labels Nov 26, 2023
GeoffreyBooth
GeoffreyBooth reviewed Nov 26, 2023
View reviewed changes
Copy link
Member

@GeoffreyBooth GeoffreyBooth left a comment

Choose a reason for hiding this comment

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

cc @nodejs/workers

lib/internal/worker.js
@@ -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?

@targos
Copy link
Member

targos commented Nov 26, 2023

I don't understand. Most of the changes are in ESM loader files, but no test is changed or added to confirm that something is fixed in this context.

@juanarbol
Copy link
Member Author

I don't understand. Most of the changes are in ESM loader files, but no test is changed or added to confirm that something is fixed in this context.

Look at test/parallel/test-worker-execargv.js

@targos
Copy link
Member

targos commented Nov 26, 2023

I don't understand. Most of the changes are in ESM loader files, but no test is changed or added to confirm that something is fixed in this context.

Look at test/parallel/test-worker-execargv.js

Exactly my point. test/parallel/test-worker-execargv.js doesn't test the ESM loaders worker.

@targos targos closed this Nov 26, 2023
@targos targos reopened this Nov 26, 2023
@juanarbol
Copy link
Member Author

Exactly my point. test/parallel/test-worker-execargv.js doesn't test the ESM loaders worker.

You are right, let discuss this a bit more, first ensure that this change only affects the esm-worker then I will polish this, give a better shape, thanks! Really, thanks!

anonrig
anonrig reviewed Nov 26, 2023
View reviewed changes
lib/internal/modules/esm/loader.js
@@ -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?

GeoffreyBooth
GeoffreyBooth reviewed Nov 26, 2023
View reviewed changes
Copy link
Member

@GeoffreyBooth GeoffreyBooth left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I’m not sure this is the right approach. Per https://nodejs.org/api/worker_threads.html#new-workerfilename-options it’s up to the code calling new Worker to define what should be passed into it, from argv to env to resourceLimits and all sorts of things. I don’t think we want to keep updating Node core code every time a hook author wishes another thing from the main thread were passed into the hooks thread.

I think the original problem can already be solved via code like (untested):

// register.js
import { argv } from 'node:process';
import { register } from 'node:module';

register('./hooks.js', {
  parentURL: import.meta.url,
  data: { argv },
});
// hooks.js
export function initialize({ argv }) {
  console.log(argv);
}
node --import ./register.js app.js

Before we go modifying core, perhaps something like this should be tried?

@juanarbol
Copy link
Member Author

Thanks everyone for reviewing this. The issue is now closed, I’ll proceed to close this PR as well

@juanarbol juanarbol closed this Nov 27, 2023
@GeoffreyBooth GeoffreyBooth mentioned this pull request Nov 27, 2023
Open
@juanarbol juanarbol reopened this Nov 28, 2023
@juanarbol
Copy link
Member Author

I just, had a lapsus 😂 re-opening, I'm still working on this

guybedford
guybedford reviewed Nov 29, 2023
View reviewed changes
Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

What about --import and --require are these also in execArgv? If so that would risk a deadlock situation in the recommended loader use case. These might need to then be filtered out I think.

lib/internal/worker.js
@@ -265,6 +265,7 @@ class Worker extends EventEmitter {
setupPortReferencing(this[kPublicPort], this, 'message');
this[kPort].postMessage({
argv,
execArgv: options.execArgv,
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?

@juanarbol
Copy link
Member Author

Closing in favor of #50752

Refs: #50885 (comment)

@juanarbol juanarbol closed this Dec 1, 2023
@juanarbol juanarbol deleted the juan/esm-argv branch December 6, 2023 03:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anonrig anonrig anonrig left review comments

@guybedford guybedford guybedford left review comments

@GeoffreyBooth GeoffreyBooth GeoffreyBooth left review comments

@himself65 himself65 himself65 approved these changes

Assignees

@juanarbol juanarbol

Labels
esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. worker Issues and PRs related to Worker support.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

--conditions xxx cannot pass into worker_thread
7 participants
@juanarbol @nodejs-github-bot @targos @GeoffreyBooth @guybedford @anonrig @himself65