module: have a single hooks thread for all workers

[dygabo]

This implements reusing the single esm loader hooks thread for all the subsequent workers.
It started as a trial to solve the test contributed by @GeoffreyBooth from #50752. The test content is taken over from that branch and only slightly modified to also support transitive worker threads (that are started by some worker and not by the main thread). This is probably a not very common use case but the interface supports it so I put in the test as well.

With this initial commit all tests are passing.

[==========] 157 tests from 28 test suites ran. (2328 ms total) [ PASSED ] 157 tests. make -s jstest ninja: Entering directory `out/Release' ninja: no work to do. [04:49|% 100|+ 4095|- 0]: Done All tests passed. 

Let's start a discussion and if there are additional usages that are missed by the implementation or if something needs improvements, let me know. If the implementation matches the expectations, we could close the test-only PR and continue the discussion here. Let me know your thoughts.

[nodejs-github-bot]

Review requested:

• @nodejs/loaders

[targos]

@nodejs/loaders

[GeoffreyBooth]

Great work!

[JakobJingleheimer]

Yahoo! 🙌 great work @dygabo. thank you for this!

a few nits, and a couple small tweaks requested.

[JakobJingleheimer]

🙌

[dygabo]

I added a testcase for a scenario where a worker thread import operation would call process.exit. I.e.

• the main thread imports some module that creates a Worker thread
• the Worker thread script imports a module => this gets delegated to the single hook thread
• on the hooks thread this causes a process.exit
• this process.exit now gets propagated to main thread and all workers.

I think this is a change in behavior compared to the previous version. Is this scenario covered correctly now? Is this an acceptable change in behavior?
If not, can this be covered with a follow-up or should it be covered here?

[Flarna]

is the isInternal flag only used by hooks thread?
if yes, the name should be likely changed (maybe in a followup).

[JakobJingleheimer]

Currently but not inherently limited to

[bmeck]

this looks like a great improvement! cheers

[nodejs-github-bot]

Commit Queue failed

- Loading data for nodejs/node/pull/52706 ✔ Done loading data for nodejs/node/pull/52706 ----------------------------------- PR info ------------------------------------ Title module: have a single hooks thread for all workers (#52706) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch dygabo:spawn-only-one-loader-thread -> nodejs:main Labels module, lib / src, needs-ci, loaders, commit-queue-squash Commits 13 - module: have a single hooks thread for all workers - Update lib/internal/modules/esm/loader.js - Apply suggestions from code review - Apply suggestions from code review - code review suggestions addressed - cache response message on exit - add test for process.exit for import running for worker thread - implement unregistration of workers from the hooks thread upon exit - address review finding - fix comment - fix or skip tests that are not fit for running on worker threads - fix flaky test with debug build - fixup! fix flaky test with debug build Committers 1 - Gabriel Bota PR-URL: https://github.com/nodejs/node/pull/52706 Reviewed-By: Geoffrey Booth Reviewed-By: Jacob Smith Reviewed-By: Antoine du Hamel Reviewed-By: Gerhard Stöbich ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/52706 Reviewed-By: Geoffrey Booth Reviewed-By: Jacob Smith Reviewed-By: Antoine du Hamel Reviewed-By: Gerhard Stöbich -------------------------------------------------------------------------------- ⚠ Commits were pushed since the last approving review: ⚠ - module: have a single hooks thread for all workers ⚠ - Update lib/internal/modules/esm/loader.js ⚠ - Apply suggestions from code review ⚠ - Apply suggestions from code review ⚠ - code review suggestions addressed ⚠ - cache response message on exit ⚠ - add test for process.exit for import running for worker thread ⚠ - implement unregistration of workers from the hooks thread upon exit ⚠ - address review finding ⚠ - fix comment ⚠ - fix or skip tests that are not fit for running on worker threads ⚠ - fix flaky test with debug build ⚠ - fixup! fix flaky test with debug build ℹ This PR was created on Fri, 26 Apr 2024 13:22:51 GMT ✔ Approvals: 4 ✔ - Geoffrey Booth (@GeoffreyBooth) (TSC): https://github.com/nodejs/node/pull/52706#pullrequestreview-2025361284 ✔ - Jacob Smith (@JakobJingleheimer): https://github.com/nodejs/node/pull/52706#pullrequestreview-2026747841 ✔ - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/52706#pullrequestreview-2026755507 ✔ - Gerhard Stöbich (@Flarna): https://github.com/nodejs/node/pull/52706#pullrequestreview-2040506445 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2024-05-08T00:05:42Z: https://ci.nodejs.org/job/node-test-pull-request/59022/ - Querying data for job/node-test-pull-request/59022/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/8995057971

[nodejs-github-bot]

Landed in 22cb99d

[mcollina]

I've added dont-land-on-v18.x and dontt-land-on-v20.x given how breaking this is.