GeezerVison

Review requested:

• @nodejs/startup

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.

Is it worthwhile to add a test for this?

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

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?

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

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.

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?

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

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?

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.

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.

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

Yes, it's needed to handle.

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

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

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

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

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.

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.

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

This pull request has been marked as stale due to 90 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.