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.

Sign up for GitHub

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

Jump to bottom

esm: do not lazy instantiate loaders to avoid infinite loop #47599

Closed
wants to merge 4 commits into from
Closed

esm: do not lazy instantiate loaders to avoid infinite loop #47599

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5ee35c5
esm: do not lazy instantiate loaders to avoid infinite loop
aduh95 Apr 17, 2023
94797cd
mock `import`
aduh95 Apr 17, 2023
ffbdec4
rename the function `init`, run it from `main/` files
aduh95 Apr 18, 2023
758fd93
nits
aduh95 Apr 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion lib/internal/main/check_syntax.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,8 @@ function loadESMIfNeeded(cb) {
const hasModulePreImport = getOptionValue('--import').length > 0;

if (hasModulePreImport) {
const { loadESM } = require('internal/process/esm_loader');
const { loadESM, init } = require('internal/process/esm_loader');
init();
loadESM(cb);
return;
}
Expand Down
7 changes: 5 additions & 2 deletions lib/internal/main/eval_stdin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,15 @@ readStdin((code) => {

const print = getOptionValue('--print');
const loadESM = getOptionValue('--import').length > 0;
if (getOptionValue('--input-type') === 'module')
if (getOptionValue('--input-type') === 'module') {
require('internal/process/esm_loader').init();
evalModule(code, print);
else
} else {
evalScript('[stdin]',
code,
getOptionValue('--inspect-brk'),
print,
loadESM);
require('internal/process/esm_loader').initIfNeeded();
}
});
7 changes: 5 additions & 2 deletions lib/internal/main/eval_string.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,11 @@ markBootstrapComplete();
const source = getOptionValue('--eval');
const print = getOptionValue('--print');
const loadESM = getOptionValue('--import').length > 0;
if (getOptionValue('--input-type') === 'module')
const esmLoader = require('internal/process/esm_loader');
if (getOptionValue('--input-type') === 'module') {
esmLoader.init();
evalModule(source, print);
else {
} else {
// For backward compatibility, we want the identifier crypto to be the
// `node:crypto` module rather than WebCrypto.
const isUsingCryptoIdentifier =
Expand All @@ -54,4 +56,5 @@ else {
getOptionValue('--inspect-brk'),
print,
loadESM);
esmLoader.initIfNeeded();
}
1 change: 1 addition & 0 deletions lib/internal/main/repl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ if (process.env.NODE_REPL_EXTERNAL_MODULE) {
}

const esmLoader = require('internal/process/esm_loader');
esmLoader.init();
esmLoader.loadESM(() => {
console.log(`Welcome to Node.js ${process.version}.\n` +
'Type ".help" for more information.');
Expand Down
2 changes: 2 additions & 0 deletions lib/internal/main/run_main_module.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ prepareMainThreadExecution(true);

markBootstrapComplete();

require('internal/process/esm_loader').init();

// Necessary to reset RegExp statics before user code runs.
RegExpPrototypeExec(/^/, '');

Expand Down
3 changes: 3 additions & 0 deletions lib/internal/main/test_runner.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ if (isUsingInspector()) {
inspectPort = process.debugPort;
}

// We might need ESM loader for custom test reporters.
require('internal/process/esm_loader').initIfNeeded();

run({ concurrency, inspectPort, watch: getOptionValue('--watch'), setup: setupTestReporters })
.once('test:fail', () => {
process.exitCode = kGenericUserError;
Expand Down
3 changes: 3 additions & 0 deletions lib/internal/main/worker_thread.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,11 +162,13 @@ port.on('message', (message) => {
});
ArrayPrototypeSplice(process.argv, 1, 0, name);
evalScript(name, filename);
require('internal/process/esm_loader').initIfNeeded();
break;
}

case 'module': {
const { evalModule } = require('internal/process/execution');
require('internal/process/esm_loader').init();
PromisePrototypeThen(evalModule(filename), undefined, (e) => {
workerOnGlobalUncaughtException(e, true);
});
Expand All @@ -179,6 +181,7 @@ port.on('message', (message) => {
// XXX: the monkey-patchability here should probably be deprecated.
ArrayPrototypeSplice(process.argv, 1, 0, filename);
const CJSLoader = require('internal/modules/cjs/loader');
require('internal/process/esm_loader').initIfNeeded();
CJSLoader.Module.runMain(filename);
break;
}
Expand Down
6 changes: 6 additions & 0 deletions lib/internal/modules/esm/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,11 @@ async function initializeHooks() {
const hooks = new Hooks();

const { DefaultModuleLoader } = require('internal/modules/esm/loader');
const { init } = require('internal/process/esm_loader');
class ModuleLoader extends DefaultModuleLoader {
// eslint-disable-next-line no-useless-constructor
constructor() { super(); }

loaderType = 'internal';
async #getModuleJob(specifier, parentURL, importAssertions) {
const resolveResult = await hooks.resolve(specifier, parentURL, importAssertions);
Expand Down Expand Up @@ -156,6 +160,8 @@ async function initializeHooks() {

const preloadScripts = hooks.initializeGlobalPreload();

init(privateModuleLoader);

return { __proto__: null, hooks, preloadScripts };
}

Expand Down
44 changes: 41 additions & 3 deletions lib/internal/process/esm_loader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,59 @@
'use strict';

const {
ArrayPrototypePush,
PromisePrototypeThen,
ReflectApply,
SafeMap,
} = primordials;

const assert = require('internal/assert');
const { createModuleLoader } = require('internal/modules/esm/loader');
const { getOptionValue } = require('internal/options');
const {
hasUncaughtExceptionCaptureCallback,
} = require('internal/process/execution');
const { pathToFileURL } = require('internal/url');
const { kEmptyObject } = require('internal/util');
const { kEmptyObject, createDeferredPromise } = require('internal/util');

let esmLoader;
let init = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let init = false;
let initialized = false;


module.exports = {
get esmLoader() {
return esmLoader ??= createModuleLoader(true);
return esmLoader ??= { __proto__: null, cjsCache: new SafeMap(), import() {
Copy link
Member

Choose a reason for hiding this comment

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

cjsCache? What is this? Is this trying to track the callers that led to the creation of this ESM loader?

const { promise, resolve, reject } = createDeferredPromise();
ArrayPrototypePush(this.importRequests, { arguments: arguments, resolve, reject });
return promise;
}, importRequests: [] };
},
initIfNeeded() {
// TODO: we could try to avoid loading ESM loader on CJS-only codebase
Copy link
Member

@joyeecheung joyeecheung Apr 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think we should initialize the ESM loader in CJS-only code base. At the very least the ESM internals must be snapshotable before we consider this (I suspect if we somehow make that happen, e.g. #45828, which requires resolving some of the the problematic circular dependencies, the bug here already might go away..)

Copy link
Contributor Author

@aduh95 aduh95 Apr 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We have to, because dynamic import() uses ESM loader. As I said in the TODO comment, we could instantiate the ESM loader only on the first dynamic import.

Copy link
Member

@joyeecheung joyeecheung Apr 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

we could instantiate the ESM loader only on the first dynamic import.

We have already been doing it in the main branch:

node/lib/internal/modules/cjs/loader.js

Line 1196 in 2ec4ef7

const cascadedLoader = getCascadedLoader();
this would only be a TODO if we first regress by eager loading the ESM loader.

return module.exports.init();
},
init(loader = undefined) {
Comment on lines +30 to +34
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit convoluted, and not clear when which pieces should be called. Could it be baked into the esmLoader getter?

assert(!init);
Copy link
Contributor

Choose a reason for hiding this comment

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

won't this throw? (I would think if (init) return)

init = true;

loader ??= createModuleLoader(true);

if (esmLoader != null) {
for (const { 0: key, 1: value } of esmLoader.cjsCache) {
// Getting back the values from the mocked loader.
loader.cjsCache.set(key, value);
}
for (let i = 0; i < esmLoader.importRequests.length; i++) {
PromisePrototypeThen(
ReflectApply(loader.import, loader, esmLoader.importRequests[i].arguments),
esmLoader.importRequests[i].resolve,
esmLoader.importRequests[i].reject,
);
}
}
esmLoader = loader;
},
async loadESM(callback) {
esmLoader ??= createModuleLoader(true);
const { esmLoader } = module.exports;
try {
const userImports = getOptionValue('--import');
if (userImports.length > 0) {
Expand Down
2 changes: 1 addition & 1 deletion lib/internal/process/pre_execution.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,8 +120,8 @@ function prepareExecution(options) {
}

function setupUserModules() {
initializeCJSLoader();
initializeESMLoader();
initializeCJSLoader();
const CJSLoader = require('internal/modules/cjs/loader');
assert(!CJSLoader.hasLoadedAnyUserCJSModule);
loadPreloadModules();
Expand Down
39 changes: 39 additions & 0 deletions test/es-module/test-esm-loader-hooks.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,4 +195,43 @@ describe('Loader hooks', { concurrency: true }, () => {
assert.strictEqual(code, 0);
assert.strictEqual(signal, null);
});

Copy link
Member

Choose a reason for hiding this comment

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

Let’s add a test for the simplest case, ./node --loader ./test/fixtures/empty.js.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tests that deal with REPL are simply not convenient and hard to not have flaky. Let's not do that?

it('should let users require and import along loaders', async () => {
const { code, signal, stdout, stderr } = await spawnPromisified(execPath, [
'--no-warnings',
'--require',
fixtures.path('printA.js'),
'--import',
fixtures.fileURL('printB.js'),
'--experimental-loader',
fixtures.fileURL('empty.js'),
'--eval',
'setTimeout(() => console.log("C"),99)',
]);

assert.strictEqual(stderr, '');
assert.match(stdout, /^A\r?\nA\r?\nB\r?\nC\r?\n$/);
assert.strictEqual(code, 0);
assert.strictEqual(signal, null);
});

it('should let users require and import along loaders with ESM', async () => {
const { code, signal, stdout, stderr } = await spawnPromisified(execPath, [
'--no-warnings',
'--require',
fixtures.path('printA.js'),
'--import',
fixtures.fileURL('printB.js'),
'--experimental-loader',
fixtures.fileURL('empty.js'),
'--input-type=module',
'--eval',
'setTimeout(() => console.log("C"),99)',
]);

assert.strictEqual(stderr, '');
assert.match(stdout, /^A\r?\nA\r?\nB\r?\nC\r?\n$/);
assert.strictEqual(code, 0);
assert.strictEqual(signal, null);
});
});
5 changes: 5 additions & 0 deletions test/parallel/test-bootstrap-modules.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,11 @@ const expectedModules = new Set([
'NativeModule internal/net',
'NativeModule internal/dns/utils',
'NativeModule internal/process/pre_execution',
'NativeModule internal/modules/esm/loader',
'NativeModule internal/process/esm_loader',
'NativeModule internal/modules/esm/assert',
'NativeModule internal/modules/esm/module_map',
'NativeModule internal/modules/esm/translators',
Comment on lines +96 to +100
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are we OK with this change? /cc @joyeecheung

Copy link
Member

@joyeecheung joyeecheung Apr 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

From the description in the issue:

I think this is happening because the loader is going to try to spawn a worker thread from within the worker thread, which is also going to do the same thing, and that's how we end up with Mr Meeseeks situation until the system is out of memory or the process terminates. I'm working on a fix.

I am not very convinced that this is the right fix. It seems some refactoring or synchronization should be done to avoid this instead. The ESM loader is full of circular dependencies and weird TDZs so I am not quite surprised that this happens, as that has already resulted in a deadlock in that off-thread PR before. I don't think eager loading actually make those problematic dependencies go away, and I suspect some other internal code changes that alter the dependency in internal/modules/esm could re-introduce this again even if we eager-initialize them in the outmost main scripts.

Copy link
Member

Choose a reason for hiding this comment

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

Now that off-thread has landed I would be happy to combine some of the ESM files to make them snapshotable. I assume the goal is to add as much of the ESM loader as possible to the snapshot? I think it’s safe to say that ESM code is common enough at this point that most projects are likely to have at least some ESM code loaded between the main app and/or its dependencies.

Not that we should do this refactor in this PR, but I’m assuming it’s something we’d like to do?

Copy link
Member

@joyeecheung joyeecheung Apr 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think that might just happen to be an alternative fix to the bug. Regardless of whether that's the right fix, I still don't think what this PR does the right fix - there are several comments in the loader assuming that there can only be only worker being spawned. If the code in the loader implementation is not enough to guard against infinite worker spawning, then it's a bug in the loader implementation (which I suspect is a result of circular dependency + TDZ invalidating the guards). The loader had been lazy-loadable without infinite worker spawning, the fact that it now does is a regression of the loader implementation. Eager-loading the loader only sweeps the bug even further deeper under the rug.

To quote myself from 4 months ago: #45828 (comment)

This refactoring is just reduces the technical debt (circular dependencies, TDZs, synchronous side-effect/runtime state queries at module loading time etc.) in the code case. The sooner it lands, the less likely that future code have to pile more technical debt onto the codebase (unless they repeat the refactoring themselves). #43772 is fine in that regard, as it is not adding any more of those, so it would not be difficult for that PR to rebase onto this or the other way around. But the current code in #44710 is still doing those things, I'd argue that code that does those stuff really should not land in the code base in the first place. We should not count on future refactoring to pay off the technical debt, or we still end up with the minefield we have now in lib/internal/modules/esm.

And I suspect that we are getting hit by the technical debt again and we are trying to run away from actually cleaning up the minefield again.

Copy link
Member

Choose a reason for hiding this comment

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

So what fix do you recommend @joyeecheung? If I understand you correctly, you agree that adding more of the ESM loader into the snapshot is a fix and is also something we want to do anyway—but you think that this particular bug should be fixed first before taking that step, to avoid sweeping this bug under the rug?

I think what @aduh95 is aiming for here is to try to be explicit about when we spawn the worker thread. Maybe we get that right first and then deal with the snapshotting?

Copy link
Member

Choose a reason for hiding this comment

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

you agree that adding more of the ESM loader into the snapshot is a fix

That wasn't what I was saying. I was suggesting that this could be caused by circular dependencies in the ESM loader, which is also a blocker of the snapshotability of the ESM (there are other blockers).

I think what @aduh95 is aiming for here is to try to be explicit about when we spawn the worker thread. Maybe we get that right first and then deal with the snapshotting?

My point is that I am not convinced getting this right requires eager-loading the ESM loader. The assumption that fixing this requires eager-loading the ESM loader might be as incorrect as the assumption that supporting import() in CJS requires eager-loading the ESM loader. The code here only moves the timing of which the entire ESM loader (custom and/or default) is created, unconditional to wether the graph involves ESM or not. I don't think that's explicit about when the worker thread should be spawned, the worker spawning code is still buried within the hooks creation code which is buried in the loader code, so it's still implicit when the worker is actually created.

Eager-loading the ESM loader leads to an unnecessary penalty to startup performance of CJS-only graphs and I don't think we should do that until the ESM loader can be snapshotted (to reduce the performance penalty). So if we have to go down the eager-loading ESM loader path, I would say including them into the snapshot is a blocker - but then I don't think that's the only path. As I said before, this more likely requires synchronization within the loader code itself to avoid the infinite spawning. What this PR currently does does not seem to introduce synchronization to the loader code, it only works around the bug by creating the loader before anything else, which isn't robust.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Point taken, I still think the current implementation that lazy instantiate the loader is a mistake, I'll try to see if I can find another way without introducing non-snapshotable module in the bootstrap path. Help welcome if someone has another implementation candidate.

Copy link
Member

@joyeecheung joyeecheung Apr 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

From what I can tell by just looking at the code (without being able to reproduce this locally), the suspicious code is:

  1. In the internal loader worker, the custom loaders are handled in initializeHooks(), called by the internal worker script

    node/lib/internal/modules/esm/utils.js

    Line 103 in 4667b07

    const customLoaderPaths = getOptionValue('--experimental-loader');
  2. Then again in the internal worker, some code could attempt to invoke createModuleLoader() again if they access require('internal/process/esm_loader').esmLoader (e.g. for handling dynamic import)

    node/lib/internal/modules/esm/loader.js

    Line 415 in 4667b07

    const userLoaderPaths = getOptionValue('--experimental-loader');
    , which has no idea that in this worker, the custom loaders are already handled, and it just creates an unnecessary CustomizedModuleLoader which creates more workers.

I think the proper synchronization should be done in createModuleLoader instead to make sure that when it's invoked within a internal loader worker - it can know that by just looking at some variable set by the worker main script, or the hooks proxy can use a different internal argument (instead of reusing --experimental-loader) to pass the loaders to the worker - it does not create that worker by constructing a CustomizedModuleLoader again. From what I can tell it probably should just return the default loader in the internal loader worker. I am not quite sure what this PR is currently doing but it looks like it's trying to achieve the same thing in a rather convoluted way - by introducing an intricate initialization order that somehow ends up leading to a mocked version of the loader in the esmLoader ??= statement when it's accessed within the internal worker. Which is...not very robust, and I doubt if the mock actually works for dynamic imports initiated in the custom loaders. Also from what I can tell I am not convinced this fix really needs eager-loading ESM loader, that looks like a by-product of the introduction of the intricate initialization order, which doesn't seem to be the right fix in the first place.

Copy link
Member

@joyeecheung joyeecheung Apr 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I have an alternate fix here: #47620 with a test that locally reproduces the bug for me. I am not sure if I am supposed to invest energy to push it forward soon (still recovering from COVID), I think I should keep myself away from the computer for the day :3 feel free to take over if I disappear from GitHub.

]);

if (!common.isMainThread) {
Expand Down