Module customization hooks and worker threads

[GeoffreyBooth]

With regard to these issues and PRs discussed in yesterday’s TSC meeting:

• esm: spawn only one hooks thread node#50752
• module: have a single hooks thread for all workers node#52706
• Module Hooks cannot be registered from worker thread in 22.2.0+ node#53182
• Revert "module: have a single hooks thread for all workers" node#53183
• module: allow module.register from workers node#53200

It appears that we have a solution in nodejs/node#53200. I spoke to @dygabo today (the author of that PR) and he thinks that he’s fixed the bug and satisfied all the notes that people have left, other than @mcollina’s request (which I’ll get to in a minute).

In particular, what nodejs/node#53200 fixes is to make the prior PR, nodejs/node#52706, work as intended: calling register from new Worker execArgv no longer causes a hang, but registers hooks on the one hooks thread that nodejs/node#52706 created (replacing the prior architecture of separate hooks threads per worker thread). This has been the intended architecture since we first moved the hooks off-thread, and has been the plan since 2022. The issue I opened last year about the bug where there are multiple hooks threads, nodejs/node#50752, has 22 👍 s and this architecture was discussed in loaders meetings with stakeholders so i do think it’s the design favored by the majority of the community, though clearly there are some who prefer the alternative approach and consider moving to a single thread to be a regression.

In particular, @mcollina’s objection to nodejs/node#53200 is that it doesn’t revert the change intended by nodejs/node#52706: he wants multiple hooks threads, though he didn’t object to nodejs/node#52706 or the previous threads where this was discussed.

What’s on main right now is what no one wants; either we should fix the single-hooks-thread implementation or revert it. On the one hand I sympathize with the users like Matteo who were affected by this change, as even the bugfixed version will still cause them to need to update their code to compensate for the new architecture; the Angular team has already done so in angular/angular-cli#27721, which fixes their app even for Node 22.2.0 with the hanging bug. That PR demonstrates a workaround for the new behavior, for people who really want scoped hooks: use child processes instead of worker threads. I assume this approach is probably slower, but it works. And it doesn’t need to be the end state; we could very well ship a new way to provide scoped hooks, and a discussion has begun in nodejs/node#53195 to brainstorm options.

The option I don’t think we’re likely to choose is the prior state of how things were in 22.1.0, where every worker thread got its own hooks thread, regardless if that’s what the user wanted. Many of the options we’re discussing involve scoping the hooks in a more limited way, where they can behave differently per thread but still run on a single hooks thread rather than N hooks threads. So I’m hesitant to revert to the 22.1.0 behavior (which, to be clear, was buggy and unintentional) when I expect it will just change again. My preference would be to land nodejs/node#53200 to fix the current implementation, land at least some documentation explaining the child processes workaround, and continue to evaluate options for the scoped hooks feature request and potentially land something to satisfy those use cases when we have a solution that seems good to everyone. This would minimize churn, as the majority of users who aren’t trying to get scoped hooks won’t be affected by the future additions to support scoped hooks.

Alternatively we could revert back to 22.1.0 and then eventually land the change that moves to a single hooks thread alongside some new API that provides scoped hooks. This would involve more churn for users, and probably push out the overall API becoming stable for quite a while.

cc @nodejs/loaders @nodejs/tsc

[jasnell]

Let's revert the release branch to deal with the reported breakage there and get a new release out, even if that means continuing with the original bug that this change was meant to fix.

In main we have a couple possible paths forward, I think @mcollina has an alternative approach in mind and we're just waiting for the PR to be opened.

[mcollina]

I think when unexpected breakage od this magnitude happens, it's best to revert an reapply later. This situation has brought up a few use cases that were covered by unit tests but not in the radar of the loaders team. It's better to regroup and reassess the situation.

Telling people that were using threads before to use processes instead is 10 years backwards in the history of Node.js. I think we are past that point. If this is the recommendation, we should remove threads. In all intents and purposes I cannot endorse that as a technical priority for the project.

[GeoffreyBooth]

Something that was expressed to me about the idea of hooks threads that correspond to loader pools is that such an architecture is very complicated both to build and to reason about, and would entail significant additional infrastructure: we’d need to provide APIs for spawning a defined number of hooks threads, for determining when to terminate the hooks threads (should they survive the pool they were meant to serve?) and so on. The intended design of a single hooks thread that all process threads share is much simpler.

Consider the meeting where this was decided, back in 2022:

Quick update from our recent team meeting: We invited Gil Tayar (author/maintainer of several pertinent libraries) to discuss spawning a dedicated loaders thread per user-land thread, and he noted that will add enormous complexity to library authors (on top of the extra complexity on node’s side). We decided for an initial implementation, it would be better to use a single loaders thread shared by all user-land threads and add caveats to the relevant sections of the docs. If there is sufficient appetite, we can subsequently add a configuration option (perhaps to the Worker constructor) to spawn dedicated loaders threads.

It’s more likely that a hooks author will want to share hooks state between all threads of the process, than to have separate hooks state per application thread. A transpilation hook will want to transpile a file once and keep the result in memory, not transpile it again and again for each thread. A testing framework might want to create a mock that applies to all threads. Instrumentation libraries would want to have only one copy of their library running, not one copy per thread. I feel pretty sure that a single hooks thread should be our default behavior, and something more sophisticated can come later (or not at all; I’m genuinely concerned about our ability to maintain even the level of complexity that we already have). I think we could add workerData to the context object passed into each hook, so that the hooks can know about the threads that processed modules are coming from and act accordingly (to give a particular mock to a particular thread, say) and that should suffice for most use cases.

