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:

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

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).

I just rebased on main. I was able to reproduce the issues cited above. To summarize:

./node --trace-uncaught --test ./test/es-module/test-esm-loader-chaining.mjs
TAP version 13
node:internal/test_runner/harness:37
      throw err;
      ^

Error [ERR_UNHANDLED_ERROR]: Unhandled error. ('This Environment was initialized without a V8::Inspector')
    at new NodeError (node:internal/errors:400:5)
    at InternalWorker.emit (node:events:508:17)
    at [kOnErrorMessage] (node:internal/worker:312:10)
    at [kOnMessage] (node:internal/worker:323:37)
    at MessagePort.<anonymous> (node:internal/worker:218:57)
    at [nodejs.internal.kHybridDispatch] (node:internal/event_target:734:20)
    at exports.emitMessage (node:internal/per_context/messageport:23:28) {
  code: 'ERR_UNHANDLED_ERROR',
  context: 'This Environment was initialized without a V8::Inspector'
}

./node --eval 'console.log(1)'
node:internal/modules/cjs/loader:1165
    throw err;
    ^

TypeError: Cannot read properties of undefined (reading 'set')
    at internalCompileFunction (node:internal/vm:102:17)
    at wrapSafe (node:internal/modules/cjs/loader:1141:20)
    at Module._compile (node:internal/modules/cjs/loader:1182:27)
    at runScript (node:internal/process/execution:82:27)
    at evalScript (node:internal/process/execution:104:10)
    at node:internal/main/eval_string:50:3

./node --input-type=module --eval 'console.log(1)'
^C # Kill hung process

@JakobJingleheimer @nodejs/v8 @targos @MoLow I’ve made a small discovery that might point to the cause of the This Environment was initialized without a V8::Inspector error. So per above we were getting that error by running ./node --trace-uncaught --test ./test/es-module/test-esm-loader-chaining.mjs. If you make the following change to src/env-inl.h:

inline bool Environment::should_create_inspector() const {
-  return (flags_ & EnvironmentFlags::kNoCreateInspector) == 0 &&
-         !options_->test_runner && !options_->watch_mode;
+  return (flags_ & EnvironmentFlags::kNoCreateInspector) == 0;
}

Then the error disappears, and the process hangs like it does for ./node --input-type=module --eval 'console.log(1)'. An __error.log file is generated, containing this:

TypeError [ERR_INVALID_ARG_VALUE]: The argument 'filename' must be a file URL object, file URL string, or absolute path string. Received undefined
    at new NodeError (node:internal/errors:400:5)
    at createRequire (node:internal/modules/cjs/loader:1337:11)
    at file:///Users/geoffrey/Sites/node/test/common/index.mjs:3:17
    at ModuleJob.run (node:internal/modules/esm/module_job:194:25)

While this kNoCreateInspector flag was added in #35025, the second line about !options_->test_runner && !options_->watch_mode was added in #44366. So it appears that something about --test is causing this error. This might be a bug in general, not specific to this PR, but I don’t have time at the moment to try to reproduce it in a minimal context (cc @MoLow).

@JakobJingleheimer For the purposes of this PR I would just test it using a command other than --test, and hopefully the --test issue can be investigated separately.

While this kNoCreateInspector flag was added in #35025, the second line about !options_->test_runner && !options_->watch_mode was added in #44366. So it appears that something about --test is causing this error. This might be a bug in general, not specific to this PR, but I don’t have time at the moment to try to reproduce it in a minimal context (cc @MoLow).

the objective of the change in #44366 was to make --inspect (with all of its variations) only affect the "inner" process since both --test and --watch have a wrapping process with internal node code, --inspect should not open a debugger session on the external process.

we might want to only pass --trace-uncaught to the inner process and ignore it in the wrapper process

./node --trace-uncaught ./test/es-module/test-esm-loader-chaining.mjs

is now passing, and errors from the worker are now surfaced (and the process terminates gracefully)

Something related to the --test trigers such an error

./node --trace-uncaught --test ./test/es-module/test-esm-loader-chaining.mjs

Also, I believe I've found the cause of the v8.serialize() failure for

./node --input-type="module" -e "console.log(1)"

ESMLoader::eval() returns a Module instance, and Module::wrapper is a Proxy, which is not serialisable. I'll dig into that (chatted with Geoffrey, and we have a couple avenues to try).

the objective of the change in #44366 was to make --inspect (with all of its variations) only affect the “inner” process since both --test and --watch have a wrapping process with internal node code, --inspect should not open a debugger session on the external process.

we might want to only pass --trace-uncaught to the inner process and ignore it in the wrapper process

@MoLow it seems like this change breaks ./node --trace-uncaught --test ./test/es-module/test-esm-loader-chaining.mjs when worker threads are used, which happens as a result of the changes in this PR. I suspect that node --test some-code-that-spawns-workers.js is probably also broken. Do you have time to look into this? Perhaps there’s a different way to prevent the inspector from covering the “wrapper” process, or we can revert that change for now (to un-break using --test with workers) and add it back once a new method is determined?

@GeoffreyBooth Il take a deep look at this on this weekend

@GeoffreyBooth @JakobJingleheimer I tryed reproducing with

// worker.js
'use strict';

const { parentPort } = require('worker_threads');

parentPort.on('message', (msg) => {
  parentPort.postMessage(msg);
  setTimeout(() => { throw new Error('test'); }, 10);
});

import { describe, it } from 'node:test'
import { deepEqual, fail } from 'node:assert'
import { Worker } from 'node:worker_threads'



it('test', async () => {
  const worker = new Worker("./worker.js");
  worker.on('online', console.log);
  worker.postMessage('hello');
  await new Promise((resolve) => {
    worker.on('message',(...args) => {
      deepEqual(args, ['hello']);
      setTimeout(() => {
        worker.terminate();
        resolve();
      }, 100);
    });
  });
});

but this does not seem to reproduce any issues with --trace-uncaught,
what am I missing?

@MoLow the issue is with --test (not --trace-uncaught)

I have tried both, and ran test-esm-loader-chaining as well. both with and without --test - was not able to reproduce

We realised the serialisation issues are indeed caused by Proxys in Module; one of the Proxies seems maybe unnecessary (a getter might suffice), but the other is very necessary (it may be possible to refactor it to something else, but it would likely be undesirable).

So we're planning to pivot the approach here: Instead of instantiating the whole public ESMLoader within the worker (what we're calling the "heavy" approach), we're going to pursue two others.

A "thin" worker that is basically just a sandbox; the public ESMLoader's relevant methods currently contain loops. With this approach, each iteration of the loop would make a request to the worker, wherein the hook is merely executed and its output returned as the worker's response. The benefits of this approach are that it's very simple (but proves it works); the cons are it will likely have unideal performance (due to many requests/responses, and thus a lot of serialisation and deserialisation).

A "thick" worker wherein we extract hook-related methods off the current ESMLoader class into a separate Hooks class (or other structure). When the worker is needed, it is spawns with the Hooks class inside; then the worker responds with the final result of a hook chain.

The response of both approaches has the same shape (a plain object with a few properties like source: Buffer | string).

We may first go the thin option as a PoC and then land the thick.