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

Worker threads: ability for tooling to pass data to workers and inject NODE_OPTIONS #30992

Closed
coreyfarrell opened this issue Dec 16, 2019 · 5 comments
Labels
feature request Issues that request new features to be added to Node.js. stale worker Issues and PRs related to Worker support.

Comments

@coreyfarrell
Copy link
Member

Is your feature request related to a problem? Please describe.
nyc 15 is moving away from spawn-wrap, will use process-on-spawn and node-preload in it's place. These two modules ensure nyc gets properly loaded into new processes, but they do not help with worker threads (note spawn-wrap also did nothing to help with worker threads). I'd like to add worker thread support to nyc.

Describe the solution you'd like
nyc needs to pass data to worker threads without requiring modifications to arguments of new Worker(...) run by the code being tested. An idea I have:

process.on('worker', options => {
	options.platformData('nyc', nycConfigClonableObject);
	options.nodeOptions.push('--require', require.resolve('./lib/wrap-thread.js'));
});

The above code assumes that new Worker() internally calculates options.nodeOptions based on the env.NODE_OPTIONS passed to it (see also #29117). This would allow nyc to inject preload modules if necessary without altering the process.env of the new thread.

Describe alternatives you've considered
Setting platform data during process startup could work for some use cases:

const threads = require('worker_threads');
threads.platformData('nyc', dataNeededToInitializeNYC);
threads.platformNodeOptions.push('--require', require.resolve('./wrap-thread.js')));

This would work for nyc but maybe another tool would need to calculate the object needed for platformData at the time of new Worker(). This is why my proposal is for 'worker spawn' hooks to ensure this is useful for more than just nyc. I'm also unsure how platformNodeOptions would be incorporated into the env.NODE_OPTIONS given to new Worker().


I considered patching the worker_threads core module. worker.SHARE_ENV is very problematic for this approach as it eliminates the possibility of using environmental variables to get nyc options into the worker threads. We would likely need to patch new Worker() to wrap worker data so that options.workerData = {userData: options.workerData, nycData}, then require('worker_threads').workerData would be patched to return the userData subproperty and we'd create a require('worker_threads').nycData export. This seems like it would be very messy, especially if any other tooling had a need to patch workerData in a similar way. I'm really not interested in this approach even if it could be forced to work.

CC @addaleax @arcanis @nodejs/tooling

@devsnek devsnek added worker Issues and PRs related to Worker support. feature request Issues that request new features to be added to Node.js. labels Dec 20, 2019
@bmeck
Copy link
Member

bmeck commented Jan 6, 2020

It seems like the event listener approach would at least allow some censoring/protecting of options? You could use Object.freeze etc on the options?

@coreyfarrell
Copy link
Member Author

@bmeck I prefer the event listener approach since it allows calculation of data and NODE_OPTIONS to happen when the thread is being created instead of needing to know those values way ahead of time, I'm not sure it helps give any special protection to the options that are specified. One drawback of using the events API is that process.removeAllListeners('worker') could remove my hook. An alternative to the event listener approach would be an API similar to async_hooks where the worker hook would only be removable by the code which added it.

I'm not sure options is actually the best name for the argument either, I'm very open to suggestions for how to best structure the API. Maybe context would be a better name since I was really thinking this would be a class instance with methods to manage data and node options.

Given the PR to run loader hooks in a worker thread I have to ask how you see this interacting with loader hooks? I assume loader hooks thread will always start before any --require's are preloaded and those preloads would not be run inside the loader worker thread? Sorry if this is answered in the PR I really haven't had a chance to dig into the code yet.

@bmeck
Copy link
Member

bmeck commented Jan 7, 2020

I assume loader hooks thread will always start before any --require's are preloaded and those preloads would not be run inside the loader worker thread? Sorry if this is answered in the PR I really haven't had a chance to dig into the code yet.

That is my presumption yes.

jasnell added a commit to jasnell/node that referenced this issue Feb 23, 2021
The `worker.platformData` and `worker.setPlatformData()` APIs allow
an arbitrary, cloneable JavaScript value to be set and passed to all
new Worker instances spawned from the current context. It is similar
to `workerData` except that `platformData` is set independently of
the `new Worker()` constructor, and the the value is passed
automatically to all new Workers.

