Skip to content

process: add isExiting #16404

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

process: add isExiting #16404

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions doc/api/process.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1208,6 +1208,17 @@ console.log(process.getgroups()); // [ 27, 30, 46, 1000 ]
*Note*: This function is only available on POSIX platforms (i.e. not Windows
or Android).

## process.isExiting
<!-- YAML
added: REPLACEME
-->

* {boolean}

This read-only boolean indicates if the process is currently exiting. That is,
when the `exit` event has been fired and no further scheduled asynchronous code
will run.

## process.kill(pid[, signal])
<!-- YAML
added: v0.0.6
Expand Down
9 changes: 5 additions & 4 deletions lib/internal/bootstrap_node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
(function(process) {
let internalBinding;
const exceptionHandlerState = { captureFn: null };
let _process;

function startup() {
const EventEmitter = NativeModule.require('events');
Expand All @@ -33,7 +34,8 @@

setupGlobalVariables();

const _process = NativeModule.require('internal/process');
_process = NativeModule.require('internal/process');
_process.setIsExiting(false);
_process.setupConfig(NativeModule._source);
_process.setupSignalHandlers();
_process.setupUncaughtExceptionCapture(exceptionHandlerState);
Expand Down Expand Up @@ -302,7 +304,6 @@

global.Buffer = NativeModule.require('buffer').Buffer;
process.domain = null;
process._exiting = false;
}

function setupGlobalTimeouts() {
Expand Down Expand Up @@ -391,8 +392,8 @@
// since that means that we'll exit the process, emit the 'exit' event
if (!caught) {
try {
if (!process._exiting) {
process._exiting = true;
if (!process.isExiting) {
_process.setIsExiting(true);
process.emit('exit', 1);
}
} catch (er) {
Expand Down
34 changes: 30 additions & 4 deletions lib/internal/process.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,11 @@
const errors = require('internal/errors');
const util = require('util');
const constants = process.binding('constants').os.signals;
const {
getHiddenValue,
setHiddenValue,
is_exiting_private_symbol: kIsExitingSymbol
} = process.binding('util');

const assert = process.assert = function(x, msg) {
if (!x) throw new errors.Error('ERR_ASSERTION', msg || 'assertion error');
Expand Down Expand Up @@ -138,12 +143,24 @@ function setupConfig(_source) {

function setupKillAndExit() {

Object.defineProperty(process, 'isExiting', {
enumerable: true,
configurable: false,
get: getIsExiting
});

Object.defineProperty(process, '_exiting', {
enumerable: true,
configurable: false,
get: getIsExiting
});

process.exit = function(code) {
if (code || code === 0)
process.exitCode = code;

if (!process._exiting) {
process._exiting = true;
if (!process.isExiting) {
setIsExiting(true);
process.emit('exit', process.exitCode || 0);
}
process.reallyExit(process.exitCode || 0);
Expand Down Expand Up @@ -247,7 +264,6 @@ function setupRawDebug() {
};
}


function setupUncaughtExceptionCapture(exceptionHandlerState) {
// This is a typed array for faster communication with JS.
const shouldAbortOnUncaughtToggle = process._shouldAbortOnUncaughtToggle;
Expand Down Expand Up @@ -275,6 +291,14 @@ function setupUncaughtExceptionCapture(exceptionHandlerState) {
};
}

function setIsExiting(isExiting) {
setHiddenValue(process, kIsExitingSymbol, isExiting);
}

function getIsExiting() {
return getHiddenValue(process, kIsExitingSymbol);
}

module.exports = {
setup_performance,
setup_cpuUsage,
Expand All @@ -285,5 +309,7 @@ module.exports = {
setupSignalHandlers,
setupChannel,
setupRawDebug,
setupUncaughtExceptionCapture
setupUncaughtExceptionCapture,
setIsExiting,
getIsExiting
};
4 changes: 2 additions & 2 deletions lib/internal/process/next_tick.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ function setupNextTick() {
if (typeof callback !== 'function')
throw new errors.TypeError('ERR_INVALID_CALLBACK');

if (process._exiting)
if (process.isExiting)
Copy link
Member

Choose a reason for hiding this comment

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

I would expect this to introduce a performance regression (getter that calls into the C++ runtime.) Have you run next-tick benchmarks?

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps rather than a getter here, a public read-only property should be added only when we are exiting... that is.. setIsExiting() could do something like...

Object.defineProperty(process, 'exiting', { configurable: false, enumerable: true, value: true });

This should largely avoid the perf regression on calling the getter multiple times.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jasnell If it weren't there prior to exiting, then it could be set by userland code long before exiting, causing the problems I was trying to solve.

Maybe a getter to something in internal/process? That way it still would be userland tamper-proof, but wouldn't dive into C++.

Copy link
Member

Choose a reason for hiding this comment

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

That would certainly be better but the getter still comes at a cost.... hmm will have to think about it a bit more.

Copy link
Member Author

Choose a reason for hiding this comment

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

nextTick could just use the actual value on internal/process, rather than the getter (which would be only for public use).

Copy link
Member

Choose a reason for hiding this comment

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

@bengl if I understand you correct you want to use a plain symbol in internal/process plus the isExiting property as getter that exposes that symbol?

return;

var args;
Expand Down Expand Up @@ -221,7 +221,7 @@ function setupNextTick() {
// CHECK(Number.isSafeInteger(triggerAsyncId) || triggerAsyncId === null)
// CHECK(triggerAsyncId > 0 || triggerAsyncId === null)

if (process._exiting)
if (process.isExiting)
return;

var args;
Expand Down
2 changes: 1 addition & 1 deletion src/env.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ class ModuleWrap;
V(contextify_context_private_symbol, "node:contextify:context") \
V(contextify_global_private_symbol, "node:contextify:global") \
V(decorated_private_symbol, "node:decorated") \
V(is_exiting_private_symbol, "node:isExiting") \
V(npn_buffer_private_symbol, "node:npnBuffer") \
V(selected_npn_buffer_private_symbol, "node:selectedNpnBuffer") \
V(domain_private_symbol, "node:domain") \
Expand Down Expand Up @@ -145,7 +146,6 @@ class ModuleWrap;
V(errno_string, "errno") \
V(error_string, "error") \
V(events_string, "_events") \
V(exiting_string, "_exiting") \
V(exit_code_string, "exitCode") \
V(exit_string, "exit") \
V(expire_string, "expire") \
Expand Down
4 changes: 3 additions & 1 deletion src/node.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4294,7 +4294,9 @@ int EmitExit(Environment* env) {
HandleScope handle_scope(env->isolate());
Context::Scope context_scope(env->context());
Local<Object> process_object = env->process_object();
process_object->Set(env->exiting_string(), True(env->isolate()));
process_object->SetPrivate(env->context(),
env->is_exiting_private_symbol(),
True(env->isolate()));

Local<String> exitCode = env->exit_code_string();
int code = process_object->Get(exitCode)->Int32Value();
Expand Down
8 changes: 7 additions & 1 deletion test/parallel/test-next-tick-when-exiting.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,14 @@
require('../common');
const assert = require('assert');

const isExitingDesc = Object.getOwnPropertyDescriptor(process, 'isExiting');
assert.ok(!('set' in isExitingDesc));
assert.ok('get' in isExitingDesc);
assert.strictEqual(isExitingDesc.configurable, false);
assert.strictEqual(isExitingDesc.enumerable, true);

process.on('exit', () => {
assert.strictEqual(process._exiting, true, 'process._exiting was not set!');
assert.strictEqual(process.isExiting, true, 'process.isExiting was not set!');

process.nextTick(() => {
assert.fail('process is exiting, should not be called.');
Expand Down