Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move ESM loaders off-thread #44710

Merged
merged 126 commits into from Apr 13, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
126 commits
Select commit Hold shift + click to select a range
03f6bb4
esm: move hook execution to separate thread
JakobJingleheimer Dec 19, 2022
a0bb205
esm: resolve optionally returns import assertions
GeoffreyBooth Jan 10, 2023
cd16ccd
port updates to `worker_thread`
JakobJingleheimer Dec 20, 2022
01f1191
move `HooksProxy` to `hooks.js`
JakobJingleheimer Dec 20, 2022
5f02297
`ESMLoader::addCustomLoaders` → `ESMLoader::registerCustomLoaders`
JakobJingleheimer Dec 20, 2022
820aa55
fix startup issues in hooks worker
JakobJingleheimer Dec 20, 2022
231b964
WIP: move importing custom loaders back to main thread
JakobJingleheimer Dec 20, 2022
ed412d5
🎉 `/test-esm-example-loader.mjs` is passing!!
JakobJingleheimer Dec 22, 2022
4776a6d
Fix tests that are loading modules/esm/worker.js on the main thread
GeoffreyBooth Dec 23, 2022
2aed49a
WIP: convert resolve to sync
JakobJingleheimer Dec 23, 2022
78a2889
Fix test
GeoffreyBooth Dec 26, 2022
5746d17
Restore parentURL validation to defaultResolve (and therefore also to…
GeoffreyBooth Dec 25, 2022
c1afb3d
Improve deserialization error (sometimes)
GeoffreyBooth Dec 26, 2022
666ee10
Rename #hooks to #chains
GeoffreyBooth Dec 26, 2022
8198a13
Subclass customized module loader
GeoffreyBooth Dec 26, 2022
2d39e13
Fix import.meta being undefined (WIP)
GeoffreyBooth Dec 27, 2022
dd6a5d3
Remove undocumented globalPreload setImportMetaCallback (WIP)
GeoffreyBooth Dec 28, 2022
1415d6e
fs.write(1, message) works at least in Mac and Windows for printing f…
GeoffreyBooth Dec 28, 2022
a374bd1
Standardize names, fix lint
GeoffreyBooth Dec 28, 2022
98b5512
Exceptions thrown inside loader hooks should be returned to the main …
GeoffreyBooth Dec 28, 2022
7be92f3
Rewrite test-esm-loader-hooks test to spawn child process and inspect…
GeoffreyBooth Dec 29, 2022
382753a
Send loader initialization errors up to the main thread so they can b…
GeoffreyBooth Dec 30, 2022
2b3e6a5
Remove debugging code (fixes 3 tests!)
GeoffreyBooth Dec 30, 2022
c73001f
Catch another worker thread exception so that the process doesn't hang
GeoffreyBooth Dec 30, 2022
b4cccd4
Add error handling around hooks method responses being undefined or h…
GeoffreyBooth Dec 30, 2022
b272d31
separate worker `data` & `lock` buffers
JakobJingleheimer Dec 31, 2022
0ec518a
chunks working?? (code is very rough!)
JakobJingleheimer Jan 4, 2023
11c1471
Fix import.meta.resolve
GeoffreyBooth Jan 7, 2023
0dd5114
More carefully pass errors back to the main thread
GeoffreyBooth Jan 8, 2023
d15b425
Some errors span multiple chunks, apparently
GeoffreyBooth Jan 8, 2023
b796c02
Fix chunking by separating metadata into separate buffers; fix error …
GeoffreyBooth Jan 9, 2023
599a616
unlock worker thread
targos Jan 2, 2023
42d485e
use syncCommPort to exchange data too
targos Jan 2, 2023
8e3e0c6
fix --import
targos Jan 9, 2023
76e23d8
fix doclint
targos Jan 10, 2023
75fe62b
remove internal check
targos Jan 10, 2023
350351f
refactor worker exception handling
targos Jan 10, 2023
f3b2ed9
errors during load should just be rethrown
targos Jan 10, 2023
7e79b2f
run global preload scripts in the main thread
targos Jan 11, 2023
c0142d6
nits
aduh95 Jan 11, 2023
5d7e3b4
nits
aduh95 Jan 11, 2023
685590c
fix linter errors
aduh95 Jan 11, 2023
49a4088
Add `HookProxy.prototype.makeAsyncRequest`
aduh95 Jan 11, 2023
d40834d
Wrap messages passed from worker to main thread so that we can carry …
GeoffreyBooth Jan 12, 2023
47bd0bf
Emit experimental warning only once
GeoffreyBooth Jan 12, 2023
17ff524
Don't re-throw uncaught exceptions and then re-catch them ad infinitum
GeoffreyBooth Jan 12, 2023
2065222
fix linter errors
aduh95 Jan 12, 2023
b0cc21f
transfer `ArrayBuffer`s
aduh95 Jan 12, 2023
29d019f
improve exception serialization
aduh95 Jan 12, 2023
8706cbd
chaining loader hooks
aduh95 Jan 12, 2023
04ebc5c
make things more consistent
aduh95 Jan 12, 2023
8aae17e
remove unnecessary parenthesis
aduh95 Jan 12, 2023
b74dcaa
simplify error cloning
aduh95 Jan 12, 2023
3312b1c
small optimisation: read error properties only once when cloning
aduh95 Jan 12, 2023
7e8be7c
skip already cloned properties
aduh95 Jan 12, 2023
7b7e5be
DRY
aduh95 Jan 13, 2023
3f4c9bf
fix `test-esm-loader-chaining`
aduh95 Jan 13, 2023
140c03b
update docs
aduh95 Jan 13, 2023
020b9d7
fix `waitAsync` usage
aduh95 Jan 13, 2023
6899203
simplify `ArrayBuffer` transfer
aduh95 Jan 13, 2023
9d392ef
do not die waiting on worker to respond,
aduh95 Jan 13, 2023
1c7dd71
`import()` should not block the main thread, even for the resolve step
aduh95 Jan 13, 2023
acfa8d9
rework docs
aduh95 Jan 13, 2023
1821301
do not keep the worker alive when awaiting hooks to resolve
aduh95 Jan 13, 2023
b1dcccc
keep the worker thread alive longer
aduh95 Jan 14, 2023
0cfc683
let uncatcught exceptinnon be uncaught exceptions
aduh95 Jan 14, 2023
7fd6fa1
use `serializeError` instead of custom logic
aduh95 Jan 14, 2023
a3982ee
fix unserializable errors handling
aduh95 Jan 14, 2023
47b4d83
only count async messages
aduh95 Jan 14, 2023
d518e53
remove global error handler only at the end of the sync messages
aduh95 Jan 14, 2023
531c9a3
use `'beforeExit'` event to detect when threads are ready to die
aduh95 Jan 14, 2023
3bcf18c
ensure there are no pending messages before letting worker die
aduh95 Jan 15, 2023
b071a89
linter fixes
aduh95 Jan 15, 2023
0911d15
get rid of the tick counter timeout
aduh95 Jan 15, 2023
0b1c22b
Use `Atomics.load` for better results
aduh95 Jan 17, 2023
88686f5
add `shared_constants` internal module to make code clearer
aduh95 Jan 18, 2023
1b1583e
rename `ModuleLoader` -> `createModuleLoader`
aduh95 Jan 18, 2023
3f4934a
lazy load esmLoader
aduh95 Jan 18, 2023
800d151
nits
aduh95 Jan 19, 2023
a5a798b
fix test-esm-loader
aduh95 Feb 10, 2023
f9371ae
unref worker more often
aduh95 Feb 10, 2023
0b79809
add more comments
aduh95 Feb 10, 2023
0cfec42
fix main thread refusing to die waiting on a response that will never…
aduh95 Feb 12, 2023
281e943
remove unrelated/unnecessary changes in `test/`
aduh95 Feb 12, 2023
3e31a1c
remove unrelated change in `src/`
aduh95 Feb 12, 2023
595a95f
restore changes related to sync resolve
aduh95 Feb 12, 2023
10bf7b8
Revert "unref worker more often"
aduh95 Feb 12, 2023
106b400
nits
aduh95 Feb 12, 2023
cc9373a
use `console.log` instead `fs.writeSync`
aduh95 Feb 13, 2023
a67af11
fix lint
aduh95 Feb 16, 2023
aab57d7
add comment to describe the shared memory organisation
aduh95 Feb 18, 2023
638b497
Remove unnecessary `Atomics.store` calls
aduh95 Feb 18, 2023
e340d24
s/BORED/IDLE/
aduh95 Feb 18, 2023
018ab18
add `SHARED_MEMORY_BYTE_LENGTH` to shared_constants
aduh95 Feb 18, 2023
80cd74c
add definition for idleness
aduh95 Feb 18, 2023
817d0e8
Update doc/api/esm.md
aduh95 Mar 1, 2023
0590df4
Update lib/internal/modules/esm/shared_constants.js
aduh95 Mar 6, 2023
cee03fa
add/fix comments + nits
aduh95 Mar 6, 2023
258cf8a
for..of -> classic for
aduh95 Mar 6, 2023
8863cb9
s/NUMBER_OF_INCOMING_MESSAGES/NUMBER_OF_MESSAGES_IN_TRANSIT/
aduh95 Mar 8, 2023
13f3d8d
add explanation for how `process.cwd()` can fail
JakobJingleheimer Mar 9, 2023
4731362
Apply suggestions from code review
aduh95 Mar 9, 2023
12717f7
fix worker permissions for internal workers
aduh95 Mar 9, 2023
61ad2a5
fix dead lock
aduh95 Mar 9, 2023
fa29a70
Apply suggestions from code review
aduh95 Mar 9, 2023
159ca6f
Apply suggestions from code review
aduh95 Mar 9, 2023
b4a8a2f
`git mv lib/internal/modules/esm/worker.js lib/internal/main/loader_t…
aduh95 Mar 10, 2023
6177afa
add comment
aduh95 Mar 10, 2023
a85288d
revert `git mv lib/internal/modules/esm/worker.js lib/internal/main/l…
aduh95 Mar 10, 2023
be8090e
never-settling `import.meta.resolve`
aduh95 Mar 10, 2023
f6679bc
reuse the public port instead of creating an additional communication…
aduh95 Mar 10, 2023
722fd0e
fix: disable test's case concurrency to avoid deadlock ~1/10 times
JakobJingleheimer Mar 13, 2023
544bf89
fix rare deadlock
JakobJingleheimer Mar 14, 2023
88f9042
update Hooks entry in to note use of threads and caveat for async ops
JakobJingleheimer Mar 20, 2023
6d8bfc6
chore: de-lint md
JakobJingleheimer Mar 20, 2023
31e7d5d
switch test fixtures from `console.log` → `writeSync(1, …)`
JakobJingleheimer Mar 23, 2023
372065e
fix loaders with node:fs
targos Mar 27, 2023
244f916
use more writeSync
targos Mar 28, 2023
bc8e970
remove remaining `await` in front of `import.meta.resolve`
aduh95 Apr 2, 2023
f44a246
resolve TODOs for edge cases
aduh95 Apr 2, 2023
221f2e2
fix race condition that could cause a dead lock when dealing with nev…
aduh95 Apr 4, 2023
9436c97
reduce the size of the sab and introduce a `never-setlle` message status
aduh95 Apr 11, 2023
ad73db6
fix race condition?
aduh95 Apr 12, 2023
95052c6
tidy code, comments, and docs
JakobJingleheimer Apr 12, 2023
acc0900
Update lib/internal/worker.js
aduh95 Apr 12, 2023
f02012f
Update lib/internal/worker.js
aduh95 Apr 12, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
39 changes: 25 additions & 14 deletions doc/api/esm.md
Expand Up @@ -7,6 +7,9 @@
<!-- YAML
added: v8.5.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/44710
description: Loader hooks are executed off the main thread.
- version:
- v18.6.0
- v16.17.0
Expand Down Expand Up @@ -325,6 +328,9 @@ added:
- v13.9.0
- v12.16.2
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/44710
description: This API now returns a string synchronously instead of a Promise.
- version:
- v16.2.0
- v14.18.0
Expand All @@ -340,29 +346,26 @@ command flag enabled.
* `specifier` {string} The module specifier to resolve relative to `parent`.
* `parent` {string|URL} The absolute parent module URL to resolve from. If none
is specified, the value of `import.meta.url` is used as the default.
* Returns: {Promise}
* Returns: {string}

Provides a module-relative resolution function scoped to each module, returning
the URL string.
the URL string. In alignment with browser behavior, this now returns
synchronously.

<!-- eslint-skip -->
> **Caveat** This can result in synchronous file-system operations, which
> can impact performance similarly to `require.resolve`.

```js
const dependencyAsset = await import.meta.resolve('component-lib/asset.css');
const dependencyAsset = import.meta.resolve('component-lib/asset.css');
```

`import.meta.resolve` also accepts a second argument which is the parent module
from which to resolve from:

<!-- eslint-skip -->
from which to resolve:

```js
await import.meta.resolve('./dep', import.meta.url);
import.meta.resolve('./dep', import.meta.url);
```

This function is asynchronous because the ES module resolver in Node.js is
allowed to be asynchronous.

## Interoperability with CommonJS

### `import` statements
Expand Down Expand Up @@ -730,6 +733,11 @@ A hook that returns without calling `next<hookName>()` _and_ without returning
`shortCircuit: true` also triggers an exception. These errors are to help
prevent unintentional breaks in the chain.

Hooks are run in a separate thread, isolated from the main. That means it is a
different [realm](https://tc39.es/ecma262/#realm). The hooks thread may be
terminated by the main thread at any time, so do not depend on asynchronous
operations to (like `console.log`) complete.

#### `resolve(specifier, context, nextResolve)`

<!-- YAML
Expand Down Expand Up @@ -762,7 +770,7 @@ changes:
Node.js default `resolve` hook after the last user-supplied `resolve` hook
* `specifier` {string}
* `context` {Object}
* Returns: {Object}
* Returns: {Object|Promise}
JakobJingleheimer marked this conversation as resolved.
Show resolved Hide resolved
* `format` {string|null|undefined} A hint to the load hook (it might be
ignored)
`'builtin' | 'commonjs' | 'json' | 'module' | 'wasm'`
Expand All @@ -772,6 +780,9 @@ changes:
terminate the chain of `resolve` hooks. **Default:** `false`
* `url` {string} The absolute URL to which this input resolves

> **Caveat** Despite support for returning promises and async functions, calls
> to `resolve` may block the main thread which can impact performance.

The `resolve` hook chain is responsible for telling Node.js where to find and
how to cache a given `import` statement or expression. It can optionally return
its format (such as `'module'`) as a hint to the `load` hook. If a format is
Expand All @@ -797,7 +808,7 @@ Node.js module specifier resolution behavior_ when calling `defaultResolve`, the
`context.conditions` array originally passed into the `resolve` hook.

```js
export async function resolve(specifier, context, nextResolve) {
export function resolve(specifier, context, nextResolve) {
const { parentURL = null } = context;

if (Math.random() > 0.5) { // Some condition.
Expand Down Expand Up @@ -1100,7 +1111,7 @@ const baseURL = pathToFileURL(`${cwd()}/`).href;
// CoffeeScript files end in .coffee, .litcoffee, or .coffee.md.
const extensionsRegex = /\.coffee$|\.litcoffee$|\.coffee\.md$/;

export async function resolve(specifier, context, nextResolve) {
export function resolve(specifier, context, nextResolve) {
if (extensionsRegex.test(specifier)) {
const { parentURL = baseURL } = context;

Expand Down
89 changes: 54 additions & 35 deletions lib/internal/main/worker_thread.js
Expand Up @@ -10,6 +10,7 @@ const {
ObjectDefineProperty,
PromisePrototypeThen,
RegExpPrototypeExec,
SafeWeakMap,
globalThis: {
Atomics,
SharedArrayBuffer,
Expand Down Expand Up @@ -89,23 +90,25 @@ port.on('message', (message) => {
const {
argv,
cwdCounter,
filename,
doEval,
workerData,
environmentData,
publicPort,
filename,
hasStdin,
manifestSrc,
manifestURL,
hasStdin,
publicPort,
workerData,
} = message;

if (argv !== undefined) {
ArrayPrototypePushApply(process.argv, argv);
}
if (doEval !== 'internal') {
if (argv !== undefined) {
ArrayPrototypePushApply(process.argv, argv);
}

const publicWorker = require('worker_threads');
publicWorker.parentPort = publicPort;
publicWorker.workerData = workerData;
const publicWorker = require('worker_threads');
publicWorker.parentPort = publicPort;
publicWorker.workerData = workerData;
}

require('internal/worker').assignEnvironmentData(environmentData);

Expand Down Expand Up @@ -138,31 +141,47 @@ port.on('message', (message) => {
debug(`[${threadId}] starts worker script ${filename} ` +
`(eval = ${doEval}) at cwd = ${process.cwd()}`);
port.postMessage({ type: UP_AND_RUNNING });
if (doEval === 'classic') {
const { evalScript } = require('internal/process/execution');
const name = '[worker eval]';
// This is necessary for CJS module compilation.
// TODO: pass this with something really internal.
ObjectDefineProperty(process, '_eval', {
__proto__: null,
configurable: true,
enumerable: true,
value: filename,
});
ArrayPrototypeSplice(process.argv, 1, 0, name);
evalScript(name, filename);
} else if (doEval === 'module') {
const { evalModule } = require('internal/process/execution');
PromisePrototypeThen(evalModule(filename), undefined, (e) => {
workerOnGlobalUncaughtException(e, true);
});
} else {
// script filename
// runMain here might be monkey-patched by users in --require.
// XXX: the monkey-patchability here should probably be deprecated.
ArrayPrototypeSplice(process.argv, 1, 0, filename);
const CJSLoader = require('internal/modules/cjs/loader');
CJSLoader.Module.runMain(filename);
switch (doEval) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: it would be great to somehow directly connect these values to where they ultimately come from (lib/internal/worker.js). as it is, they're kind of the string version of magic numbers that sort of happen to agree.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit (maybe out of scope): these cases are mutually exclusive with no fall-through. I think it might be cleaner and easier to grok as a map instead of a switch (where one might forget to break), like

const evaluators = {
  classic(filename) {},
  default(filename) {},
  internal(filename) {
    require(filename;
  },
  module(filename) {},
};

evaluators[doEval ?? 'default'](filename);

case 'internal': {
// Create this WeakMap in js-land because V8 has no C++ API for WeakMap.
internalBinding('module_wrap').callbackMap = new SafeWeakMap();
require(filename)(workerData, publicPort);
break;
}

case 'classic': {
const { evalScript } = require('internal/process/execution');
const name = '[worker eval]';
// This is necessary for CJS module compilation.
// TODO: pass this with something really internal.
ObjectDefineProperty(process, '_eval', {
__proto__: null,
configurable: true,
enumerable: true,
value: filename,
});
ArrayPrototypeSplice(process.argv, 1, 0, name);
evalScript(name, filename);
break;
}

case 'module': {
const { evalModule } = require('internal/process/execution');
PromisePrototypeThen(evalModule(filename), undefined, (e) => {
workerOnGlobalUncaughtException(e, true);
});
break;
}

default: {
// script filename
// runMain here might be monkey-patched by users in --require.
// XXX: the monkey-patchability here should probably be deprecated.
ArrayPrototypeSplice(process.argv, 1, 0, filename);
const CJSLoader = require('internal/modules/cjs/loader');
CJSLoader.Module.runMain(filename);
break;
}
}
} else if (message.type === STDIO_PAYLOAD) {
const { stream, chunks } = message;
Expand Down