Ultimately this is about the conception of how people think about threads. Are they part of an existing app, spreading out CPU-intensive to multiple cores, or are they basically pseudo-child processes where you can launch mini-Nodes in different ways to do various different things? The desire to have different hooks for different threads relates to the latter concept, which is why child processes are a workable alternative. We only have this issue in the first place because of new Worker execArgv, which is a Node invention.

[jasnell]

Let's separate the concerns here.

Issue 1: Reverting the change in the release branch to unbreak the folks this change broke. This should be our immediate first action.

Issue 2: What is the long term correct fix and what steps do we take to get there that (a) accomplishes the goal AND (b) minimizes breaks in the ecosystem. There is a path forward here where we are working with the ecosystem and not in spite of them.

Before we tackle issue 2, let's deal with issue 1 which is the more pressing matter.

[mcollina]

The current list of broken things:

1. mocking tools (esmock)
2. frontend frameworks (Angular)
3. testing tools (ava)

The latter is quite important, because it's built around the concept of using worker threads for isolation. The community is telling us it is a valid use case for threads.

(Note that I'm 100% supportive of sharing one loader thread, I think this implementation landed too early).

I consider not restoring this functionality as soon as possible is disrespectful for our users. How many wasted hours of dev time should we push to the module authors?

[GeoffreyBooth]

The current list of broken things:

Can you share the issues for esmock and ava? I’ve only seen the issue for Angular.

[mcollina]

There aren’t public issues, I’ve just tried them.

[GeoffreyBooth]

Issue 1: Reverting the change in the release branch to unbreak the folks this change broke. This should be our immediate first action.

Issue 2: What is the long term correct fix and what steps do we take to get there that (a) accomplishes the goal AND (b) minimizes breaks in the ecosystem. There is a path forward here where we are working with the ecosystem and not in spite of them.

So say we do step 1 here, of reverting on the release branch. Then presumably we land nodejs/node#53200 on main, and both it and the predecessor nodejs/node#52706 get tagged as don’t-backport to any release line. Then what?

We can’t restore the 22.1.0 behavior and migrate to a single hooks thread. So even after the revert in 22.3.0 changes us back to the 22.1.0 behavior, eventually we’re going to ship another intentional change that will force some people to react to. So now we cause changes twice: in 22.3.0, and again in some future version when we try again to ship the “one hooks thread” design, possibly with something in addition to opt into scoped hooks. Maybe that’s better than just shipping the bugfix? I dunno. All I know is that I’ve heard very consistent feedback that library authors want us to minimize the number of changes they need to react to.

With regard to Ava, it already has a --no-worker-threads option to tell it to run tests in child processes rather than worker threads: https://github.com/avajs/ava/blob/main/docs/08-common-pitfalls.md#nodejs-command-line-options-child-processes-and-worker-threads. So it’s already been shipping the same workaround that Angular built.

I don’t know what issue esmock could be having. It only added a reference to worker_threads two weeks ago: iambumblehead/esmock@0a2a93a. There are no references to new Worker in its codebase.

Lastly, it may be that I was wrong to assume that worker threads are faster than child processes; apparently it’s the reverse: https://github.com/orgs/nodejs/discussions/44264. So encouraging people to migrate from new Worker to child_process.fork might actually be a good recommendation, and perhaps there’s not much reason for us to build complicated hooks thread orchestration APIs when child_process.fork already exists and is probably faster. And anyone using worker threads to get scoped loaders today, who migrate to child processes, won’t need to do another migration later on regardless of how hooks and threads may evolve.

[dygabo]

Please consider my two cents.

Note: If no change in behaviour compared to the behaviour in 22.1.0 is acceptable, then the only short term way forward is the revert.

Having said that, when I started working actively on this was after some time of following the discussions (on and off). From my perspective we needed a solution to reduce the number of threads from 2*N to N+1 in order to reduce resource consumption, etc.

This kind of change is intrusive by definition and IMO it is expected that changes in behaviour will occur. We unfortunatelly introduced the hanging process bug with that PR (unintentedly, needles to say). #53200 tries to fix that. But one more decision of #52706 (obviously not an easy one) was that module.register can only be called by the main thread and we postponed the work for a solution on that.

Supporting module.register from an arbitrary thread introduces one more issue. If we do that, we need to ensure that the thread calling module.register has ownership of the hook inside the hook chain. Otherwise the hook chain might grow indefinitely. This might have effects on scenarios like "a whole thread pool shares a hook". If these are discussed and agreed upon, we should also put that in the documentation as a limitation or thing that hooks implementors have to think about.

Personally, I would opt for a solution based on nodejs/node#53200 (comment). But #53200 needs some work after the last discussions and it would be good to be able to do that work without the release next week pressure.

I also think that the API openness of both worker_threads and loader hooks (which is a good thing to begin with) can create a few runtime scenarios that are hard to diagnose in troubleshooting scenarios. These should be somehow clearly documented.

And then there's the concern of hooks polluting the state on the hooks thread in a way that impacts other threads. I think our open interface makes room for this kind of bugs (that isolation of worker threads tries to solve)

[mcollina]

I personally think this is fixable, and a design that can cater to most use cases can be done. Even if not, it should be clearly communicated ahead of time and circulated widely. What we need to do now is to restore a situation that did not break users, and then design something that can work for everybody.

[aduh95]

Let's land the revert now, and then let's open a "revert revert" PR that can also land immediately with dont-land- labels. That way, 22.x gets back the old behavior while we can keep iterating on main until we're happy with the behavior, at which point we can remove the labels.

[ShogunPanda]

@aduh95 When you talk about revert revert it basically means reapplying the original nodejs/node#52706 to main? If is that so, wouldn't be easier to just land nodejs/node#53183 on v22.x only?

[mcollina]

@ShogunPanda I think the double revert make the job easier for the releasers.