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

module: handle Top-Level Await non-fulfills better #34640

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
2 changes: 2 additions & 0 deletions doc/api/process.md
Original file line number Diff line number Diff line change
Expand Up @@ -2588,6 +2588,8 @@ cases:
and generally can only happen during development of Node.js itself.
* `12` **Invalid Debug Argument**: The `--inspect` and/or `--inspect-brk`
options were set, but the port number chosen was invalid or unavailable.
* `13` **Unfinished Top-Level Await**: `await` was used outside of a function
Copy link
Member

Choose a reason for hiding this comment

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

this will be used if any part of the import() process stalls, for example a loader returning a promise that never resolves.

Copy link
Member Author

Choose a reason for hiding this comment

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

Are you suggesting to explicitly list that special case? I would prefer not to, if you’re working on an ESM loader then you can probably imagine that import() stalling would look like unfinished TLA. The far more common case is going to be actual TLA usage, I assume

Copy link
Member

@devsnek devsnek Aug 6, 2020

Choose a reason for hiding this comment

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

No I'm just saying, with the current implementation, 13 doesn't actually imply the lack of promise resolution came from a source text module with TLA. I think the best we could do is moving the exitCode check to where module.evaluate() is called, although that still isn't inherently a source text module using TLA.

Copy link
Member Author

Choose a reason for hiding this comment

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

To be clear, I consider any kind of failure to finish the main module’s evaluation, whether through TLA or not, something that should not be considered a successful operation. This is intentional, and I would be against restricting the check here to only module.evaluate().

Copy link
Member

Choose a reason for hiding this comment

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

I agree we shouldn't consider those success cases. I'm saying, the message says "Unfinished Top-Level Await" but it can be caused by other things, which is potentially confusing. It might be better to rename it to "ESM Import Stalled" or something and mention that while many things can cause it, it would probably be caused by await (promise that never resolves)

Copy link
Member Author

Choose a reason for hiding this comment

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

I don’t think most people would know what “ESM Import Stalled” means.

Again, I think this is mostly relevant when you’re actually developing a loader or similar, and in that case you most likely don’t even need any explanation at all.

Copy link

@bricss bricss Aug 6, 2020

Choose a reason for hiding this comment

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

IMO, Stalled Top-Level Await will be the best mix of both 🙄

in the top-level code, but the passed `Promise` never resolved.
* `>128` **Signal Exits**: If Node.js receives a fatal signal such as
`SIGKILL` or `SIGHUP`, then its exit code will be `128` plus the
value of the signal code. This is a standard POSIX practice, since
Expand Down
24 changes: 22 additions & 2 deletions lib/internal/modules/run_main.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,29 @@ function runMainESM(mainPath) {
esmLoader.loadESM((ESMLoader) => {
addaleax marked this conversation as resolved.
Show resolved Hide resolved
const main = path.isAbsolute(mainPath) ?
pathToFileURL(mainPath).href : mainPath;
return ESMLoader.import(main);
handleMainPromise(ESMLoader.import(main));
});
}

function handleMainPromise(promise) {
// Handle a Promise from running code that potentially does Top-Level Await.
// In that case, it makes sense to:
// - Treat a rejection as an unhandled exception
// - Set the exit code to a specific non-zero value if the main code never
// finishes running.
function handler() {
if (process.exitCode === undefined)
process.exitCode = 13;
}
process.on('exit', handler);
promise
.finally(() => process.off('exit', handler))
.catch((err) => {
internalBinding('errors').triggerUncaughtException(
err, true /* fromPromise */);
});
}

// For backwards compatibility, we have to run a bunch of
// monkey-patchable code that belongs to the CJS loader (exposed by
// `require('module')`) even when the entry point is ESM.
Expand All @@ -62,5 +81,6 @@ function executeUserEntryPoint(main = process.argv[1]) {
}

module.exports = {
executeUserEntryPoint
executeUserEntryPoint,
handleMainPromise,
};
15 changes: 5 additions & 10 deletions lib/internal/process/execution.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

const {
JSONStringify,
PromiseResolve,
} = primordials;

const path = require('path');
Expand Down Expand Up @@ -43,20 +42,16 @@ function evalModule(source, print) {
if (print) {
throw new ERR_EVAL_ESM_CANNOT_PRINT();
}
const { log, error } = require('internal/console/global');
const { decorateErrorStack } = require('internal/util');
const { log } = require('internal/console/global');
const asyncESM = require('internal/process/esm_loader');
PromiseResolve(asyncESM.ESMLoader).then(async (loader) => {
const { handleMainPromise } = require('internal/modules/run_main');
handleMainPromise((async () => {
const loader = await asyncESM.ESMLoader;
const { result } = await loader.eval(source);
if (print) {
log(result);
}
})
.catch((e) => {
decorateErrorStack(e);
error(e);
process.exit(1);
});
})());
}