This is a *partial* fix of nodejs#30992
but does not implement a complete fix.

Signed-off-by: James M Snell <jasnell@gmail.com>
jasnell added a commit to jasnell/node that referenced this issue Mar 1, 2021
The `worker.platformData` and `worker.setPlatformData()` APIs allow
an arbitrary, cloneable JavaScript value to be set and passed to all
new Worker instances spawned from the current context. It is similar
to `workerData` except that `platformData` is set independently of
the `new Worker()` constructor, and the the value is passed
automatically to all new Workers.

This is a *partial* fix of nodejs#30992
but does not implement a complete fix.

Signed-off-by: James M Snell <jasnell@gmail.com>
jasnell added a commit to jasnell/node that referenced this issue Mar 11, 2021
The `worker.platformData` and `worker.setPlatformData()` APIs allow
an arbitrary, cloneable JavaScript value to be set and passed to all
new Worker instances spawned from the current context. It is similar
to `workerData` except that `platformData` is set independently of
the `new Worker()` constructor, and the the value is passed
automatically to all new Workers.

This is a *partial* fix of nodejs#30992
but does not implement a complete fix.

Signed-off-by: James M Snell <jasnell@gmail.com>
jasnell added a commit to jasnell/node that referenced this issue Mar 11, 2021
These APIs allow arbitrary, cloneable JavaScript values to be set and
passed to all new Worker instances spawned from the current context.
It is similar to `workerData` except that environment data is set
independently of the `new Worker()` constructor, and the the value is
passed automatically to all new Workers.

This is a *partial* fix of nodejs#30992
but does not implement a complete fix.

Signed-off-by: James M Snell <jasnell@gmail.com>
jasnell added a commit that referenced this issue Mar 15, 2021
These APIs allow arbitrary, cloneable JavaScript values to be set and
passed to all new Worker instances spawned from the current context.
It is similar to `workerData` except that environment data is set
independently of the `new Worker()` constructor, and the the value is
passed automatically to all new Workers.

This is a *partial* fix of #30992
but does not implement a complete fix.

Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #37486
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
danielleadams pushed a commit that referenced this issue Mar 16, 2021
These APIs allow arbitrary, cloneable JavaScript values to be set and
passed to all new Worker instances spawned from the current context.
It is similar to `workerData` except that environment data is set
independently of the `new Worker()` constructor, and the the value is
passed automatically to all new Workers.

This is a *partial* fix of #30992
but does not implement a complete fix.

Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #37486
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
danielleadams pushed a commit that referenced this issue Mar 16, 2021
These APIs allow arbitrary, cloneable JavaScript values to be set and
passed to all new Worker instances spawned from the current context.
It is similar to `workerData` except that environment data is set
independently of the `new Worker()` constructor, and the the value is
passed automatically to all new Workers.

This is a *partial* fix of #30992
but does not implement a complete fix.

Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #37486
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
targos pushed a commit to targos/node that referenced this issue Aug 8, 2021
These APIs allow arbitrary, cloneable JavaScript values to be set and
passed to all new Worker instances spawned from the current context.
It is similar to `workerData` except that environment data is set
independently of the `new Worker()` constructor, and the the value is
passed automatically to all new Workers.

This is a *partial* fix of nodejs#30992
but does not implement a complete fix.

Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: nodejs#37486
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
targos pushed a commit that referenced this issue Sep 1, 2021
These APIs allow arbitrary, cloneable JavaScript values to be set and
passed to all new Worker instances spawned from the current context.
It is similar to `workerData` except that environment data is set
independently of the `new Worker()` constructor, and the the value is
passed automatically to all new Workers.

This is a *partial* fix of #30992
but does not implement a complete fix.

Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #37486
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
@github-actions
Copy link
Contributor

github-actions bot commented Mar 4, 2022

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Mar 4, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Apr 4, 2022

There has been no activity on this feature request and it is being closed. If you feel closing this issue is not the right thing to do, please leave a comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot closed this as completed Apr 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. stale worker Issues and PRs related to Worker support.
Projects
None yet
Development

No branches or pull requests

3 participants