Skip to content

Commit

Permalink
module: handle Top-Level Await non-fulfills better
Browse files Browse the repository at this point in the history
Handle situations in which the main `Promise` from a TLA module
is not fulfilled better:

- When not resolving the `Promise` at all, set a non-zero exit code
  (unless another one has been requested explicitly) to distinguish
  the result from a successful completion.
- When rejecting the `Promise`, always treat it like an uncaught
  exception. In particular, this also ensures a non-zero exit code.

Refs: #34558

PR-URL: #34640
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
  • Loading branch information
addaleax authored and codebytere committed Aug 11, 2020
1 parent d0a599a commit c21e62f
Show file tree
Hide file tree
Showing 8 changed files with 111 additions and 14 deletions.
2 changes: 2 additions & 0 deletions doc/api/process.md
Original file line number Diff line number Diff line change
Expand Up @@ -2566,6 +2566,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
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
19 changes: 16 additions & 3 deletions lib/internal/modules/run_main.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,23 @@ function shouldUseESMLoader(mainPath) {
function runMainESM(mainPath) {
const esmLoader = require('internal/process/esm_loader');
const { pathToFileURL } = require('internal/url');
esmLoader.loadESM((ESMLoader) => {
handleMainPromise(esmLoader.loadESM((ESMLoader) => {
const main = path.isAbsolute(mainPath) ?
pathToFileURL(mainPath).href : mainPath;
return 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 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);
return promise.finally(() => process.off('exit', handler));
}

// For backwards compatibility, we have to run a bunch of
Expand All @@ -62,5 +74,6 @@ function executeUserEntryPoint(main = process.argv[1]) {
}

module.exports = {
executeUserEntryPoint
executeUserEntryPoint,
handleMainPromise,
};
16 changes: 5 additions & 11 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,15 @@ 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 asyncESM = require('internal/process/esm_loader');
PromiseResolve(asyncESM.ESMLoader).then(async (loader) => {
const { log } = require('internal/console/global');
const { loadESM } = require('internal/process/esm_loader');
const { handleMainPromise } = require('internal/modules/run_main');
handleMainPromise(loadESM(async (loader) => {
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(() => {});

0 comments on commit c21e62f

Please sign in to comment.