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: fix abort-on-uncaught-exception #3038

Conversation

misterdjules
Copy link

This PR fixes 0af4c9e so that node
aborts at the right time when throwing an error and using
--abort-on-uncaught-exception.

Basically, it wraps most node internal callbacks with:

if (!domain || domain.emittingTopLevelError)
  runCallback();
else {
  try {
    runCallback();
  } catch (err) {
    process._fatalException(err);
  }
}

so that V8 can abort properly in Isolate::Throw if
--abort-on-uncaught-exception was passed on the command line, and domain
can handle the error if one is active and not already in the top level
domain's error handler.

It also reverts 921f2de partially:
node::FatalException does not abort anymore because at that time, it's
already too late.

It adds process._forceTickDone, which is really a hack to allow
test-next-tick-error-spin.js to pass and start the discussion. It's here to basically avoid an
infinite recursion when throwing in a domain from a nextTick callback,
and queuing the same callback on the next tick from the domain's error
handler.

This change is an alternative approach to #3036 for fixing #3035.

Fixes #3035.

/cc @nodejs/post-mortem

This PR fixes 0af4c9e so that node
aborts at the right time when throwing an error and using
--abort-on-uncaught-exception.

Basically, it wraps most node internal callbacks with:

if (!domain || domain.emittingTopLevelError)
  runCallback();
else {
  try {
    runCallback();
  } catch (err) {
    process._fatalException(err);
  }
}

so that V8 can abort properly in Isolate::Throw if
--abort-on-uncaught-exception was passed on the command line, and domain
can handle the error if one is active and not already in the top level
domain's error handler.

It also reverts 921f2de partially:
node::FatalException does not abort anymore because at that time, it's
already too late.

It adds process._forceTickDone, which is really a hack to allow
test-next-tick-error-spin.js to pass. It's here to basically avoid an
infinite recursion when throwing in a domain from a nextTick callback,
and queuing the same callback on the next tick from the domain's error
handler.

This change is an alternative approach to nodejs#3036 for fixing nodejs#3035.

Fixes nodejs#3035.
@misterdjules misterdjules added the post-mortem Issues and PRs related to the post-mortem diagnostics of Node.js. label Sep 24, 2015
@misterdjules
Copy link
Author

For now, I'm looking for comments on the general approach, not on details. An alternative approach at fixing the same issue is #3036.

@Fishrock123
Copy link
Member

FWIW I'm all for not screwing more with timers than we need too. It's already complex enough.

@misterdjules
Copy link
Author

Closing in favor of #3036 now that #3036's V8-related change landed upstream. Please feel free to express your disagreement though if you think it's not the right way to move forward.

@misterdjules misterdjules deleted the fix-abort-on-uncaught-exception-bis branch July 24, 2017 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
post-mortem Issues and PRs related to the post-mortem diagnostics of Node.js.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

node does not abort at the right time when using --abort-on-uncaught-exception
2 participants