GeezerVison

Review requested:

• @nodejs/modules

Does each worker thread get its own loader thread? E.g. if an app launches 2x worker threads, there will be 6x threads total: one main, 2x worker, 3x loader?

The current design puts all loaders in the same worker thread (eg there are a total of 2 threads: the main and the worker). Internal things are handled via the main thread; userland things are handled by the worker thread.

I think that's slightly different than what I'm asking about. If a node app uses the worker threads API to launch worker threads, does each get its own loader thread? (where each loader thread may have one or more loaders running within it)

Not does each loader get a thread, but does each user thread get a loader thread.

If a node app uses the worker threads API to launch worker threads, does each get its own loader thread?

I think the current state (before this PR) is that loaders always execute in the main thread, even if they’re “for” user code that is in a worker thread. So I would think that after this PR, loaders would run inside their worker thread, regardless of whether they’re customizing main or worker thread user code.

So in other words, for an app using custom loaders where user code that spawns three workers, there are five threads total: main, loaders, and the three workers spawned by the user code.

This is just my guess, others can please correct me if I’m mistaken.

I don't know how it's implemented but I would expect that each Node.js thread (main and workers) has its own separate loaders thread, at least for two reasons:

• Worker threads are supposed to be as isolated as possible from the main thread and from each other
• You can spawn a worker thread with a different set of --loader flags.

What remains to be done?

Something in this implementation is causing node to hang on startup. We suspect it's some kind of circular dependency, and the subsequent dependency smacks into the Atomics lock (preventing the rest of the flow to complete, which would release the initial lock). I think this circular dep is between ESMLoader and Worker (it's rather difficult to troubleshoot as output is swallowed).

There is a working PoC, so we know this works in principle (and that the Atomics otherwise behave appropriately, so the problem lies with integrating it into node).

This was on hold whilst I was on holiday. I'm working on it today and tomorrow (but will be away on a business trip next week).

I found the circular dep:

process/esm_loader.js:12 ← modules/esm/public_loader_proxy.js:3 ← worker_thread.js:29/36 ← process/pre_execution.js:562 ← process/esm_loader.js

Gah, it wasn't a circular dependency 🤦‍♂️ it was setting url to null here:

https://github.com/nodejs/node/blob/9836c67198b2d01350f283e10b872849e81abefa/lib/internal/worker.js#L141-L149

which caused the worker to spawn with nothing in it.

(I shouldn't have copied it over from the previous attempt at off-threading, which was later hard-coding the url).

I'm not sure if the internal worker should go through the classic path, or merely get require()ed (I tried both, and it doesn't seem to make a difference).

There is (now was) a circular dependency issue after the empty worker issue is addressed in

https://github.com/nodejs/node/blob/9836c67198b2d01350f283e10b872849e81abefa/lib/internal/main/worker_thread.js#L142

that's now fixed too.

NODE_DEBUG=esm,worker ./out/Release/node -p 1 still doesn't print 1, BUT it does now print debugging output, and we can see it getting fairly far along now, so progress:

ESM 13018: hooking up public ESMLoader
ESM 13018: creating worker for publicESMLoader
[…]
WORKER 15812: instantiating Worker. url: file:///…/nodejs/node/lib/internal/modules/esm/worker.js doEval: classic
WORKER 15812: [0] created Worker with ID 1

esm/worker.js's debug (worker for public ESM running) is not printing, so that's likely why things have stalled (as soon as the worker is running, it releases the lock).

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

Landed in 4667b07

@JakobJingleheimer @aduh95 Oh, shit. This is merged? 😅 What version will it appear in? I want to check the testdouble loader to see if my changes to support off-thread loaders work.

@giltayar it will (hopefully) land on v20, see #47381 (comment)

kudos to everyone involved! This was a mammoth task! 7 months and 100+ commits 😳🙏!

I'm so glad to see this land! 😻

I maintain a test suite for interoperable path resolution¹, and I'm able to confirm that 4667b07 is compatible with browsers for synchronous import.meta.resolve(…). 🥳

What is the recommended way to pass data from the main thread to a loader? Thanks in advance for any reply

I see it is documented here (sorry for bothering) https://nodejs.org/api/esm.html#globalpreload I'll play around with this

We are unable to access the app path with ESM loaders being off threaded in v20 easily. See nodejs/help#4190
Wondering if you are aware of a better solution than the one I've shared in the help issue. Thanks

Looks like loaders no longer have access to process.argv

Looks like loaders no longer have access to process.argv

Correct; that is how worker threads work. There is a dedicated issue for this as well as a proposal to address (which is already linked).

I'm locking this as most of the recent comments are telling us the sky is blue.

It looks like this should be marked as a semver-major and we should avoid backporting it to v18.x, @nodejs/loaders thoughts?

Marking it as semver-major would be wrong I think, because it's an experimental API, but it could be labelled as dont-land-on-v18.x though.
That being said, I think we do want to backport it to v18.x, but that cannot happen until the ecosystem is ready for it, so backport-requested is actually the correct place for this PR to be atm if we follow that plan.