Skip to content

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

Merged
nodejs-github-bot merged 13 commits into
nodejs:mainfrom
dygabo:spawn-only-one-loader-thread
May 8, 2024
Merged

module: have a single hooks thread for all workers#52706
nodejs-github-bot merged 13 commits into
nodejs:mainfrom
dygabo:spawn-only-one-loader-thread

Conversation

@dygabo
Copy link
Copy Markdown
Member

@dygabo dygabo commented Apr 26, 2024

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
Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/loaders

@nodejs-github-bot 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 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 dygabo force-pushed the spawn-only-one-loader-thread branch from 1025b03 to fa24d02 Compare April 26, 2024 14:03
@dygabo dygabo marked this pull request as draft April 26, 2024 14:08
@dygabo dygabo force-pushed the spawn-only-one-loader-thread branch from fa24d02 to ee16c17 Compare April 26, 2024 14:21
@targos
Copy link
Copy Markdown
Member

targos commented Apr 26, 2024

@nodejs/loaders

Copy link
Copy Markdown
Member

@GeoffreyBooth GeoffreyBooth left a comment

Choose a reason for hiding this comment

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

Great work!

Comment thread lib/internal/modules/esm/loader.js Outdated
@GeoffreyBooth GeoffreyBooth marked this pull request as ready for review April 26, 2024 15:50
@dygabo dygabo force-pushed the spawn-only-one-loader-thread branch from 03f69a8 to 2fdf7b3 Compare April 27, 2024 04:57
Copy link
Copy Markdown
Member

@JakobJingleheimer JakobJingleheimer left a comment

Choose a reason for hiding this comment

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

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

a few nits, and a couple small tweaks requested.

Comment thread lib/internal/modules/esm/hooks.js
Comment thread lib/internal/modules/esm/hooks.js Outdated
Comment thread lib/internal/modules/esm/hooks.js Outdated
Comment thread lib/internal/modules/esm/hooks.js
Comment thread lib/internal/modules/esm/hooks.js
Comment thread lib/internal/modules/esm/worker.js
Comment thread lib/internal/worker.js Outdated
Comment thread lib/internal/worker.js Outdated
Comment thread lib/internal/worker.js
Comment thread lib/internal/worker.js Outdated
Comment thread lib/internal/modules/esm/worker.js Outdated
Comment thread lib/internal/modules/esm/worker.js Outdated
Comment thread lib/internal/modules/esm/hooks.js Outdated
Comment thread lib/internal/modules/esm/hooks.js Outdated
dygabo

This comment was marked as resolved.

Copy link
Copy Markdown
Member

@JakobJingleheimer JakobJingleheimer left a comment

Choose a reason for hiding this comment

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

🙌

Comment thread lib/internal/modules/esm/worker.js Outdated
@dygabo
Copy link
Copy Markdown
Member Author

dygabo commented Apr 29, 2024

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