nodejs
module: have a single hooks thread for all workers
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.
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?
@dygabo I think test failures in https://ci.nodejs.org/job/node-test-commit-custom-suites-freestyle/35851/ might be related to this PR.
Please take a look (after Labor Day ;o))
@dygabo I think test failures in https://ci.nodejs.org/job/node-test-commit-custom-suites-freestyle/35851/ might be related to this PR.
Please take a look (after Labor Day ;o))
I'm on short vacation for the remaining of this week and will be mostly offline. But I will look into it beginning of next week.
As far as I know the freestyle test job runs all tests in a worker thread instead main.
The tests you created and the usecase itself clearly states different behavior in worker compare to main.
So maybe it's just needed to skip them in this test job.
I pushed a fix for the failing tests when running on worker threads (at least temporary until register from worker is sorted out). One of the tests is fixed, the others are skipped for now as they are based on calling register from within the test, causing the register to be called from a worker for the freestyle pipeline and that is as discussed postponed to be discussed separately. Maybe we should document somewhere more prominent that modifying the customization hooks chain is only supported from the main thread for now.
Failed to start CI
⚠ Something was pushed to the Pull Request branch since the last approving review. ✘ Refusing to run CI on potentially unsafe PRhttps://github.com/nodejs/node/actions/runs/8967278724
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
Removing https://github.com/nodejs/node/labels/author%20ready as it seems this PR adds some flakiness, which should be addressed before merging.
Removing author ready PRs that have at least one approval, no pending requests for changes, and a CI started. as it seems this PR adds some flakiness, which should be addressed before merging.
fixed 🔔 no flakiness reproducible with last pushed commit
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 Botahttps://github.com/nodejs/node/actions/runs/8995057971PR-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
I've added dont-land-on-v18.x and dontt-land-on-v20.x given how breaking this is.
