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

inspector: log exceptions in message handlers #14980

Merged
merged 0 commits into from
Sep 7, 2017
Merged

inspector: log exceptions in message handlers #14980

merged 0 commits into from
Sep 7, 2017

Conversation

eugeneo
Copy link
Contributor

@eugeneo eugeneo commented Aug 22, 2017

Fixes: #14965

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

inspector: JS bindings were updated.

@nodejs-github-bot nodejs-github-bot added the inspector Issues and PRs related to the V8 inspector protocol label Aug 22, 2017
@eugeneo
Copy link
Contributor Author

eugeneo commented Aug 22, 2017

I am not entirely sure what to do with the exceptions here. post is the main entry point of this API, but the problem is that multiple message handlers can be called within one call to post. Some options that were considered:

  1. Log exception and do not throw - this is what this PR does. This is altering current behavior where the exceptions used to be thrown from the post.
  2. Throw it from process.nextTick - nextTick may never happen, e.g. if the handler has issues while handling messages while V8 is suspended.
  3. Introduce 'handlerError' event on session object with the default handler that does logging.
  4. Throwing first or last exception from the post. Log all other messages.
  5. Stop calling the callbacks after the first exception, rethrow the exception from the post

Any suggestions on a best approach?

@jasnell
Copy link
Member

jasnell commented Aug 24, 2017

@eugeneo ... would you consider these to be fatal events?

If so, then option 5 would likely be best, but it's also the most draconian.

If they are not fatal, then a 6th option would be to use process.emitWarning() to emit each message as a separate warning. Users can then decide for themselves how they would prefer to handle them (e.g. use the default output to stderr, redirect those to a file, register a warning handler, switch them off entirely, etc).

@eugeneo
Copy link
Contributor Author

eugeneo commented Aug 24, 2017

@jasnell Thank you for the suggestion. I definitely don't think those events are fatal - so now the code emits warnings.

jasnell
jasnell reviewed Aug 24, 2017
View reviewed changes
test/inspector/test-bindings.js Outdated
let attempt = 1;
const promise = new Promise(function(resolve) {
let count = 0;
process.on('warning', () => {
Copy link
Member

Choose a reason for hiding this comment

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

you can use common.mustCall(3) here to limit the number of warning events triggered.

jasnell
jasnell approved these changes Aug 24, 2017
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM with green CI and a suggestion.

@eugeneo
Copy link
Contributor Author

eugeneo commented Aug 24, 2017

Thank you for the review, I updated the code. CI: https://ci.nodejs.org/job/node-test-pull-request/9829/

@TimothyGu
Copy link
Member

Would it be possible to call the callbacks in a process.nextTick?

@eugeneo
Copy link
Contributor Author

eugeneo commented Aug 25, 2017

@TimothyGu I'm bit concerned it might be possible to deadlock inspector callback and never see the nextTick happen (e.g. if the bad callback is invoked while the V8 is suspended and causes an infinite loop somehow).

@TimothyGu
Copy link
Member

@eugeneo Sounds quite complicated… I'll defer to your judgment then!

@eugeneo
Copy link
Contributor Author

eugeneo commented Aug 30, 2017

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

@jasnell
Copy link
Member

jasnell commented Aug 30, 2017

Hmm... Getting some failures on 32-bit systems. See: https://ci.nodejs.org/job/node-test-commit-linux/12132/nodes=debian8-x86/console

@eugeneo
Copy link
Contributor Author

eugeneo commented Aug 30, 2017

@jasnell I'm fixing them - there's an unexpected warning firing there.

@BridgeAR
Copy link
Member

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

@eugeneo
Copy link
Contributor Author

eugeneo commented Aug 31, 2017

CI failures were not caused by this CL but I am not confident with this test coverage enough to merge this PR.

@jasnell jasnell added the wip Issues and PRs that are still a work in progress. label Aug 31, 2017
@jasnell
Copy link
Member

jasnell commented Aug 31, 2017

I've added in the in progress label so folks can know not to land it until you're more comfortable with it.

@eugeneo
Copy link
Contributor Author

eugeneo commented Sep 7, 2017

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

@eugeneo eugeneo closed this Sep 7, 2017
@eugeneo eugeneo merged commit 9bae3ea into nodejs:master Sep 7, 2017
@eugeneo
Copy link
Contributor Author

eugeneo commented Sep 7, 2017

Landed as 9bae3ea

MylesBorins pushed a commit that referenced this pull request Sep 10, 2017
@MylesBorins
inspector: log exceptions in message handlers
5cb81bd 
Fixes: #14965
PR-URL: #14980
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Sep 10, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Sep 11, 2017
@MylesBorins
inspector: log exceptions in message handlers
a81a0f4 
Fixes: #14965
PR-URL: #14980
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 12, 2017
@MylesBorins
inspector: log exceptions in message handlers
105acf4 
Fixes: #14965
PR-URL: #14980
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
addaleax pushed a commit to addaleax/node that referenced this pull request Sep 13, 2017
@addaleax
inspector: log exceptions in message handlers
d19377c 
Fixes: nodejs#14965
PR-URL: nodejs#14980
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
@eugeneo eugeneo deleted the profiler-aix branch April 26, 2018 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
inspector Issues and PRs related to the V8 inspector protocol wip Issues and PRs that are still a work in progress.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

exceptions in inspector callback cause fatal error
6 participants
@eugeneo @jasnell @TimothyGu @BridgeAR @MylesBorins @nodejs-github-bot