Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Apr 15, 2019 •
edited
Loading

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.

image

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

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

This comment has been minimized.

Sign in to view
@nodejs-github-bot nodejs-github-bot added the process Issues and PRs related to the process subsystem. label Apr 15, 2019
@BridgeAR BridgeAR force-pushed the improve-debugging branch 2 times, most recently from 0e4eb06 to 82c44ca Compare April 15, 2019 19:47
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@BridgeAR
Copy link
Member Author

@nodejs/process @nodejs/util PTAL

@BridgeAR
Copy link
Member Author

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

@benjamingr
Copy link
Member

@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
Copy link
Member Author

@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
addaleax approved these changes Apr 21, 2019
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

@nodejs/tsc PTAL, especially about the semverness.

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

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@Fishrock123
Copy link
Contributor

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
Copy link
Member Author

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

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 23, 2019
@BridgeAR
Copy link
Member Author

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

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@BridgeAR
Copy link
Member Author

Rebased due to issues with our CI not properly rebasing.

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@BridgeAR BridgeAR mentioned this pull request Apr 25, 2019
Closed
2 tasks
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@BridgeAR BridgeAR added the review wanted PRs that need reviews. label Apr 29, 2019
@BridgeAR
Copy link
Member Author

This could use another review.

@targos
Copy link
Member

targos commented Apr 30, 2019

an fatal -> a fatal

@BridgeAR BridgeAR changed the title process: inspect error in case of an fatal exception process: inspect error in case of a fatal exception Apr 30, 2019
@BridgeAR
process: inspect error in case of a fatal exception
f044b21 
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

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 3, 2019 •
edited by BridgeAR
Loading

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
Copy link
Member Author

BridgeAR commented May 7, 2019

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

@BridgeAR
Copy link
Member Author

@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
Trott approved these changes May 13, 2019
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

BridgeAR added a commit to BridgeAR/node that referenced this pull request May 16, 2019
@BridgeAR
process: inspect error in case of a fatal exception
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: nodejs#27243
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BridgeAR
Copy link
Member Author

BridgeAR commented May 16, 2019 •
edited
Loading

Thanks a lot for the reviews!

Landed in a9f518c 🎉

@BridgeAR BridgeAR closed this May 16, 2019
targos pushed a commit that referenced this pull request May 17, 2019
@BridgeAR @targos
process: inspect error in case of a fatal exception
0393045 
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 <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BridgeAR added a commit that referenced this pull request May 21, 2019
@BridgeAR
2019-05-21, Version 12.3.0 (Current)
e0afe0e 
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
@BridgeAR BridgeAR mentioned this pull request May 21, 2019
Merged
4 tasks
BridgeAR added a commit that referenced this pull request May 21, 2019
@BridgeAR
2019-05-21, Version 12.3.0 (Current)
20c11be 
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 a commit that referenced this pull request May 21, 2019
@BridgeAR
2019-05-21, Version 12.3.0 (Current)
9fec70a 
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
@mmarchini mmarchini mentioned this pull request Jun 25, 2019
Closed
@BridgeAR BridgeAR deleted the improve-debugging branch January 20, 2020 12:03
@MoLow MoLow mentioned this pull request Sep 21, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devsnek devsnek devsnek left review comments

@joyeecheung joyeecheung joyeecheung left review comments

@addaleax addaleax addaleax approved these changes

@targos targos targos approved these changes

@Trott Trott Trott approved these changes

@jasnell jasnell Awaiting requested review from jasnell

@mcollina mcollina Awaiting requested review from mcollina

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. process Issues and PRs related to the process subsystem. review wanted PRs that need reviews. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@BridgeAR @nodejs-github-bot @benjamingr @Fishrock123 @targos @Trott @addaleax @joyeecheung @devsnek @BethGriggs