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.