Hooks thread direction

[GeoffreyBooth]

So for the last six months or so, the top priority of the loaders work has been fixing the last bug standing in the way of stability, where the separate thread we spawn to run module customization hooks is unintentionally duplicated for each application worker thread rather than shared between all of them. A (stable) fix might now be almost at hand, but it was raised that we should perhaps reevaluate whether the intended behavior is what we want. As I see things, we have three options, with varying pros and cons:

1. Keep things as they are, where there’s one hooks thread per application thread.

   • Pro: Already built; the hooks are running in fully isolated environments separate from each other, similar to the isolation provided by the worker threads. (Note that other methods of isolation might be on the horizon such as NodeRealm, ShadowRealm or module compartments.)
   • Con: Wasteful of memory (an application with a main plus N worker threads will have N+1 hooks threads); and duplicative for many common use cases such as transpilers, where the same work such as transpiling a file might happen repetitively in each hooks thread rather than the result/state being shared between all of them.

2. Run the hooks in a dedicated thread that supports all threads in the process, with a separate chain of hooks for each application thread.

   • Pro: Conserves memory as compared with multiple hooks threads; sharing of state across all application threads can reduce processing for otherwise repeated work, or enable use cases like mocks that need to maintain state at the process level.
   • Con: Hooks would need to be intentional to keep state per thread rather than per process.

3. Move the module customization hooks back on the main thread and change the resolve and load hooks to be synchronous. (A big reason that the threads are off-thread currenly is so that async hooks can affect sync CommonJS require calls.) Similar to (or could be combined with/replaced by) @joyeecheung’s proposal for synchronous main thread hooks.

   • Pro: Could achieve many (all?) of the goals of Joyee’s proposal, of providing a supported API to replace CommonJS monkey-patching; avoids the overhead of spawning a separate hooks thread; probably easier to develop hooks for, assuming that isolation is not desired.
   • Con: All hooks would need to be sync functions; no isolation of hooks code from application code.

I’m not sure if it would be technically feasible to support async hooks for import and sync hooks for require; and on a product level I don’t think we want separate hooks APIs for require and import. If others diagree with either of those assessments then I guess we could have fourth and/or fifth options to consider. I honestly don’t have a preference between any of the options; I just want to get this API stable, ideally before Node 23 in October.

Aside from the overall “which option do you prefer” question, some other questions I have for the folks who have been with us on this journey:

• How necessary is it for the resolve and load hooks to be async? Do you commonly write async resolve and/or load hooks, where refactoring it to be sync would be difficult or impossible? (We are considering taking the “syncify async work via a separate thread” code from the hooks thread and splitting it off into a userland package that libraries could use as a general utility. We might also provide some kind of sync network request API similar to readFileSync.)

• How necessary is isolation for hooks code, and on what level is it desired? Even the current API doesn’t isolate hooks from other hooks running in the same chain; they’re only isolated from application code.

@nodejs/loaders plus the participants of #103: @cspotcode @giltayar @arcanis @bumblehead @bizob2828 plus participants of nodejs/node#53332: @VoltrexKeyva @Flarna @alan-agius4

[Qard]

My preference is 3. I don't feel we're getting much from loaders being off-thread, certainly not enough to justify the significant complexity.

Also, we don't necessarily need a thread to support async code behind a require, we just need related code to be on a separate event loop. Anna previously did a bunch of work in this realm with synchronous-worker. We could build a similar construct in to Node.js to contain related work we need to syncify.

Alternatively, we can also just look at what cases need async presently and figure out if we can just provide ways to do them synchronously. For example, http imports only need async because we don't have a sync http client.

[guybedford]

The work towards (2) is very impressive and seems like it is close to fruition in finding a consistent and sensible model. (3) is also viable provided that we could have a top-level async hook like preImport that runs for the main process import as well as for every dynamic import(). This would allow for real async work to still be able to happen in the loading pipeline. With access to be able to call the full chain for the resolve and load hooks it would be possible to still implement features like network imports using proper async implementations while still having sync hooks underneath.

I could support either option (2) or (3).

[Flarna]

I think (3) results in the easiest to understand and maintainable solution for node core and hooks maintainers. I see not that much of a gain we got from moving loaders off-thread. But I'm for sure biased a bit as an APM user.

