process: make exitCode configurable again

[ljharb]

This change was done in #44711, and it was unintentional. It caused #45683, and also makes it impossible to mock out the exitCode in tests. It affects all of node 19, and all of node 20 thus far (so it should not be backported to node 18 or earlier)

Filing this PR per #44711 (comment)

Refs: #44711
Fixes: #45683

cc @nodejs/process @nlf @daeyeon

[nodejs-github-bot]

Review requested:

• @nodejs/startup

[isaacs]

I like this change, solves my problem. (I worked around it by just letting the code in my tests update the "real" process.exitCode and then verifying it's been set, and setting it back to 0. But that's less good, because it means I can't also verify that it was set the number of times I expect, in the places I expect, etc.)

Another idea to consider, which would preserve the air-tight protection against non-number exitCode:

• Make process.exitCode a getter/setter, configurable: true
• The setter verifies that the argument is a number, and then sets the "real" exit code somewhere internal
• Downside: if you delete process.exitCode then setting process.exitCode = 1 just won't do anything.

[Trott]

Is it worthwhile to add a test for this?

[ljharb]

Happy to if you can suggest where to put it :-)

[Trott]

Happy to if you can suggest where to put it :-)

Maybe see if it's possible/reasonable to include it in test/parallel/test-process-exit-code-validation.js? That has some code that runs delete process.exitCode which seems like something we're interested in here.

If that doesn't seem like the right place, perhaps create a separate test file as test/parallel/test-proces-exitCode-configurable or test/parallel/test-process-exitCode-delete or something similar to that?

[ljharb]

What's interesting is that that test isn't failing now.

[benjamingr]

+1 (agree with @Trott this should have a test)

[daeyeon]

The configurable: false is intended to prevent the deletion of the setter, ensuring that the API remains intact. Allowing the API removed could result in a mismatch between the exit code set by users in JavaScript and the one returned by the native side.

process.exitCode = 1;
delete process.exitCode;
process.exitCode = 0; 

// With this patch, node will exit with 1, not 0.

Previously, the JavaScript side hold the exitCode, and the native side had to make several cross-boundary calls during exit to retrieve the value. And, exiting Node.js with a clear exit code was not always valid since not all objects allowed for the exitCode could be converted to a clear integer value.

node/lib/internal/bootstrap/node.js

Lines 123 to 124 in c55625f

	validateInteger(value, 'code');
	fields[kExitCode] = value;

Since v20, JavaScript side and the native side share the exitCode through fields[kExitCode], eliminating the need for cross-boundary calls from the native side. The setter now only accepts values that can be straightforwardly converted to an integer as the exit code.

@isaacs I'm not entirely sure if I grasp your issue correctly, but it seems like you want to monitor changes in the exit code during testing. I'm not certain if this is a good idea, but would it resolve your issue if we would trigger an event along with a modified exit code whenever it's changed?

[isaacs]

@daeyeon

I'm not entirely sure if I grasp your issue correctly, but it seems like you want to monitor changes in the exit code during testing. I'm not certain if this is a good idea, but would it resolve your issue if we would trigger an event along with a modified exit code whenever it's changed?

Yes, it's for testing. It's somewhat less convenient to not be able to just do t.intercept(process, 'exitCode') and verify that it's set the exact number of times in the exact places expected, and have it automatically revert at the end of the test. But it's not that big a deal, really. I would not recommend adding a new API just for monitoring exitCode changes.

The configurable: false is intended to prevent the deletion of the setter, ensuring that the API remains intact. Allowing the API removed could result in a mismatch between the exit code set by users in JavaScript and the one returned by the native side.

Personally, I think that's fine. Put a note in the docs that delete process.exitCode is no longer equivalent to process.exitCode = 0, and trust that no one is going to intentionally break their own programs. But I can see the argument either way. 🤷

[ljharb]

I'm happy to add a test, but the existing delete process.exitCode test should be failing, and isn't, so I'd like to debug that first. Any ideas?

[daeyeon]

the existing delete process.exitCode test should be failing, and isn't, so I'd like to debug that first. Any ideas?

It seems that process.exitCode has been deleted, causing breaking the core that uses process.exitCode.

process.exitCode = 0;

