module: have a single hooks thread for all workers #52706

dygabo · 2024-04-26T13:22:51Z

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 · 2024-04-26T13:22:55Z

Review requested:

@nodejs/loaders

targos · 2024-04-26T14:32:07Z

@nodejs/loaders

GeoffreyBooth

Great work!

lib/internal/modules/esm/loader.js

JakobJingleheimer

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

a few nits, and a couple small tweaks requested.

lib/internal/modules/esm/hooks.js

lib/internal/modules/esm/worker.js

lib/internal/worker.js

lib/internal/modules/esm/worker.js

lib/internal/modules/esm/hooks.js

JakobJingleheimer

🙌

lib/internal/modules/esm/worker.js

dygabo · 2024-04-29T11:20:42Z

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 · 2024-04-29T11:05:41Z

lib/internal/worker.js

+      port2: toHooksThread,
+    } = new MessageChannel();
+    if (!isInternal) {
+      // This is not an internal hooks thread => it needs a channel to the hooks thread:


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

Currently but not inherently limited to

lib/internal/modules/esm/worker.js

lib/internal/modules/esm/hooks.js

lib/internal/modules/esm/worker.js

bmeck

this looks like a great improvement! cheers

Co-authored-by: Geoffrey Booth <webadmin@geoffreybooth.com>

Co-authored-by: Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com> Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

nodejs-github-bot · 2024-05-08T00:05:42Z

CI: https://ci.nodejs.org/job/node-test-pull-request/59022/

nodejs-github-bot · 2024-05-08T01:57:47Z

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 · 2024-05-08T02:20:50Z

Landed in 22cb99d

PR-URL: #52706 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Apr 26, 2024

Flarna added module Issues and PRs related to the module subsystem. loaders Issues and PRs related to ES module loaders labels Apr 26, 2024

dygabo force-pushed the spawn-only-one-loader-thread branch from 1025b03 to fa24d02 Compare April 26, 2024 14:03

dygabo marked this pull request as draft April 26, 2024 14:08

dygabo force-pushed the spawn-only-one-loader-thread branch from fa24d02 to ee16c17 Compare April 26, 2024 14:21

GeoffreyBooth approved these changes Apr 26, 2024

View reviewed changes

lib/internal/modules/esm/loader.js Outdated Show resolved Hide resolved

GeoffreyBooth marked this pull request as ready for review April 26, 2024 15:50

dygabo force-pushed the spawn-only-one-loader-thread branch from 03f69a8 to 2fdf7b3 Compare April 27, 2024 04:57

JakobJingleheimer approved these changes Apr 27, 2024

View reviewed changes

aduh95 reviewed Apr 27, 2024

View reviewed changes

lib/internal/modules/esm/worker.js Outdated Show resolved Hide resolved

aduh95 reviewed Apr 27, 2024

View reviewed changes

lib/internal/modules/esm/worker.js Outdated Show resolved Hide resolved

aduh95 reviewed Apr 27, 2024

View reviewed changes

lib/internal/modules/esm/hooks.js Outdated Show resolved Hide resolved

aduh95 reviewed Apr 27, 2024

View reviewed changes

lib/internal/modules/esm/hooks.js Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

JakobJingleheimer approved these changes Apr 27, 2024

View reviewed changes

aduh95 approved these changes Apr 27, 2024

View reviewed changes

lib/internal/modules/esm/worker.js Outdated Show resolved Hide resolved

dygabo force-pushed the spawn-only-one-loader-thread branch from bb59bc8 to 7f86cde Compare April 29, 2024 11:22

dygabo changed the title ~~esm: have a single hooks thread for all workers~~ module: have a single hooks thread for all workers Apr 29, 2024

Flarna reviewed Apr 29, 2024

View reviewed changes

lib/internal/modules/esm/hooks.js Show resolved Hide resolved

bmeck reviewed Apr 30, 2024

View reviewed changes

lib/internal/modules/esm/worker.js Show resolved Hide resolved

bmeck approved these changes Apr 30, 2024

View reviewed changes

Flarna added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 30, 2024

Flarna approved these changes Apr 30, 2024

View reviewed changes

Flarna added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 30, 2024

dygabo and others added 12 commits May 7, 2024 21:27

module: have a single hooks thread for all workers

76add28

Update lib/internal/modules/esm/loader.js

113b41c

Co-authored-by: Geoffrey Booth <webadmin@geoffreybooth.com>

Apply suggestions from code review

e19528f

Co-authored-by: Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com> Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

Apply suggestions from code review

101dd4f

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

code review suggestions addressed

7cc6400

cache response message on exit

a2abef0

add test for process.exit for import running for worker thread

0a4c11f

implement unregistration of workers from the hooks thread upon exit

3ee4295

address review finding

f233e04

fix comment

c014520

fix or skip tests that are not fit for running on worker threads

e8708bd

fix flaky test with debug build

a25aab1

dygabo force-pushed the spawn-only-one-loader-thread branch from ff0c7c0 to a25aab1 Compare May 7, 2024 19:27

This comment was marked as outdated.

Sign in to view

fixup! fix flaky test with debug build

7659901

Flarna added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label May 7, 2024

This comment was marked as outdated.

Sign in to view

GeoffreyBooth added the commit-queue Add this label to land a pull request using GitHub Actions. label May 8, 2024

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels May 8, 2024

GeoffreyBooth approved these changes May 8, 2024

View reviewed changes

GeoffreyBooth added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels May 8, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 8, 2024

nodejs-github-bot merged commit 22cb99d into nodejs:main May 8, 2024
62 checks passed

dygabo deleted the spawn-only-one-loader-thread branch May 8, 2024 04:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

module: have a single hooks thread for all workers #52706

module: have a single hooks thread for all workers #52706

dygabo commented Apr 26, 2024 •

edited

nodejs-github-bot commented Apr 26, 2024

targos commented Apr 26, 2024

GeoffreyBooth left a comment

JakobJingleheimer left a comment

This comment was marked as resolved.

JakobJingleheimer left a comment

dygabo commented Apr 29, 2024

Flarna Apr 29, 2024

JakobJingleheimer Apr 29, 2024

bmeck left a comment

This comment was marked as outdated.

This comment was marked as outdated.

nodejs-github-bot commented May 8, 2024

nodejs-github-bot commented May 8, 2024

nodejs-github-bot commented May 8, 2024

module: have a single hooks thread for all workers #52706

module: have a single hooks thread for all workers #52706

Conversation

dygabo commented Apr 26, 2024 • edited

nodejs-github-bot commented Apr 26, 2024

targos commented Apr 26, 2024

GeoffreyBooth left a comment

Choose a reason for hiding this comment

JakobJingleheimer left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

JakobJingleheimer left a comment

Choose a reason for hiding this comment

dygabo commented Apr 29, 2024

Flarna Apr 29, 2024

Choose a reason for hiding this comment

JakobJingleheimer Apr 29, 2024

Choose a reason for hiding this comment

bmeck left a comment

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

nodejs-github-bot commented May 8, 2024

nodejs-github-bot commented May 8, 2024

nodejs-github-bot commented May 8, 2024

dygabo commented Apr 26, 2024 •

edited