One target of the off thread setup - to get rid of the memory allocated by hooks (e.g. transpilers,...) - is not even on the plate anymore. And I doubt this will be ever done looking at the complexity the off thread solution has already now.

[mcollina]

As for 2, I think it's the only way to keep the off-thread model. It's incredibly complex.

I think 3 is possible if we add an API or module to "deasync" based on spawned worker thread.
The use cases for resolve() and load() to be async exists, and I think we would want to keep supporting them somehow. This can be developed in the ecosystem, and iterated quickly outside of core. This has the benefit of pushing the complexity down into userland.

Essentially my preference is 3, 2, 1.

[JakobJingleheimer]

It sounds like many of us (myself included) don't know what the use-cases for async hooks are. My gut says there surely are legit ones. There is for sure complexity in supporting it, but that's mostly done. In that vein, I think: 2, 3, 1.

Perhaps Anna's solution is a better option for syncifying things. But I think we are all too tired of this to invest in a completely new approach.

If we can definitely say async hooks are not necessary (or too bespoke to warrant supporting in core): 3, 2, 1.

[giltayar]

It feels like the sentiment is option 3. And while some loaders need to be async, my guess is that most of them can be totally sync, and these sync loaders are today paying the overhead needed to support async loaders.

Feels like making loaders need sync, and forcing async loaders to syncify their stuff by them creating worker threads makes sense.

Makes simple things simple, make complex things possible. So, and I say it with a heavy hear, I prefer option 3. If not, option 2. Option 1, for my use case (mocking modules) makes things complex and really heavy on memory, so is not really an option in my opinion.

[giltayar]

But if option 3 is implemented, you should re-invite me to give the ESM loader talk again 😂.

[arcanis]

I'd prefer 3 as well. The current async off-thread model has been revealed relatively fragile, with various edge cases difficult to solve in userland, or even wrap our heads around.

[ShogunPanda]

While I worker on the multiple chain for 2, I confirm it was insanely complex before my changes and it will stay insanely complex after them. I agree with @mcollina that 2 is the only viable option for fully-async hooks.

But I do also prefer 3 as well, with a caveat: we need to another a new API that allows to synchronously wait for another spawned thread. This way we have the following benefits:

1. We unify (finally) CJS and ESM since everything is sync by default
2. We support (totally legit) async hooks needs

If can't provide this, then I suggest to keep 2.

[dygabo]

I think either 3 (preferred) or 2 (if there are usecases that are covered only by 2 that are absolutely necessary). But the latter needs more details/decisions concerning corner cases.

Also, IMO the maintenance difficulties for this part of the code that we get by using the off-thread solution need to be analyzed. Getting changes to work in a stable way is difficult with the off-thread variant, especially with the increased api surface because of the module.register() being allowed only on some of the threads with inheritance rules for descendants, etc. In case of the latest variant of the single thread for many workers I think the contributions of @ShogunPanda from #53332 go in the right direction for solving the problem but some cases on that branch and also on #53200 are not yet defined:

• correct cleanup after a worker has ended
• consolidated consensus on behavior of subscribing threads in case one of the subscribers causes the HooksThread to error. All subscribers should be ended if the HooksThread ends for whatever reason and that might create confusion and breaks isolation
• some connection to HooksThread issues as describer in module: allow module.register from workers node#53200. #53488 makes some steps to solve this.

fwiw, I think if the new proposal from @joyeecheung will get to cover the esm and cjs cases and adding the exports/link hook to run on-thread could be a good solution for APM needed instrumentations.

[joyeecheung]

we need to another a new API that allows to synchronously wait for another spawned thread.

It seems you are describing Atomics.wait(), which is what the current module.register() implementation already uses in the synchronous paths. In the user land, babel/register has been using it to wait on another thread to do asynchronous work in their (monkey-patched) CJS hooks too.

[mcollina]

@joyeecheung I mean that using Atomics.wait(), SharedArrayBuffer and MessagePort for doing synchronous communication is not straightforward. We could add an utility (in userland, maybe?) to to automate the creation of those components.

[joyeecheung]

