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

esm: spawn only one hooks thread #50752

Closed

Conversation

GeoffreyBooth
Copy link
Member

Our intent with moving module customization hooks into a separate thread was that one such “hooks thread” would be spawned regardless of however many worker threads the user’s application code spawned. On current main this is not the case; a new hooks thread is created alongside each new worker thread.

This PR aims to fix that, but currently all I have is a test that fails on current main but should pass, once this bug is fixed. I’m opening this as a placeholder for when a fix is ready, and I encourage anyone who wants to take a crack at it to help make this test pass. I’m happy to let others push commits on my branch.

cc @nodejs/loaders @nodejs/workers

@GeoffreyBooth GeoffreyBooth added module Issues and PRs related to the module subsystem. esm Issues and PRs related to the ECMAScript Modules implementation. worker Issues and PRs related to Worker support. loaders Issues and PRs related to ES module loaders labels Nov 16, 2023
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Nov 16, 2023
@GeoffreyBooth GeoffreyBooth added the help wanted Issues that need assistance from volunteers or PRs that need help to proceed. label Feb 1, 2024
@GeoffreyBooth GeoffreyBooth added loaders-agenda Issues and PRs to discuss during the meetings of the Loaders team and removed loaders-agenda Issues and PRs to discuss during the meetings of the Loaders team labels Feb 26, 2024
@JakobJingleheimer
Copy link
Contributor

JakobJingleheimer commented Feb 26, 2024

Posting this here in case someone else is working on this:

I think the problem looks like:

node    → instantiate HooksProxy
          ↳ spawn InternalWorker
                   
app.js  → spawn Worker
          ↳ instantiate HooksProxy
            ↳ spawn InternalWorker

The various HooksProxys need to always connect to the same InternalWorker. I think the way to do that is to have the "main" MessageChannel::port2 (instantiated by node itself) available to the various HooksProxys. They use that to send their message to InternalWorker and include their own MessageChannel::port2 for InternalWorker to respond back (listening on their own MessageChannel::port1, not main's).

Each HooksProxy needs to instantiate its own lock and pass that to the (possibly existing) working.

PS Sorry, I may be getting the port numbers mixed up (documentation for these are always the most convoluted possible 😩). In the above, InternalWorker receives all messages (regardless of originating thread) on the same port; it then responds back to the originator's (own) provided port.

@GeoffreyBooth
Copy link
Member Author

Fixed by #52706

@GeoffreyBooth GeoffreyBooth removed help wanted Issues that need assistance from volunteers or PRs that need help to proceed. loaders-agenda Issues and PRs to discuss during the meetings of the Loaders team labels May 21, 2024
@GeoffreyBooth GeoffreyBooth deleted the fix-spawning-hooks-thread branch May 21, 2024 02:28
@GeoffreyBooth
Copy link
Member Author

For those of you subscribed to this thread, I just posted nodejs/loaders#201 to ask for your feedback on what direction you think this API should go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. loaders Issues and PRs related to ES module loaders module Issues and PRs related to the module subsystem. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants