From e948ef351b4111d78ae8ba08f29eb138af22fa0b Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 6 Aug 2020 02:56:17 +0200 Subject: [PATCH] module: handle Top-Level Await non-fulfills better 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: https://github.com/nodejs/node/pull/34558 PR-URL: https://github.com/nodejs/node/pull/34640 Reviewed-By: James M Snell Reviewed-By: Guy Bedford Reviewed-By: Myles Borins Reviewed-By: Yongsheng Zhang --- doc/api/process.md | 2 + lib/internal/modules/run_main.js | 19 ++++- lib/internal/process/execution.js | 16 ++-- test/es-module/test-esm-tla-unfinished.mjs | 82 +++++++++++++++++++ .../es-modules/tla/rejected-withexitcode.mjs | 2 + test/fixtures/es-modules/tla/rejected.mjs | 1 + .../tla/unresolved-withexitcode.mjs | 2 + test/fixtures/es-modules/tla/unresolved.mjs | 1 + 8 files changed, 111 insertions(+), 14 deletions(-) create mode 100644 test/es-module/test-esm-tla-unfinished.mjs create mode 100644 test/fixtures/es-modules/tla/rejected-withexitcode.mjs create mode 100644 test/fixtures/es-modules/tla/rejected.mjs create mode 100644 test/fixtures/es-modules/tla/unresolved-withexitcode.mjs create mode 100644 test/fixtures/es-modules/tla/unresolved.mjs diff --git a/doc/api/process.md b/doc/api/process.md index 1f58ddd998cd65..7584c886adadac 100644 --- a/doc/api/process.md +++ b/doc/api/process.md @@ -2586,6 +2586,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 diff --git a/lib/internal/modules/run_main.js b/lib/internal/modules/run_main.js index 0967ef539ca20e..d7b0ee56a1e8a5 100644 --- a/lib/internal/modules/run_main.js +++ b/lib/internal/modules/run_main.js @@ -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 @@ -62,5 +74,6 @@ function executeUserEntryPoint(main = process.argv[1]) { } module.exports = { - executeUserEntryPoint + executeUserEntryPoint, + handleMainPromise, }; diff --git a/lib/internal/process/execution.js b/lib/internal/process/execution.js index 08d6ec8c6ea906..f8d9e043efda82 100644 --- a/lib/internal/process/execution.js +++ b/lib/internal/process/execution.js @@ -2,7 +2,6 @@ const { JSONStringify, - PromiseResolve, } = primordials; const path = require('path'); @@ -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) { diff --git a/test/es-module/test-esm-tla-unfinished.mjs b/test/es-module/test-esm-tla-unfinished.mjs new file mode 100644 index 00000000000000..7d35c86285ac81 --- /dev/null +++ b/test/es-module/test-esm-tla-unfinished.mjs @@ -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/); +} diff --git a/test/fixtures/es-modules/tla/rejected-withexitcode.mjs b/test/fixtures/es-modules/tla/rejected-withexitcode.mjs new file mode 100644 index 00000000000000..34e98e0147f134 --- /dev/null +++ b/test/fixtures/es-modules/tla/rejected-withexitcode.mjs @@ -0,0 +1,2 @@ +process.exitCode = 42; +await Promise.reject(new Error('Xyz')); diff --git a/test/fixtures/es-modules/tla/rejected.mjs b/test/fixtures/es-modules/tla/rejected.mjs new file mode 100644 index 00000000000000..752a3b91ff6534 --- /dev/null +++ b/test/fixtures/es-modules/tla/rejected.mjs @@ -0,0 +1 @@ +await Promise.reject(new Error('Xyz')); diff --git a/test/fixtures/es-modules/tla/unresolved-withexitcode.mjs b/test/fixtures/es-modules/tla/unresolved-withexitcode.mjs new file mode 100644 index 00000000000000..1cb982311080b8 --- /dev/null +++ b/test/fixtures/es-modules/tla/unresolved-withexitcode.mjs @@ -0,0 +1,2 @@ +process.exitCode = 42; +await new Promise(() => {}); diff --git a/test/fixtures/es-modules/tla/unresolved.mjs b/test/fixtures/es-modules/tla/unresolved.mjs new file mode 100644 index 00000000000000..231a8cd634825c --- /dev/null +++ b/test/fixtures/es-modules/tla/unresolved.mjs @@ -0,0 +1 @@ +await new Promise(() => {});