Repository navigation
esm, loader: move to own thread #43658
Description
What is the problem this feature will solve?
- limit contamination
- facilitate synchronous
import.meta.resolve()
For the initial implementation, the same loaders thread will be used for all user-land threads. A subsequent enhancement may add a configuration option to the Worker constructor to spawn its own dedicated loaders thread.
What is the feature you are proposing to solve the problem?
Move loaders off-thread
What alternatives have you considered?
No response
Per direction from TSC
Following in the vein of babel: babel/babel#14025
And https://github.com/bmeck/using-node-workshop/tree/main/steps/6_async_and_blocking
- notify package authors of change (please comment to be added if not already included—sorry I know only so many)
- esmock
- jest?
- mocha (Gil Tayar)
- ts-node (Andrew Bradley)
- yarn (Maël Nison)
Activity
JakobJingleheimer commentedon Jul 2, 2022
@bmeck @MylesBorins could either of you speak to the impetus for this?
aduh95 commentedon Jul 5, 2022
Related: #31229
GeoffreyBooth commentedon Jul 20, 2022
Pros and cons here: nodejs/modules#351 (comment)
cspotcode commentedon Jul 20, 2022
Can add to the list of package authors:
esmock
I wonder, does it make sense for the loaders team to have a thread somewhere which can be subscribed to for notifications of breaking changes? All relevant discussion can happen elsewhere, the thread would be like an RSS feed. Package maintainers can opt-in to notifications of breaking or potentially exciting/disruptive changes by subscribing to that thread.
Might scale better than us hoping we know a comprehensive list of all loaders.
JakobJingleheimer commentedon Jul 20, 2022
Since Node.js doesn't maintain anything of the sort, that sounds like a better alternative to "surprise!". It's not full-proof, though.
JakobJingleheimer commentedon Sep 17, 2022
We have a working Proof of Concept: https://github.com/JakobJingleheimer/worker-atomics
[-]esm, loaders: move to own thread[/-][+]esm, loader: move to own thread[/+]loynoir commentedon Oct 5, 2022
devkarlson commentedon Oct 19, 2022
7 remaining items
GeoffreyBooth commentedon Oct 23, 2022
There's also a potential benefit in moving non-custom loading off-thread as it would protect internals from prototype pollution (I think). That would argue that we should make this same refactor for CommonJS too.
aduh95 commentedon Oct 23, 2022
@mcollina I'm not convinced we can answer those questions before we have a fully working implementation; without data, we can only make assumptions, and I wouldn't want us to draw a conclusion over possibly baseless assumptions.
mcollina commentedon Oct 24, 2022
Absolutely! I'm concerned about the addition to our startup memory footprint, as this matters for some of our usecases.
I'm flagging that this might be problematic and I was surprised because it was not mentioned in the main text of the issue.
A few more question:
JakobJingleheimer commentedon Oct 24, 2022
In both cases, (in the current design/implementation) only if the "loaders" worker is not around (otherwise it will be re-used).
mcollina commentedon Oct 24, 2022
So there is only one loaders worker for all worker threads created by Node.js?
targos commentedon Oct 24, 2022
No, there's a separate loaders worker for each worker thread.
Flarna commentedon Oct 24, 2022
I assume it should be possible to configure loader hooks per worker thread therefore this may complicate this thread.
I'm also a bit skeptical to have a single loader thread for all workers as this seems to allow "leaking" data between workers which should be isolated. Also this single loader worker would be parallelism blocker.
mhdawson commentedon Nov 1, 2022
The comment from @targos and @JakobJingleheimer seem contradictory?
cspotcode commentedon Nov 1, 2022
There are 3 different threading models being considered, that I'm aware of.
JakobJingleheimer commentedon Nov 1, 2022
Sorry, I think #43658 (comment) is the intended behaviour (the current PR may not achieve that yet). The rationale being that workers are intended to be isolated, so their loaders' state should also be isolated/fresh.
JakobJingleheimer commentedon Nov 13, 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
Workerconstructor) to spawn dedicated loaders threads.mcollina commentedon Nov 14, 2022
Was there any progress on not spawning any thread if no custom loaders are defined?
JakobJingleheimer commentedon Nov 15, 2022
Yep! nodejs/loaders#118 (comment) and I believe this should be logistically/technically possible.