throws(() => {
  delete process.exitCode; // configurable: true
}, /Cannot delete property 'exitCode' of #<process>/);

In the test, we last set exitCode to 0 and then do delete process.exitCode. When an exception is thrown by assert.throws(), it is caught by the global uncaught exception handler in the code below.

node/lib/internal/process/execution.js

Lines 156 to 167 in 48fcb20

	process.emit('uncaughtExceptionMonitor', er, type);
	if (exceptionHandlerState.captureFn !== null) {
	exceptionHandlerState.captureFn(er);
	} else if (!process.emit('uncaughtException', er, type)) {
	// If someone handled it, then great. Otherwise, die in C++ land
	// since that means that we'll exit the process, emit the 'exit' event.
	try {
	if (!process._exiting) {
	process._exiting = true;
	process.exitCode = kGenericUserError;
	process.emit('exit', kGenericUserError);
	}

In 165 line, the handler set exitCode to kGenericUserError, but it's not passed to the native side. Because process.exitCode is undefined. As a result, although the exception causes node process to exit, the exitCode remains as 0 instead of kGenericUserError. The test.py does not catch the error.

[ljharb]

Is that a deeper bug that I should try to fix?

[daeyeon]

Yes, it's needed to handle.

[ljharb]

ok, i was able to verify that the test failed locally, and then i updated the test to pass with the change, so in theory this is good to go :-D

[nodejs-github-bot]

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

[daeyeon]

I think it needs to be handled so that even when users delete process.exitCode, the exit code set by the core code will still be applied.

process.exitCode = kGenericUserError; 

process.exitCode is also used by the core. This is concerning because if users delete it, the intended behavior in the core, like the code mentioned above (#49579 (comment)), won't be applied.

[ljharb]

I'm not sure how that would be possible. If it's deleted, there's nothing there.

[ljharb]

Do note that process.exitCode = kGenericUserError; will still work after it's been deleted, it just won't have the magic getter/setter anymore.

[daeyeon]

If process.exitCode = kGenericUserError; worked, the error code after terminating the node should be kGenericUserError, not 0. Last time I checked, it was 0... right?

[daeyeon]

I mean it should not be 0 when calling the exit status. echo $?

[ljharb]

it can be restricted to any domain you like :-) just integer numbers then?

[daeyeon]

I'm not sure I can give you the answer you want, as I can't guess how you'd implement it based on your current description. Currently, the types allowed are:

* {integer|string|null|undefined} The exit code. For string type, only
  integer strings (e.g.,'1') are allowed. **Default:** `undefined`.

[ljharb]

ok, so I’ll move forward with that schema, thanks.

[ljharb]

hmm - seems like the exitCode is read in C++, so i'm not sure how it can read the JS-set value once it's overridden.

@daeyeon are you comfortable with this proceeding with the understanding that if a user mocks process.exitCode, it's up to them to restore it? (just like other globals)

If not, what about #49579 (comment) ?

[daeyeon]

@daeyeon are you comfortable with this proceeding with the understanding that if a user mocks process.exitCode, it's up to them to restore it? (just like other globals)

The core's behavior related to exitCode will be breaking during the time gap between mocking and restoring. It's probably not enough to rely on the restoring after finishing the mocking.

If not, what about #49579 (comment) ?

Looks like a good idea. In addition to moving the getters/setters to the process prototype, I think that the core code should use the exitCode of the process prototype, rather than the process itself. This would address concerns about breaking and can provide guidance on how users can monitor it.

[isaacs]

What if the getter/setter was on the prototype of the process, rather than the process object itself?

Something like this:

class Process {
  #exitCode = 0
  get exitCode() {
    return this.#exitCode
  }
  set exitCode(ec) {
    if (ec === Math.floor(ec)) {
      this.#exitCode = ec
    } else {
      throw new Error('cannot set exit code to non-int')
    }
  }
}

// make the property on the prototype non-configurable
Object.defineProperty(Process.prototype, 'exitCode', {
  ...Object.getOwnPropertyDescriptor(Process.prototype, 'exitCode'),
  configurable: false,
})

const process = new Process()
Object.defineProperty(process, 'exitCode', {
  value: 2,
  configurable: true,
  writable: true,
})
console.log(process.exitCode) // 2, from defined property on object
delete process.exitCode // deletes from object, not prototype
console.log(process.exitCode) // 0

try {
  Object.defineProperty(process.__proto__, 'exitCode', { value: 99 })
} catch {
  console.log('cannot redefine') // this is printed
}

With this approach:

• You can spy read/write accesses to the property on the process object without any problems, even if it's not configurable
• When deleted, it falls back to the prototypical behavior, so delete process.exitCode is once again safe (though, it's a no-op in most cases, unless it's been overridden on purpose.)

That is, you get this weirdness, which imo is way less weird than what we have now:

process.exitCode = 1
delete process.exitCode
console.log(process.exitCode) // 1
// process exits with code 1, not zero as would've happened prior to the non-configurable propdef

[ljharb]

That's a clever alternative - but then the actual exit code still wouldn't read from the overridden value?

[isaacs]

That's a clever alternative - but then the actual exit code still wouldn't read from the overridden value?

Correct, if you Object.defineProperty(process, 'exitCode', { ... }), then anything you do to that ownProp definition would not affect the actual exit status code.

But isn't that sort of what happens in the code right now? Like, if you make the getter/setter configurable, then if someone redefines it, it no longer affects the actual process exit status code, until/unless they put it back. The difference is that "put it back" is delete process.exitCode vs stashing the propDef and redefining it.

[joyeecheung]

I think we should just test that redefining process.exitCode with accessors that call out to the original at the end still works as expected. That means users can still mock it if they call out to the original accessors. If they don't, or if they delete it, then it is just not going to get picked up as the actual exit code. And we can just put a note in the doc warning that delete process.exitCode is not going to work, like what @isaacs suggested in #49579 (comment)

[joyeecheung]

What if the getter/setter was on the prototype of the process, rather than the process object itself?

Maybe we can have both? By default we define it both on the prototype and the process object itself. The own version is configurable, and mocking code can define different accessors on the process itself, calling out to the original own accessors for Node.js to pick up the exit code if necessary. If they delete the own version then there's still the prototype version for compat.

[ljharb]

Sounds like a good direction. I'll update this soon.