From 6dd288ec5430177b7684f1b23268b949364e2c77 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 6 Aug 2020 02:56:17 +0200 Subject: [PATCH 1/3] 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 --- doc/api/process.md | 2 + lib/internal/modules/run_main.js | 24 +++++- lib/internal/process/execution.js | 15 ++-- 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 + .../esm_display_syntax_error_import.out | 1 - ...esm_display_syntax_error_import_module.out | 1 - .../esm_loader_not_found_cjs_hint_bare.out | 6 +- 11 files changed, 120 insertions(+), 17 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 8856c011f89a32..8ee89837c9c54e 100644 --- a/doc/api/process.md +++ b/doc/api/process.md @@ -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 + 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..1c41cf189da2e5 100644 --- a/lib/internal/modules/run_main.js +++ b/lib/internal/modules/run_main.js @@ -43,10 +43,29 @@ function runMainESM(mainPath) { esmLoader.loadESM((ESMLoader) => { 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. @@ -62,5 +81,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..61c29d89b266d3 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,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) { 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(() => {}); diff --git a/test/message/esm_display_syntax_error_import.out b/test/message/esm_display_syntax_error_import.out index fe174d54a5c49f..387a63a734b512 100644 --- a/test/message/esm_display_syntax_error_import.out +++ b/test/message/esm_display_syntax_error_import.out @@ -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:*:*) diff --git a/test/message/esm_display_syntax_error_import_module.out b/test/message/esm_display_syntax_error_import_module.out index d220627bd02654..ae8b99d55fef20 100644 --- a/test/message/esm_display_syntax_error_import_module.out +++ b/test/message/esm_display_syntax_error_import_module.out @@ -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:*:*) diff --git a/test/message/esm_loader_not_found_cjs_hint_bare.out b/test/message/esm_loader_not_found_cjs_hint_bare.out index 6063709859573b..10fd7a10dce681 100644 --- a/test/message/esm_loader_not_found_cjs_hint_bare.out +++ b/test/message/esm_loader_not_found_cjs_hint_bare.out @@ -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? From 520196069f99555c8748ef7e49df37f6f6c586cf Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 6 Aug 2020 04:30:49 +0200 Subject: [PATCH 2/3] fixup! module: handle Top-Level Await non-fulfills better --- lib/internal/modules/run_main.js | 6 +++--- lib/internal/process/execution.js | 7 +++---- test/message/esm_display_syntax_error_import.out | 1 + test/message/esm_display_syntax_error_import_module.out | 1 + test/message/esm_loader_not_found_cjs_hint_bare.out | 6 +++--- 5 files changed, 11 insertions(+), 10 deletions(-) diff --git a/lib/internal/modules/run_main.js b/lib/internal/modules/run_main.js index 1c41cf189da2e5..507d076d162b0b 100644 --- a/lib/internal/modules/run_main.js +++ b/lib/internal/modules/run_main.js @@ -40,11 +40,11 @@ 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; - handleMainPromise(ESMLoader.import(main)); - }); + return ESMLoader.import(main); + })); } function handleMainPromise(promise) { diff --git a/lib/internal/process/execution.js b/lib/internal/process/execution.js index 61c29d89b266d3..f8d9e043efda82 100644 --- a/lib/internal/process/execution.js +++ b/lib/internal/process/execution.js @@ -43,15 +43,14 @@ function evalModule(source, print) { throw new ERR_EVAL_ESM_CANNOT_PRINT(); } const { log } = require('internal/console/global'); - const asyncESM = require('internal/process/esm_loader'); + const { loadESM } = require('internal/process/esm_loader'); const { handleMainPromise } = require('internal/modules/run_main'); - handleMainPromise((async () => { - const loader = await asyncESM.ESMLoader; + handleMainPromise(loadESM(async (loader) => { const { result } = await loader.eval(source); if (print) { log(result); } - })()); + })); } function evalScript(name, body, breakFirstLine, print) { diff --git a/test/message/esm_display_syntax_error_import.out b/test/message/esm_display_syntax_error_import.out index 387a63a734b512..fe174d54a5c49f 100644 --- a/test/message/esm_display_syntax_error_import.out +++ b/test/message/esm_display_syntax_error_import.out @@ -5,3 +5,4 @@ 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:*:*) diff --git a/test/message/esm_display_syntax_error_import_module.out b/test/message/esm_display_syntax_error_import_module.out index ae8b99d55fef20..d220627bd02654 100644 --- a/test/message/esm_display_syntax_error_import_module.out +++ b/test/message/esm_display_syntax_error_import_module.out @@ -5,3 +5,4 @@ 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:*:*) diff --git a/test/message/esm_loader_not_found_cjs_hint_bare.out b/test/message/esm_loader_not_found_cjs_hint_bare.out index 10fd7a10dce681..6063709859573b 100644 --- a/test/message/esm_loader_not_found_cjs_hint_bare.out +++ b/test/message/esm_loader_not_found_cjs_hint_bare.out @@ -1,6 +1,6 @@ -internal/modules/run_main.js:* - internalBinding('errors').triggerUncaughtException( - ^ +internal/process/esm_loader.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? From 9b3aa518d030736c9b7528f24e8ce8cf86512647 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 6 Aug 2020 04:33:08 +0200 Subject: [PATCH 3/3] fixup! fixup! module: handle Top-Level Await non-fulfills better --- lib/internal/modules/run_main.js | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/lib/internal/modules/run_main.js b/lib/internal/modules/run_main.js index 507d076d162b0b..d7b0ee56a1e8a5 100644 --- a/lib/internal/modules/run_main.js +++ b/lib/internal/modules/run_main.js @@ -49,21 +49,14 @@ function runMainESM(mainPath) { 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. + // 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); - promise - .finally(() => process.off('exit', handler)) - .catch((err) => { - internalBinding('errors').triggerUncaughtException( - err, true /* fromPromise */); - }); + return promise.finally(() => process.off('exit', handler)); } // For backwards compatibility, we have to run a bunch of