process: add isExiting #16404

bengl · 2017-10-23T08:45:47Z

This replaces the previously undocumented _exiting property with a
read-only property giving the same information. Setting the property is
restricted to internals, to prevent tampering. Tampering could affect
nextTick behavior, for example.

Twitter thread for context: https://twitter.com/bengl/status/922361992379166721

It may or may not make sense to keep _exiting around as an alias, if there's userland code using it. That's easy enough to do if reviewers would like that.

/cc @addaleax @Trott

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

process

bengl · 2017-10-23T08:46:36Z

CI: https://ci.nodejs.org/job/node-test-pull-request/10918/

bnoordhuis · 2017-10-23T08:51:37Z

doc/api/process.md

+-->
+
+This read-only boolean tells whether the process is currently exiting. That is,
+when the `exit` even has been fired and no further scheduled asynchronus code


Typo: asynchronous

bnoordhuis · 2017-10-23T08:54:36Z

lib/internal/process.js

+const {
+  getHiddenValue,
+  setHiddenValue,
+  is_exiting_private_symbol: kIsExiting


kIsExitingSymbol? kIsExiting suggests it's a boolean.

Aren't we using kAnything to signify symbols where, I assume, k === key? At the very least, http2, tls & url already do it that way.

@apapirovski k is for konstants ;)

bnoordhuis · 2017-10-23T08:57:39Z

lib/internal/process/next_tick.js

@@ -246,7 +246,7 @@ function setupNextTick() {
    if (typeof callback !== 'function')
      throw new errors.TypeError('ERR_INVALID_CALLBACK');

-    if (process._exiting)
+    if (process.isExiting)


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

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.

@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++.

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

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

@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?

Fishrock123 · 2017-10-23T15:47:36Z

Why expose this publicly now? The only code that can run during exist is within 'exit' event handlers, right?

If anything we should make the property non-writable-configurable or just hide it entirely...

addaleax · 2017-10-23T15:51:02Z

Why expose this publicly now?

For one, it tells you whether there’s a chance async operations that you schedule will finish or not.

The only code that can run during exist is within 'exit' event handlers, right?

Yes, it should tell you exactly that – whether you’re inside an 'exit' handler or not.

jasnell · 2017-10-23T17:12:06Z

Defensively marking as semver-major. I've seen code in the wild that looks at process._exiting

addaleax · 2017-10-23T17:13:55Z

It may or may not make sense to keep _exiting around as an alias, if there's userland code using it. That's easy enough to do if reviewers would like that.

Yes, I’d think so. It doesn’t really cost anything to keep that around, right?

vsemozhetbyt · 2017-10-23T20:41:59Z

doc/api/process.md

+-->
+
+This read-only boolean tells whether the process is currently exiting. That is,
+when the `exit` even has been fired and no further scheduled asynchronus code


even -> event?

Maybe it is worth to add a type before the description for consistency with other properties:

* {boolean}

cjihrig · 2017-10-24T18:51:47Z

doc/api/process.md

@@ -1188,6 +1188,15 @@ 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: vX.X.X


vX.X.X -> REPLACEME

Aside: Maybe some of the cool new md linting can get a custom rule to validate these YAML bits? /cc @watilde

cjihrig · 2017-10-24T18:52:21Z

doc/api/process.md

+added: vX.X.X
+-->
+
+This read-only boolean tells whether the process is currently exiting. That is,


"tells whether" -> "indicates if"?

BridgeAR · 2017-11-23T11:14:08Z

I am wondering if we really need to expose this at all... I am not a huge fan of adding functionality that was never requested. Does someone have a use case for me right quick?
Without exposing it we could simply use a plain symbol and do not have to worry about the performance implications anymore as that would not have any.

addaleax · 2017-11-30T00:52:12Z

Hm - ping @bengl?

@BridgeAR I think this makes sense just from the perspective of API completeness alone. Without this, you can't tell whether asynchronous work that you're going to schedule will run or not.

Trott · 2017-12-04T13:59:44Z

@BridgeAR Here's a use case: #17453

jasnell · 2017-12-05T17:32:48Z

I'm + 1 on adding this and keeping the _exiting around as an alias for a while.

addaleax · 2017-12-05T18:13:58Z

and keeping the _exiting around as an alias for a while.

Or indefinitely. ;)