function evalScript(name, body, breakFirstLine, print) {
Expand Down
82 changes: 82 additions & 0 deletions test/es-module/test-esm-tla-unfinished.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
import '../common/index.mjs';
import assert from 'assert';
import child_process from 'child_process';
import fixtures from '../common/fixtures.js';

{
// Unresolved TLA promise, --eval
const { status, stdout, stderr } = child_process.spawnSync(
process.execPath,
['--input-type=module', '--eval', 'await new Promise(() => {})'],
{ encoding: 'utf8' });
assert.deepStrictEqual([status, stdout, stderr], [13, '', '']);
}

{
// Rejected TLA promise, --eval
const { status, stdout, stderr } = child_process.spawnSync(
process.execPath,
['--input-type=module', '-e', 'await Promise.reject(new Error("Xyz"))'],
{ encoding: 'utf8' });
assert.deepStrictEqual([status, stdout], [1, '']);
assert.match(stderr, /Error: Xyz/);
}

{
// Unresolved TLA promise with explicit exit code, --eval
const { status, stdout, stderr } = child_process.spawnSync(
process.execPath,
['--input-type=module', '--eval',
'process.exitCode = 42;await new Promise(() => {})'],
{ encoding: 'utf8' });
assert.deepStrictEqual([status, stdout, stderr], [42, '', '']);
}

{
// Rejected TLA promise with explicit exit code, --eval
const { status, stdout, stderr } = child_process.spawnSync(
process.execPath,
['--input-type=module', '-e',
'process.exitCode = 42;await Promise.reject(new Error("Xyz"))'],
{ encoding: 'utf8' });
assert.deepStrictEqual([status, stdout], [1, '']);
assert.match(stderr, /Error: Xyz/);
}

{
// Unresolved TLA promise, module file
const { status, stdout, stderr } = child_process.spawnSync(
process.execPath,
[fixtures.path('es-modules/tla/unresolved.mjs')],
{ encoding: 'utf8' });
assert.deepStrictEqual([status, stdout, stderr], [13, '', '']);
}

{
// Rejected TLA promise, module file
const { status, stdout, stderr } = child_process.spawnSync(
process.execPath,
[fixtures.path('es-modules/tla/rejected.mjs')],
{ encoding: 'utf8' });
assert.deepStrictEqual([status, stdout], [1, '']);
assert.match(stderr, /Error: Xyz/);
}

{
// Unresolved TLA promise, module file
const { status, stdout, stderr } = child_process.spawnSync(
process.execPath,
[fixtures.path('es-modules/tla/unresolved-withexitcode.mjs')],
{ encoding: 'utf8' });
assert.deepStrictEqual([status, stdout, stderr], [42, '', '']);
}

{
// Rejected TLA promise, module file
const { status, stdout, stderr } = child_process.spawnSync(
process.execPath,
[fixtures.path('es-modules/tla/rejected-withexitcode.mjs')],
{ encoding: 'utf8' });
assert.deepStrictEqual([status, stdout], [1, '']);
assert.match(stderr, /Error: Xyz/);
}
2 changes: 2 additions & 0 deletions test/fixtures/es-modules/tla/rejected-withexitcode.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
process.exitCode = 42;
await Promise.reject(new Error('Xyz'));
1 change: 1 addition & 0 deletions test/fixtures/es-modules/tla/rejected.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
await Promise.reject(new Error('Xyz'));
2 changes: 2 additions & 0 deletions test/fixtures/es-modules/tla/unresolved-withexitcode.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
process.exitCode = 42;
await new Promise(() => {});
1 change: 1 addition & 0 deletions test/fixtures/es-modules/tla/unresolved.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
await new Promise(() => {});
1 change: 0 additions & 1 deletion test/message/esm_display_syntax_error_import.out
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,3 @@ SyntaxError: The requested module '../fixtures/es-module-loaders/module-named-ex
at ModuleJob._instantiate (internal/modules/esm/module_job.js:*:*)
at async ModuleJob.run (internal/modules/esm/module_job.js:*:*)
at async Loader.import (internal/modules/esm/loader.js:*:*)
at async Object.loadESM (internal/process/esm_loader.js:*:*)
1 change: 0 additions & 1 deletion test/message/esm_display_syntax_error_import_module.out
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,3 @@ SyntaxError: The requested module './module-named-exports.mjs' does not provide
at ModuleJob._instantiate (internal/modules/esm/module_job.js:*:*)
at async ModuleJob.run (internal/modules/esm/module_job.js:*:*)
at async Loader.import (internal/modules/esm/loader.js:*:*)
at async Object.loadESM (internal/process/esm_loader.js:*:*)
6 changes: 3 additions & 3 deletions test/message/esm_loader_not_found_cjs_hint_bare.out
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
internal/process/esm_loader.js:*
internalBinding('errors').triggerUncaughtException(
^
internal/modules/run_main.js:*
internalBinding('errors').triggerUncaughtException(
^

Error [ERR_MODULE_NOT_FOUND]: Cannot find module '*test*fixtures*node_modules*some_module*obj' imported from *test*fixtures*esm_loader_not_found_cjs_hint_bare.mjs
Did you mean to import some_module/obj.js?
Expand Down