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

process: runtime deprecate coercion to integer in process.exit() #44711

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 4 additions & 1 deletion doc/api/deprecations.md
Expand Up @@ -3175,6 +3175,9 @@ thing instead.

<!-- YAML
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/44711
description: Runtime deprecation.
- version: v18.10.0
pr-url: https://github.com/nodejs/node/pull/44714
description: Documentation-only deprecation of `process.exitCode` integer
Expand All @@ -3187,7 +3190,7 @@ changes:
coercion.
-->

Type: Documentation-only
Type: Runtime

Values other than `undefined`, `null`, integer numbers, and integer strings
(e.g., `'1'`) are deprecated as value for the `code` parameter in
Expand Down
25 changes: 25 additions & 0 deletions lib/internal/bootstrap/node.js
Expand Up @@ -102,6 +102,31 @@ process.domain = null;
}
process._exiting = false;

{
const warnIntegerCoercionDeprecation = deprecate(
() => {},
'Implicit coercion to integer for exit code is deprecated.',
'DEP0164'
);

let exitCode;

ObjectDefineProperty(process, 'exitCode', {
__proto__: null,
get() {
return exitCode;
},
set(code) {
if (perThreadSetup.isDeprecatedExitCode(code)) {
warnIntegerCoercionDeprecation();
}
exitCode = code;
},
enumerable: true,
configurable: false,
Copy link
Member

Choose a reason for hiding this comment

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

i believe this line caused #45683. was this intentional? delete process.exitCode has always worked in the past.

Copy link
Contributor

Choose a reason for hiding this comment

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

This also makes it impossible to spy on process.exitCode in tests, making it more difficult to test libraries that set exitCode.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure it was intentional, I would suggest opening a PR turning it to configurable: true and see if anyone objects.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks; filed #49579

});
}

// process.config is serialized config.gypi
const nativeModule = internalBinding('builtins');

Expand Down
35 changes: 35 additions & 0 deletions lib/internal/process/per_thread.js
Expand Up @@ -13,7 +13,10 @@ const {
ArrayPrototypeSplice,
BigUint64Array,
Float64Array,
Number,
NumberIsInteger,
NumberMAX_SAFE_INTEGER,
NumberMIN_SAFE_INTEGER,
ObjectFreeze,
ObjectDefineProperty,
ReflectApply,
Expand Down Expand Up @@ -179,9 +182,23 @@ function wrapProcessMethods(binding) {

memoryUsage.rss = rss;

const { deprecate } = require('internal/util');
const warnIntegerCoercionDeprecationSync = deprecate(
() => {},
'Implicit coercion to integer for exit code is deprecated.',
'DEP0164',
true
);

function exit(code) {
process.off('exit', handleProcessExit);

if (isDeprecatedExitCode(code)) {
// Emit the deprecation warning synchronously since deprecation warning is
// generally emitted in a next tick but we have no next tick timing here.
warnIntegerCoercionDeprecationSync();
}

if (code || code === 0)
process.exitCode = code;

Expand Down Expand Up @@ -406,6 +423,23 @@ function toggleTraceCategoryState(asyncHooksEnabled) {
}
}

function isDeprecatedExitCode(code) {
if (code !== null && code !== undefined) {
const value =
typeof code === 'string' && code !== '' ? Number(code) : code;
// Check if the value is an integer.
if (
typeof value !== 'number' ||
!NumberIsInteger(value) ||
value < NumberMIN_SAFE_INTEGER ||
value > NumberMAX_SAFE_INTEGER
) {
return true;
}
}
return false;
}

module.exports = {
toggleTraceCategoryState,
assert,
Expand All @@ -414,4 +448,5 @@ module.exports = {
hrtime,
hrtimeBigInt,
refreshHrtimeBuffer,
isDeprecatedExitCode,
};
4 changes: 2 additions & 2 deletions lib/internal/process/warning.js
Expand Up @@ -166,8 +166,8 @@ function emitWarning(warning, type, code, ctor) {
process.nextTick(doEmitWarning, warning);
}

function emitWarningSync(warning) {
process.emit('warning', createWarningObject(warning));
function emitWarningSync(warning, type, code, ctor) {
process.emit('warning', createWarningObject(warning, type, code, ctor));
}

function createWarningObject(warning, type, code, ctor, detail) {
Expand Down
7 changes: 5 additions & 2 deletions lib/internal/util.js
Expand Up @@ -96,7 +96,7 @@ let validateString;
// Mark that a method should not be used.
// Returns a modified function which warns once by default.
// If --no-deprecation is set, then it is a no-op.
function deprecate(fn, msg, code) {
function deprecate(fn, msg, code, useEmitSync) {
if (process.noDeprecation === true) {
return fn;
}
Expand All @@ -114,7 +114,10 @@ function deprecate(fn, msg, code) {
warned = true;
if (code !== undefined) {
if (!codesWarned.has(code)) {
process.emitWarning(msg, 'DeprecationWarning', code, deprecated);
const emitWarning = useEmitSync ?
require('internal/process/warning').emitWarningSync :
process.emitWarning;
emitWarning(msg, 'DeprecationWarning', code, deprecated);
codesWarned.add(code);
}
} else {
Expand Down
100 changes: 100 additions & 0 deletions test/parallel/test-process-exit-code-deprecation.js
@@ -0,0 +1,100 @@
'use strict';

require('../common');

const deprecated = [
{
code: '',
expected: 0,
},
{
code: '1 one',
expected: 0,
},
{
code: 'two',
expected: 0,
},
{
code: {},
expected: 0,
},
{
code: [],
expected: 0,
},
{
code: true,
expected: 1,
},
{
code: false,
expected: 0,
},
{
code: 2n,
expected: 0,
expected_useProcessExitCode: 1,
},
{
code: 2.1,
expected: 2,
},
{
code: Infinity,
expected: 0,
},
{
code: NaN,
expected: 0,
},
];
const args = deprecated;

if (process.argv[2] === undefined) {
const { spawnSync } = require('node:child_process');
const { inspect, debuglog } = require('node:util');
const { strictEqual } = require('node:assert');

const debug = debuglog('test');
const node = process.execPath;
const test = (index, useProcessExitCode) => {
const { status: code, stderr } = spawnSync(node, [
__filename,
index,
useProcessExitCode,
]);
debug(`actual: ${code}, ${inspect(args[index])} ${!!useProcessExitCode}`);
debug(`${stderr}`);

const expected =
useProcessExitCode && args[index].expected_useProcessExitCode ?
args[index].expected_useProcessExitCode :
args[index].expected;

strictEqual(code, expected, `actual: ${code}, ${inspect(args[index])}`);
strictEqual(
['[DEP0164]'].some((pattern) => stderr.includes(pattern)),
true
);
};

for (const index of args.keys()) {
// Check `process.exit([code])`
test(index);
// Check exit with `process.exitCode`
test(index, true);
}
} else {
const index = parseInt(process.argv[2]);
const useProcessExitCode = process.argv[3] !== 'undefined';
if (Number.isNaN(index)) {
return process.exit(100);
}

if (useProcessExitCode) {
process.exitCode = args[index].code;
} else {
process.exit(args[index].code);
}
}