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

support registering hook in worker threads #295

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

kherock
Copy link
Contributor

@kherock kherock commented May 31, 2022

This provides a better implementation for #285 by setting and reading the main script from process.env. This allows modules required from threads to trigger reloads.

I considered some alternative solutions where we monkeypatch into the Worker constructor, but ultimately I think an env variable solves this well

@kherock kherock force-pushed the fix-worker-threads-hook branch 2 times, most recently from 1d4ef3c to 7335fed Compare May 31, 2022 03:11
@kherock
Copy link
Contributor Author

kherock commented Jan 15, 2023

Hi @bjornstar! Could this get a review please?

@bjornstar
Copy link
Collaborator

I'd like to see a test so we can validate it's working.

lib/wrap.js Outdated
Comment on lines 14 to 23
if (isMainThread) {
// When using worker threads, each thread inherits execArgv and re-evaulates
// --require scripts. Worker threads do not inherit argv, so we copy argv[1]
// to the environment for those processes to use.
process.env.NODE_DEV_SCRIPT = process.argv[1];
}

const script = process.argv[1];
const script = process.env.NODE_DEV_SCRIPT;
Copy link
Collaborator

Choose a reason for hiding this comment

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

The standard use case does not involve an environment variable called NODE_DEV_SCRIPT

We should not be populating an environment variable on every run.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How do you suggest we do this instead? worker threads do not inherit argv, and I think polluting the worker data sounds worse that adding an env variable. There is also already a precedent of setting NODE_DEV_PRELOAD!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just discovered and refactored this to use getEnvironmentData/setEnvironmentData which feels like it might be more in line with what you're looking for. I also have #284 which gets rid of the other existing env var as well!

@kherock
Copy link
Contributor Author

kherock commented Jan 16, 2023

After adding a test case, I realized that it wasn't actually working as I expected - worker threads don't seem to inherit IPC handles, so I had to adapt the ipc.js helper to set up a new MessagePort as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants