process: add isExiting

[bengl]

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]

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

[bnoordhuis]

Typo: asynchronous

[bnoordhuis]

kIsExitingSymbol? kIsExiting suggests it's a boolean.

[apapirovski]

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.

[addaleax]

@apapirovski k is for konstants ;)

[bnoordhuis]

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

[jasnell]

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.

[bengl]

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

[jasnell]

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

[bengl]

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

[BridgeAR]

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

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]

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]

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

[addaleax]

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]

1. even -> event?
2. Maybe it is worth to add a type before the description for consistency with other properties:

* {boolean}

[cjihrig]

vX.X.X -> REPLACEME

[Trott]

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

[cjihrig]

"tells whether" -> "indicates if"?

[BridgeAR]

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]

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]

@BridgeAR Here's a use case: #17453

[jasnell]

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

[addaleax]

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

Or indefinitely. ;)

[addaleax]

ping @bengl again

[bengl]

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]

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

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

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

[BridgeAR]

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