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.

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

src: allow fatal exceptions to be enhanced #28562

Merged
merged 1 commit into from
Jul 7, 2019
Merged

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Jul 5, 2019

This commit allows fatal exceptions to be enhanced so that exceptions thrown from an 'unhandledException' handler have the stack attached properly.

I still need to add a test, but first I want to make sure the approach is acceptable. If it's not, another approach could be passing kEnhance/kDontEnhance to the TryCatchScope constructor.

Fixes: #28550

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jul 5, 2019
src/node_errors.cc Outdated Show resolved Hide resolved
@joyeecheung
Copy link
Member

If it's not, another approach could be passing kEnhance/kDontEnhance to the TryCatchScope constructor

Alternatively we can just use a normal v8::TryCatch in TriggerUncaughtException and don't touch node::errors::TryCatchScope - but it's also likely that we want to enhance the stack for other uses of node::errors::TryCatchScope

@devsnek
Copy link
Member

devsnek commented Jul 5, 2019

i'd say instead of having a concept of enhance we should just check for CanContinue, and always get a better trace if possible.

@cjihrig cjihrig force-pushed the err-stack branch 3 times, most recently from 5aa189d to 1f21d4f Compare July 5, 2019 18:09
@cjihrig
Copy link
Contributor Author

cjihrig commented Jul 5, 2019

Added a test, and took CanContinue() into consideration.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jul 6, 2019

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

EDIT(cjihrig): CI was green.

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 6, 2019
This commit allows fatal exceptions to be enhanced so that
exceptions thrown from an unhandledException handler have
the stack attached properly.

PR-URL: nodejs#28562
Fixes: nodejs#28550
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@cjihrig cjihrig merged commit 36fdf1a into nodejs:master Jul 7, 2019
@cjihrig cjihrig deleted the err-stack branch July 7, 2019 15:58
basti1302 added a commit to instana/nodejs that referenced this pull request Jul 7, 2019
The issue should be fixed in the next Node release, see
nodejs/node#28562.

Also, log a warning when Node.js v12.6.0 is used and uncaught exception
reporting is enabled.
targos pushed a commit that referenced this pull request Jul 20, 2019
This commit allows fatal exceptions to be enhanced so that
exceptions thrown from an unhandledException handler have
the stack attached properly.

PR-URL: #28562
Fixes: #28550
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
This was referenced Jul 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stack trace gets lost in Node.js 12.6.0 when rethrowing from uncaught exception handler
7 participants