Unless there is a standardized API for doing this, I think in general it's best to let the user land be creative when developing a helper that doesn't have to be implemented in Node.js core. When it gets mature enough or becomes a de-facto standard we can consider bringing it into core i.e. undici style. But developing it in core from scratch is suboptimial (think of release churn, CI flakes, compatibility across release lines, etc.). It can be a user-land package under the Node.js organization if necessary. Also since it likely only relies on Web and JS APIs this can be reused in browsers and other runtimes which can be better for the JS ecosystem.

[ShogunPanda]

@joyeecheung I agree. But I think we should have a standard way to do it ourself in order not to repeat in several parts of the codebase. We could even have this in NPM and eventually add it as a dep. WDYT?

[joyeecheung]

Having something on npm SGTM if someone is up for designing an API and implementing it (at this point I would be surprised if there aren’t already some existing helpers on npm TBH). Although that sounds like a something that can happen in parallel, not really a blocker for synchronous module hooks IMO.

[ShogunPanda]

Sounds fair. I'll develop an API soon.

[GeoffreyBooth]

Sounds fair. I’ll develop an API soon.

Possibly related: https://github.com/un-ts/synckit

[sheremet-va]

Vitest was hoping to use the Node.js loader API when it stabilizes. Unfortunately, Vitest uses Vite plugins to resolve import paths and load modules and they are always async - this is actually the main reason why Vitest exists in the first place (jest's sync nature was too limiting), so this is very much our use case:

• How necessary is it for the resolve and load hooks to be async? Do you commonly write async resolve and/or load hooks, where refactoring it to be sync would be difficult or impossible?

I am unaware of the current internal implementation, so I have a few questions that might be important to us.

What are the possible performance issues with manual Atomic.wait? Or does it just do that under the hood already? What about ESM? Does the default ESM loader fall back to the async implementation if the custom loader is async? (Meaning, it cannot be loaded with require under --experimental-require-module). We expect to transform every source code, and use the default loader for node_modules paths (but keep it customizable). Vitest also doesn't intend to transform CJS modules, it will always use the default loader in that case.

[joyeecheung]

What are the possible performance issues with manual Atomic.wait? Or does it just do that under the hood already?

module.register() (off-thread) already does Atomic.wait() (for require() customization in imported CJS) and Atomic.waitAsync() (others) under the hood for the off-thread hooks.

What about ESM? Does the default ESM loader fall back to the async implementation if the custom loader is async?

The in-thread hooks being proposed in #198 will only support synchronous hook functions, since it intends to replace the wide-spread monkey patching in the user land which are generally in-thread, synchronous hooks (e.g. see require-in-the-middle/pirates), as require() is synchronous. If users want to keep the hooks async, they could either continue using the off-thread hooks provided by module.register() (although people are finding it increasingly difficult to maintain due to the threading model, the last time the threading model was "corrected" as designed it caused regressions and got reverted, so it is uncertain when it'll become stable), or use the proposed in-thread module.registerHooks() with their own worker that does async work.

For a "loading plugins that are ESM" scenario, with the in-thread hooks users could still spawn their own worker and Atomics.wait() on it to get the results synchronously from the plugins, like what babel/register/experimental-worker does for its .mjs plugins. This model has already been implemented by the off-thread module.register() internally under the hood, the in-thread module.registerHooks() just transfer the choice/control/complexity of the additional worker over to the hooks authors. If ESM plugin loading is the only thing that forces the loader to be async, require(esm) can be used to synchronously load ESM plugins with the caveat that TLA won't be supported (it's likely that require(esm) will be unflagged before module.registerHooks() is released). So you will be basically choosing between synchronous + in-thread hooks v.s. asynchronous + off-thread hooks.

From a performance perspective, in-thread synchronous hooks will run typically faster than asynchronous off-thread hooks if the asynchronous work doesn't involve network access, because worker startup + locking + messaging overhead would be non-trivial, and asynchronous FS APIs have a lot of overhead that the synchronous FS APIs don't have. #198 (comment) might be an intersting data point (tsx's synchronous hooks based on CommonJS loader monkey patching is ~2.5x faster than module.register() based off-thread hooks when transpiling a simple ts file).