process: add exitWithException()

[Fishrock123]

Adds a public way to access node::FatalException from javascript, as process.exitWithException().

If an error stack is available on the value, this method of exiting does not add additional context, unlike regular re-throwing, yet also uses the regular error printing logic. This behavior is particularly desirable when doing 'fatal exits' from the unhandledRejection event.

... Been wanting this for some months now. 🙃

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

[nodejs-github-bot]

@Fishrock123 build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2393/pipeline

[addaleax]

@Fishrock123 Yeah, I assume the question is which error should take precedence. For this API it might be the most intuitive thing to let the error from the getter bubble back up to JS land, but … that might be hard for the general case, when we might not have a JS stack below the FatalException handler?

[Fishrock123]

So, uhhh, considering you can make node SIGABRT with a throw statement... I think we'll lave that for a separate issue.

node -e "throw { get stack() { throw new Error('weird throw but ok') } }"

Edit: fixed in #25834

[BridgeAR]

@Fishrock123 this needs a rebase

[Fishrock123]

Rebased and updated this to process.triggerFatalException() from process.throw(). PTAL.

[nodejs-github-bot]

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

[joyeecheung]

I know the name comes from existing internal methods, but TBH I think it's a bit confusing, since

1. There is process._fatalException which is called by this
2. There is v8::Isolate::SetFatalErrorHandler and our handler does something quite different - it prints the C++ stack trace, writes the node report if configured, and then ABORT() the process.

If there is a scale of how fatal something is, 2 feels more "fatal" to me, since at that point there is less stuff that you are allowed to do (e.g. this happens when we use the V8 API incorrectly and there is basically nothing the user can do to recover from that)

Something like exitWithException may be less confusing, and we should mention the exit code behavior - it's kind of tricky with this one, it could be either 6, 7, 1, or process.exitCode, depending on a bunch of conditions. (Also note that process._fatalException is monkey-patchable and there is no clear deprecation for it)

[joyeecheung]

Or, maybe we could make this an overload of process.exit(), as the part missing from the current process.exit() is just a call of process._fatalException and a call to ReportException. It also reads more naturally in the unhandledRejection handler as you do not appear to retrigger an exception (which is what it does, the error will not be triggered in the VM again).

[nodejs-github-bot]

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

[Fishrock123]

Do people think this sort of functionality should attempt to bypass _fatalException altogether?

[BridgeAR]

LGTM with the comments addressed.

[BridgeAR]

@Fishrock123 I would keep it. That way it's consistent and has the same output as other errors.

[Fishrock123]

@joyeecheung LGTY?

[joyeecheung]

What happens if this is uncaughtException? I think that results in a dead loop? I think we should at least document about this footgun, but maybe a better solution is to just implement this separately and exclude part of process._fatalException() - or otherwise this does not exit unconditionally any more.

[Fishrock123]

Hmm, throw seems to be short-cut somehow, but this blows the stack size. :/

[joyeecheung]

Are we even supposed to emit 'uncaughtException' when this is called? If so is there any point of using this instead of...well, just throwing an uncaught exception? I can see it makes sense to add this method if it does not trigger 'uncaughtException', but not really if it does..

[fhinkel]

ping @Fishrock123

[Fishrock123]

The problem with this is that internals need to be suitable restructured so as to bypass emitting the 'uncaughtException' event... If it emits 'uncaughtException' you end up with terrible circular execution issues and it's just a mess.

Which probably also means that it need to be renamed, again...

[Fishrock123]

It would probably be easier to just expose the exception logger and then call process.exit(1), maybe.

[jasnell]

It is unfortunate that this PR stalled. To move forward, it would need to be rebased and updated and have the uncaughtException recursion figured out. @Fishrock123, I'm going to close the PR but we can reopen if you wish to pick this back up again.