process: inspect error in case of a fatal exception#27243

BridgeAR · 2019-04-15T18:15:27Z

This makes sure that errors that shut down the application are
inspected with util.inspect(). That makes sure that all extra
properties on the error will be visible and also that the stack trace
is highlighted (Node.js internal frames will be grey and node modules
are underlined).

That should overall improve the debugging experience for users.

This should be semver-minor, since this only applies in case of an
fatal exception and it always ends up for the actual application user.

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

test/message/throw_custom_error.out

BridgeAR · 2019-04-16T16:50:40Z

@nodejs/process @nodejs/util PTAL

BridgeAR · 2019-04-18T14:52:25Z

This is IMO quite a decent change and it would be great to get some reviews!

benjamingr · 2019-04-20T12:00:14Z

@BridgeAR the actual changes LGTM (and I'm for them in general) but I'm hesitant to approve because:

This is a semver-major change since it touches error messages
This will inevitably require changing a lot of code dealing with exit exceptions for that change

BridgeAR · 2019-04-21T18:12:44Z

@benjamingr

This is a semver-major change since it touches error messages

The error message stays identical to the one before. The message printed to stderr is just different. AFAIK we never had any guarantees about that and we can't really make such guarantees since the error stack could change with every single Node.js patch release (the line numbers in the stack could change). Thus, this does not seem to be semver-major to me?

This will inevitably require changing a lot of code dealing with exit exceptions for that change

I guess you mean in case someone checks error messages from a child_process? I doubt that this really requires any changes since it's not possible to match the whole stderr output due to the changing stack trace. Using a regular expression matching against err.message should always keep on working as before since the error message itself is not touched / manipulated.

Just running CITGM to be cautious:

This PR: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1828/
Master: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1829/

addaleax

LGTM but I’d also prefer to be careful and maybe land this as a semver-major change? I could definitely see this forcing people to change tests…

BridgeAR · 2019-04-22T03:27:43Z

@nodejs/tsc PTAL, especially about the semverness.

(CITGM is clean and this change should have little programmatic influence)

test/parallel/test-error-reporting.js

Fishrock123 · 2019-04-22T19:40:08Z

If the textual output is the same, I don't think this is a semver issue to produce color escape codes for environments that report that they accept colors.

If you are automatically interpreting the output, you're almost certainly piping it or writing it to a file in a way that won't report color escape code acceptance.

BridgeAR · 2019-04-22T21:10:36Z

@Fishrock123 the textual output is mostly identical. In case the error had extra properties (e.g., all Node.js core errors have the code property) that is going to be printed as well. That was not the case before. But these properties are appended and the start of the error message is identical. The test changes outline all differences.

BridgeAR · 2019-04-23T13:15:52Z

This fails in the custom builds while it should pass. It seems our custom suite has some trouble right now.

BridgeAR · 2019-04-25T02:59:13Z

Rebased due to issues with our CI not properly rebasing.

lib/internal/process/execution.js

test/message/throw_custom_error.out

BridgeAR · 2019-04-29T19:08:15Z

This could use another review.

targos · 2019-04-30T12:44:49Z

an fatal -> a fatal

This makes sure that errors that shut down the application are inspected with `util.inspect()`. That makes sure that all extra properties on the error will be visible and also that the stack trace is highlighted (Node.js internal frames will be grey and node modules are underlined).

nodejs-github-bot · 2019-05-03T11:17:15Z

CI: https://ci.nodejs.org/job/node-test-pull-request/22921/ ✅ (besides Windows)
Windows rebuild: https://ci.nodejs.org/job/node-test-commit-windows-fanned/27060/ (yellow build)
Windows rebuild: https://ci.nodejs.org/job/node-test-commit-windows-fanned/27085/ (yellow build - the flaky test is a http2 error that is independent from this change)

BridgeAR · 2019-05-07T23:46:52Z

@addaleax are you fine if I land this as is with the labels as they are?

BridgeAR · 2019-05-11T16:51:12Z

@nodejs/tsc I would like to land this soon as semver-minor. If no one brings up any concerns up to the 15th, I would keep the labels as they are and land this.

Trott

The actual change/feature seems all right to me. Not sure about semver-ness and will defer to others on that.

This makes sure that errors that shut down the application are inspected with `util.inspect()`. That makes sure that all extra properties on the error will be visible and also that the stack trace is highlighted (Node.js internal frames will be grey and node modules are underlined). PR-URL: nodejs#27243 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Rich Trott <[email protected]>

BridgeAR · 2019-05-16T10:50:58Z

Thanks a lot for the reviews!

Landed in a9f518c 🎉

This makes sure that errors that shut down the application are inspected with `util.inspect()`. That makes sure that all extra properties on the error will be visible and also that the stack trace is highlighted (Node.js internal frames will be grey and node modules are underlined). PR-URL: #27243 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Rich Trott <[email protected]>

Notable changes: * process: * Log errors using `util.inspect` in case of fatal exceptions (Ruben Bridgewater) #27243 * repl: * Add `process.on('uncaughtException')` support (Ruben Bridgewater) #27151 * stream: * Implemented `Readable.from` async iterator utility (Guy Bedford) #27660 * tls: * Expose built-in root certificates (Ben Noordhuis) #26415 * Support `net.Server` options (Luigi Pinca) #27665 * Expose `keylog` event on TLSSocket (Alba Mendez) #27654 * worker: * Added the ability to unshift messages from the `MessagePort` (Anna Henningsen) #27294

Notable changes: * esm: * Added the `--experimental-wasm-modules` flag to support WebAssembly modules (Myles Borins & Guy Bedford) #27659 * process: * Log errors using `util.inspect` in case of fatal exceptions (Ruben Bridgewater) #27243 * repl: * Add `process.on('uncaughtException')` support (Ruben Bridgewater) #27151 * stream: * Implemented `Readable.from` async iterator utility (Guy Bedford) #27660 * tls: * Expose built-in root certificates (Ben Noordhuis) #26415 * Support `net.Server` options (Luigi Pinca) #27665 * Expose `keylog` event on TLSSocket (Alba Mendez) #27654 * worker: * Added the ability to unshift messages from the `MessagePort` (Anna Henningsen) #27294 PR-URL: #27799

BridgeAR added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 15, 2019