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

src: disconnect inspector before exiting out of fatal exception #29611

Closed
wants to merge 3 commits into from
Closed

src: disconnect inspector before exiting out of fatal exception #29611

wants to merge 3 commits into from

Conversation

joyeecheung
Copy link
Member

So that coverage, .etc are properly written in case of a normal
fatal exception.

Fixes: #29570

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

@joyeecheung
src: disconnect inspector before exiting out of fatal exception
83fc7ad 
So that coverage, .etc are properly written in case of a normal
fatal exception.
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Sep 19, 2019
@joyeecheung
Copy link
Member Author

cc @bcoe @addaleax

@joyeecheung joyeecheung added errors Issues and PRs related to JavaScript errors originated in Node.js core. coverage Issues and PRs related to native coverage support. labels Sep 19, 2019
@nodejs-github-bot
Copy link
Collaborator

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

joyeecheung
joyeecheung commented Sep 19, 2019
View reviewed changes
src/node_errors.cc Outdated
@@ -978,6 +978,7 @@ void TriggerUncaughtException(Isolate* isolate,

// Now we are certain that the exception is fatal.
ReportFatalException(env, error, message, EnhanceFatalException::kEnhance);
WaitForInspectorDisconnect(env);
Copy link
Member Author

@joyeecheung joyeecheung Sep 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I didn't leave this for other Exit() calls in TriggerUncaughtException() as those cases are more abnormal than this (e.g. the global uncaught exception handler throws itself, or the global hook is monkey-pached incorrectly) and I think it would unsafe to wait until the inspector shut down in those cases.

joyeecheung
joyeecheung commented Sep 19, 2019
View reviewed changes
src/node_errors.cc
@@ -978,6 +978,9 @@ void TriggerUncaughtException(Isolate* isolate,

// Now we are certain that the exception is fatal.
ReportFatalException(env, error, message, EnhanceFatalException::kEnhance);
#if HAVE_INSPECTOR
profiler::EndStartedProfilers(env);
Copy link
Member Author

Choose a reason for hiding this comment

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

Uh just realized we should call profiler::EndStartedProfilers instead since the whole WaitForInspectorDisconnect resets signals.

patilharshal16
patilharshal16 reviewed Sep 19, 2019
View reviewed changes
test/fixtures/v8-coverage/throw.js
@@ -0,0 +1,7 @@
const a = 99;
if (true) {
Copy link

Choose a reason for hiding this comment

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

no point of adding else block

Copy link
Member Author

@joyeecheung joyeecheung Sep 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

This is in the test though?

bcoe
bcoe suggested changes Sep 19, 2019
View reviewed changes
Copy link
Contributor

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

One little nit with regards to test name 😄

Thanks for digging into this, I thought it would be something obvious.

test/parallel/test-v8-coverage.js Outdated
@@ -35,6 +35,24 @@ function nextdir() {
assert.strictEqual(fixtureCoverage.functions[0].ranges[1].count, 0);
}

// Outputs coverage when process.exit(1) exits process.
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps, change to:

// Outputs coverage when error is thrown in first tick.

Copy link
Member Author

Choose a reason for hiding this comment

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

Uh thanks for catching my copy-past error :)

@nodejs-github-bot
Copy link
Collaborator

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

@danbev danbev added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 23, 2019
@Trott
Copy link
Member

Trott commented Sep 24, 2019

@joyeecheung This is ready to land in your opinion?

@joyeecheung
Copy link
Member Author

@Trott yes

bcoe pushed a commit to bcoe/node-1 that referenced this pull request Sep 24, 2019
@joyeecheung @bcoe
src: disconnect inspector before exiting out of fatal exception
b263423 
So that coverage, .etc are properly written in case of a normal
fatal exception.

PR-URL: nodejs#29611
Fixes: nodejs#29570
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Ben Coe <bencoe@gmail.com>
@bcoe
Copy link
Contributor

bcoe commented Sep 24, 2019

Landed in b263423

@bcoe bcoe closed this Sep 24, 2019
targos pushed a commit that referenced this pull request Oct 1, 2019
@joyeecheung @targos
src: disconnect inspector before exiting out of fatal exception
a86b71f 
So that coverage, .etc are properly written in case of a normal
fatal exception.

PR-URL: #29611
Fixes: #29570
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Ben Coe <bencoe@gmail.com>
@targos targos mentioned this pull request Oct 1, 2019
Merged
@beevelop beevelop mentioned this pull request Oct 1, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@patilharshal16 patilharshal16 patilharshal16 left review comments

@devnexen devnexen devnexen approved these changes

@cjihrig cjihrig cjihrig approved these changes

@bcoe bcoe bcoe approved these changes

Assignees
No one assigned
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++. coverage Issues and PRs related to native coverage support. errors Issues and PRs related to JavaScript errors originated in Node.js core.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

NODE_V8_COVERAGE not output when Error thrown during initialization
8 participants
@joyeecheung @nodejs-github-bot @Trott @bcoe @cjihrig @devnexen @patilharshal16 @danbev