From a90defebcf2e849db4514a55c9b718642944232a Mon Sep 17 00:00:00 2001 From: Gang Chen <13298548+MoonBall@users.noreply.github.com> Date: Fri, 14 Jan 2022 08:37:41 +0800 Subject: [PATCH] esm: make `process.exit()` default to exit code 0 Due to a bug in top-level await implementation, it used to default to exit code 13. PR-URL: https://github.com/nodejs/node/pull/41388 Backport-PR-URL: https://github.com/nodejs/node/pull/41508 Fixes: https://github.com/nodejs/node/issues/40808 Reviewed-By: Antoine du Hamel Reviewed-By: Guy Bedford --- .../modules/esm/handle_process_exit.js | 12 ++++++++++++ lib/internal/modules/run_main.js | 19 +++++++++---------- lib/internal/process/per_thread.js | 6 ++++++ test/es-module/test-esm-tla-unfinished.mjs | 18 ++++++++++++++++++ test/fixtures/es-modules/tla/process-exit.mjs | 1 + .../unresolved-with-worker-process-exit.mjs | 8 ++++++++ .../esm_display_syntax_error_import.out | 1 + ...esm_display_syntax_error_import_module.out | 1 + test/parallel/test-bootstrap-modules.js | 1 + 9 files changed, 57 insertions(+), 10 deletions(-) create mode 100644 lib/internal/modules/esm/handle_process_exit.js create mode 100644 test/fixtures/es-modules/tla/process-exit.mjs create mode 100644 test/fixtures/es-modules/tla/unresolved-with-worker-process-exit.mjs diff --git a/lib/internal/modules/esm/handle_process_exit.js b/lib/internal/modules/esm/handle_process_exit.js new file mode 100644 index 00000000000000..1d728974d8d091 --- /dev/null +++ b/lib/internal/modules/esm/handle_process_exit.js @@ -0,0 +1,12 @@ +'use strict'; + +// 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 handleProcessExit() { + if (process.exitCode == null) process.exitCode = 13; +} + +module.exports = { + handleProcessExit, +}; diff --git a/lib/internal/modules/run_main.js b/lib/internal/modules/run_main.js index bc6f90ce7d089a..b58e8d1fb205da 100644 --- a/lib/internal/modules/run_main.js +++ b/lib/internal/modules/run_main.js @@ -1,13 +1,15 @@ 'use strict'; const { - PromisePrototypeFinally, StringPrototypeEndsWith, } = primordials; const CJSLoader = require('internal/modules/cjs/loader'); const { Module, toRealPath, readPackageScope } = CJSLoader; const { getOptionValue } = require('internal/options'); const path = require('path'); +const { + handleProcessExit, +} = require('internal/modules/esm/handle_process_exit'); function resolveMainPath(main) { // Note extension resolution for the main entry point can be deprecated in a @@ -51,16 +53,13 @@ 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 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; +async function handleMainPromise(promise) { + process.on('exit', handleProcessExit); + try { + return await promise; + } finally { + process.off('exit', handleProcessExit); } - process.on('exit', handler); - return PromisePrototypeFinally(promise, () => process.off('exit', handler)); } // For backwards compatibility, we have to run a bunch of diff --git a/lib/internal/process/per_thread.js b/lib/internal/process/per_thread.js index 44a724073c4511..2043e5e0f0e078 100644 --- a/lib/internal/process/per_thread.js +++ b/lib/internal/process/per_thread.js @@ -36,6 +36,10 @@ const { const format = require('internal/util/inspect').format; const constants = internalBinding('constants').os.signals; +const { + handleProcessExit, +} = require('internal/modules/esm/handle_process_exit'); + function assert(x, msg) { if (!x) throw new ERR_ASSERTION(msg || 'assertion error'); } @@ -165,6 +169,8 @@ function wrapProcessMethods(binding) { memoryUsage.rss = rss; function exit(code) { + process.off('exit', handleProcessExit); + if (code || code === 0) process.exitCode = code; diff --git a/test/es-module/test-esm-tla-unfinished.mjs b/test/es-module/test-esm-tla-unfinished.mjs index 7d35c86285ac81..d7658c19e98e1c 100644 --- a/test/es-module/test-esm-tla-unfinished.mjs +++ b/test/es-module/test-esm-tla-unfinished.mjs @@ -80,3 +80,21 @@ import fixtures from '../common/fixtures.js'; assert.deepStrictEqual([status, stdout], [1, '']); assert.match(stderr, /Error: Xyz/); } + +{ + // Calling process.exit() in .mjs should return status 0 + const { status, stdout, stderr } = child_process.spawnSync( + process.execPath, + [fixtures.path('es-modules/tla/process-exit.mjs')], + { encoding: 'utf8' }); + assert.deepStrictEqual([status, stdout, stderr], [0, '', '']); +} + +{ + // Calling process.exit() in worker thread shouldn't influence main thread + const { status, stdout, stderr } = child_process.spawnSync( + process.execPath, + [fixtures.path('es-modules/tla/unresolved-with-worker-process-exit.mjs')], + { encoding: 'utf8' }); + assert.deepStrictEqual([status, stdout, stderr], [13, '', '']); +} diff --git a/test/fixtures/es-modules/tla/process-exit.mjs b/test/fixtures/es-modules/tla/process-exit.mjs new file mode 100644 index 00000000000000..78e86b018e7fbb --- /dev/null +++ b/test/fixtures/es-modules/tla/process-exit.mjs @@ -0,0 +1 @@ +process.exit(); diff --git a/test/fixtures/es-modules/tla/unresolved-with-worker-process-exit.mjs b/test/fixtures/es-modules/tla/unresolved-with-worker-process-exit.mjs new file mode 100644 index 00000000000000..8d65baa2b59cce --- /dev/null +++ b/test/fixtures/es-modules/tla/unresolved-with-worker-process-exit.mjs @@ -0,0 +1,8 @@ +import { Worker, isMainThread } from 'worker_threads'; + +if (isMainThread) { + new Worker(new URL(import.meta.url)); + await new Promise(() => {}); +} else { + process.exit(); +} diff --git a/test/message/esm_display_syntax_error_import.out b/test/message/esm_display_syntax_error_import.out index fe174d54a5c49f..cdb9b030705974 100644 --- a/test/message/esm_display_syntax_error_import.out +++ b/test/message/esm_display_syntax_error_import.out @@ -6,3 +6,4 @@ SyntaxError: The requested module '../fixtures/es-module-loaders/module-named-ex 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:*:*) + at async handleMainPromise (internal/modules/run_main.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..abb9d2ba626c20 100644 --- a/test/message/esm_display_syntax_error_import_module.out +++ b/test/message/esm_display_syntax_error_import_module.out @@ -6,3 +6,4 @@ SyntaxError: The requested module './module-named-exports.mjs' does not provide 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:*:*) + at async handleMainPromise (internal/modules/run_main.js:*:*) diff --git a/test/parallel/test-bootstrap-modules.js b/test/parallel/test-bootstrap-modules.js index 683171fe3c7b20..7062375c7c6241 100644 --- a/test/parallel/test-bootstrap-modules.js +++ b/test/parallel/test-bootstrap-modules.js @@ -67,6 +67,7 @@ const expectedModules = new Set([ 'NativeModule internal/modules/esm/resolve', 'NativeModule internal/modules/esm/transform_source', 'NativeModule internal/modules/esm/translators', + 'NativeModule internal/modules/esm/handle_process_exit', 'NativeModule internal/process/esm_loader', 'NativeModule internal/options', 'NativeModule internal/priority_queue',