module: allow multiple chain

[ShogunPanda]

This PR is based on and includes commits from #53200.

Description

This PR introduces the following changes:

1. Introduction of the concept of "hooks chain" (see below)
2. Addition of a context parameters to the initialize hook. The context contains the registering thread ID.
3. Addition of the registering thread ID, and initialize's data to the context's properties passed to the resolve and load hooks.

How do "hooks chain" work?

Rather than maintaining a single chain of hooks for all threads as it is today, this PR introduces the concept of separate hooks chains.

Each thread has a separate chain registered inside the hooks thread. This chain is either copied from the parent thread when the thread is spawned or created from scratch if the thread is the first one calling module.register within the application code.

This also covers the case where the process.execArgv contains --import or --experimental-loader. Since the chain copied from the parent already contains the (already initialized) hooks the duplicate register detection will prevent the arguments to be processed again.

Thanks to threadId and the initialize's data this PR also covers the case where user want to use a singleton hook shared across threads. All these properties allow the hook to work differently depending on the context. Also, this will ensure that modification made to either parent or children after initialization don't affect other sibling threads.

The only case not covered at the moment from this PR is the one in which the user want a worker thread to start with an empty chain. This is already in theory achievable as we already have the methods to empty a chain, but these methods are not exposed externally. Once we expose those, this case will be covered as well, leaving no other open cases I'm aware of.

[nodejs-github-bot]

Review requested:

• @nodejs/loaders

[GeoffreyBooth]

In today's loaders meeting we discussed #53200 using BroadcastChannel to resolve a potential hang when the hooks thread is created by a register call from a worker thread; and this PR would remove the public facing clear API (we'll explore deregistration in a follow up).

[ShogunPanda]

@dygabo @GeoffreyBooth @mcollina I've updated the PR with the feedback from the loaders' meeting. I've also updated the PR description above with the changes in the behavior of the new functionality. PTAL

@dygabo There is only one test failing, and only in worker mode. I realized it happens due to the hang you described in #53200 (comment). Do you have bandwidth (and rough ETA) to fix it or do you want me to take care of that as well?

[dygabo]

There is only one test failing, and only in worker mode

I also put some time into it over the past few days but I have no immediate good news about it. It is difficult with the primitives that we have in this case to cover the usecase and come up with a race-free implementation of this. I still dig into the native code trying to come up with a solution but at the moment I am not very optimist about a satisfying result.

EDIT: of course, if you have ideas and can work on it, I am happy with it. You could even push to my fork if you want.

[ShogunPanda]

@dygabo @GeoffreyBooth @mcollina I've included this PR not to use workerData at all. While the idea was good, it was not feasible if it contained transferrable objects like MessagePort (since they can only exist on a thread). Since we still have data it should not be a problem.

[nodejs-github-bot]

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

[mcollina]

lgtm

@alan-agius4 can you take a look?

[mcollina]

@po there seem to be some related test failures https://ci.nodejs.org/job/node-test-commit-osx/59113/nodes=osx11-x64/testReport/junit/(root)/es-module/test_esm_hooks_chains/

[ShogunPanda]

@po there seem to be some related test failures https://ci.nodejs.org/job/node-test-commit-osx/59113/nodes=osx11-x64/testReport/junit/(root)/es-module/test_esm_hooks_chains/

I'll take a look soon.

PS: you got fooled from my multiple nicks, isn't it? XD

[Flarna]

Are we really sure that this single thread for all customization hooks is the way to go?

In my opinion the isolation topic is only partially solved as all hooks for all workers run in the same thread.

• hanging/slow hook effects all unrelated workers
• global state in a hook is visible to others
• crash/exit in one hook effects all
• a hook could monkey patch other hooks (or their dependencies)

By the way, how is the behavior if the hook thread exists?

• Does it end the process?
• Is a fresh worker recreated on demand if another worker imports a module?

[GeoffreyBooth]

Are we really sure that this single thread for all customization hooks is the way to go?

We can discuss it, though this PR is probably not the best place. Maybe #50752?

The intent with moving the hooks off-thread was primarily so that the hooks could apply to CommonJS while still being async. Isolation from application code was viewed as a nice side effect, though not a primary goal. Even in the current behavior, separate hooks in the same chain still affect each other, because we have only one hooks thread for all hooks, not separate hooks threads for each set of registered hooks.

The reasons for wanting one hooks thread for N application threads, instead of N hooks threads for N application threads, is performance (surely one thread is faster than N) and because most common use cases want shared state for the entire process. A TypeScript transpiler wants to cache its transpilation results globally, and not need to retranspile the same file N times for N threads.

[Flarna]

surely one thread is faster than N

Depends. For CPU bound work it's slower as only one CPU can be utilized by one worker. But memory wise it's better.
Anyhow, before digging into performance improvements we should be clear about functional part.

As long as current users of different loader chains like @alan-agius4 can't confirm that this limited isolation is fine I see the risk that this again breaks a lot users similar as the last try to go to a single thread did it.
Not because the initial decision was wrong - just because it's again a breaking change which introduces again a limitation not there before.

I agree that there are a lot valid usecases for global states but last approach clearly showed that there is more.

Anyhow, if all are sure that this is the way to go I still miss a well defined error handling in case one of the hooks shows a problematic behavior.
I'm fine to do this via follow-ups. But in that case we should mark this PR with a dont-and-on-v22 label and set a reminder to revert before v23 is created in case we are not able to finish this journey.

[alan-agius4]

As long as current users of different loader chains like @alan-agius4 can't confirm that this limited isolation is fine I see the risk that this again breaks a lot users similar as the last try to go to a single thread did it.

Yes would be totally fine for our use case.

[ShogunPanda]

Closing as hooks are going back in thread.