addaleax · 2017-12-10T02:59:37Z

ping @bengl again

This replaces the previously undocumented `_exiting` property with a read-only property giving the same information. Setting the property is restricted to internals, to prevent tampering. Tampering could affect nextTick behavior, for example.

bengl · 2017-12-10T23:14:26Z

I've updated this with the requested changes, however, as @bnoordhuis suggested, nextTick performance is greatly degraded by this change. I'm going to investigate using internal/process as the canonical location of the boolean.

addaleax · 2017-12-11T00:50:28Z

@bengl If necessary, there’s always the option of falling back to just renaming it, right? I would prefer that over trying to protect it from user access at all costs.

bengl · 2017-12-11T01:21:42Z

@addaleax Yeah, I'd definitely prefer to prevent user tampering, but the added overhead of having to pass an extra reference in so node.cc#EmitExit can set it might not be worth the effort it right now.

I'll do another PR in a bit, just adding the alias and documenting it.

BridgeAR · 2018-01-31T01:52:59Z

@bengl is this still work in progress? And it needs a rebase.

BridgeAR · 2018-02-07T03:02:41Z

Closing due to long inactivity. Please feel free to reopen if you would like to work on this again.

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Oct 23, 2017

bnoordhuis reviewed Oct 23, 2017

View reviewed changes

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 23, 2017

vsemozhetbyt reviewed Oct 23, 2017

View reviewed changes

cjihrig reviewed Oct 24, 2017

View reviewed changes

MylesBorins force-pushed the master branch from b7405ab to 7f086dd Compare December 8, 2017 16:37

process: add isExiting

1b7a9bf

This replaces the previously undocumented `_exiting` property with a read-only property giving the same information. Setting the property is restricted to internals, to prevent tampering. Tampering could affect nextTick behavior, for example.

bengl force-pushed the bengl/processisexiting branch from fc062cc to 1b7a9bf Compare December 10, 2017 23:11

maclover7 force-pushed the master branch from bb5575a to 993b716 Compare January 26, 2018 22:02

cjihrig force-pushed the master branch from 993b716 to 082f952 Compare January 26, 2018 22:36

BridgeAR closed this Feb 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

process: add isExiting #16404

process: add isExiting #16404

bengl commented Oct 23, 2017

bengl commented Oct 23, 2017 •

edited

Loading

bnoordhuis Oct 23, 2017

bnoordhuis Oct 23, 2017

apapirovski Oct 23, 2017

addaleax Oct 23, 2017

bnoordhuis Oct 23, 2017

jasnell Oct 23, 2017

bengl Oct 23, 2017

jasnell Oct 23, 2017

bengl Oct 23, 2017

BridgeAR Nov 23, 2017

Fishrock123 commented Oct 23, 2017

addaleax commented Oct 23, 2017

jasnell commented Oct 23, 2017

addaleax commented Oct 23, 2017

vsemozhetbyt Oct 23, 2017

cjihrig Oct 24, 2017

Trott Oct 25, 2017

cjihrig Oct 24, 2017

BridgeAR commented Nov 23, 2017

addaleax commented Nov 30, 2017

Trott commented Dec 4, 2017

jasnell commented Dec 5, 2017

addaleax commented Dec 5, 2017

addaleax commented Dec 10, 2017

bengl commented Dec 10, 2017

addaleax commented Dec 11, 2017

bengl commented Dec 11, 2017

BridgeAR commented Jan 31, 2018

BridgeAR commented Feb 7, 2018

process: add isExiting #16404

process: add isExiting #16404

Conversation

bengl commented Oct 23, 2017

Checklist

Affected core subsystem(s)

bengl commented Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 commented Oct 23, 2017

addaleax commented Oct 23, 2017

jasnell commented Oct 23, 2017

addaleax commented Oct 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Nov 23, 2017

addaleax commented Nov 30, 2017

Trott commented Dec 4, 2017

jasnell commented Dec 5, 2017

addaleax commented Dec 5, 2017

addaleax commented Dec 10, 2017

bengl commented Dec 10, 2017

addaleax commented Dec 11, 2017

bengl commented Dec 11, 2017

BridgeAR commented Jan 31, 2018

BridgeAR commented Feb 7, 2018

bengl commented Oct 23, 2017 •

edited

Loading