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 #3036

Closed

Conversation

misterdjules
Copy link

Revert 0af4c9e, parts of
921f2de and port
nodejs/node-v0.x-archive#25835 from v0.12 to
master so that node aborts at the right time when an error is thrown
and --abort-on-uncaught-exception is used.

This change floats a patch on top of V8 that has not been submitted
upstream. If this change turns out to be the preferred approach to fix
issue #3035, the V8 related changes will be submitted upstream.

Fixes #3035.

EDIT: The V8 related changes have landed upstream, see https://codereview.chromium.org/1375933003/.

@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

/cc @nodejs/post-mortem

This is one of the proposed approaches to fix #3035. For now, I'm looking for feedback on the general approach. I'll submit another PR shortly that takes a different approach and reference it here.

} else {
ret = cb->Call(context, argc, argv);
}
Local<Value> ret = cb->Call(context, argc, argv);;
Copy link
Contributor

Choose a reason for hiding this comment

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

extra semi-colon at the end.

Copy link
Author

Choose a reason for hiding this comment

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

Will fix.

misterdjules pushed a commit to misterdjules/node-1 that referenced this pull request Sep 24, 2015
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
Copy link
Author

@thefourtheye Thanks for the review, updated this PR according to your comments.

@misterdjules
Copy link
Author

Also, just to be really clear, an alternative approach to this PR is available with #3038. At this point I'm really looking for comments on the general approach so that we can decide how to move forward and fix #3035.

* - the custom callback set returns true.
* Otherwise it won't abort.
*/
typedef bool (*abort_on_uncaught_exception_t)(Isolate*);
Copy link
Member

Choose a reason for hiding this comment

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

I'd make it return an enum so that it can be extended easily in the future.

The naming is kind of incongruous with V8's normal style. UncaughtExceptionCallback is closer to what I'd expect.

For that matter, is it actually necessary to abort inside V8? You could rewrite the logic in isolate.cc to this:

if (uncaught_exception_callback_ != nullptr &&
    PredictExceptionCatcher() != CAUGHT_BY_JAVASCRIPT) {
  uncaught_exception_callback_(reinterpret_cast<v8::Isolate*>(this));
}

It's then up to the callback to abort or not. You can optionally pass message_obj as the second argument.

Copy link
Author

Choose a reason for hiding this comment

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

I'd make it return an enum so that it can be extended easily in the future.

Good point.

The naming is kind of incongruous with V8's normal style. UncaughtExceptionCallback is closer to what I'd expect.

Sounds good too 👍

For that matter, is it actually necessary to abort inside V8?

It's not. However in case there's no uncaught exception callback set by the embedder, V8 has to abort. It seemed more consistent to me to abort in the same frame whether or not such a callback is set.

Copy link
Member

Choose a reason for hiding this comment

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

I'd combine it like this:

if (uncaught_exception_callback_ != nullptr &&
    PredictExceptionCatcher() != CAUGHT_BY_JAVASCRIPT) {
  uncaught_exception_callback_(reinterpret_cast<v8::Isolate*>(this));
} else if (FLAG_abort_on_uncaught_exception &&
           PredictExceptionCatcher() != CAUGHT_BY_JAVASCRIPT) {
  // ...
  base::OS::Abort();
}

In a perfect world --abort_on_uncaught_exception wouldn't exist in V8 and just be handled by the embedder.

Copy link
Author

Choose a reason for hiding this comment

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

Before I continue commenting, I just want to say that I'm thinking out loud and trying to find the right set of abstractions. I think your suggestions are great and help move this PR forward. I really really appreciate them and don't want to sound like I'm dismissing them, and I would appreciate if we could continue this constructive conversation.

The problem with aborting in node or in V8 when using --abort-on-uncaught-exception is consistency. For instance, to get the same exit codes and signals in both cases would require to use the same abort implementation. In this case, it would mean using the same implementation as base::OS::Abort(), which we can't use since it's not part of V8's public API (unless I'm missing something). This implementation has changed in the past and we would need to update our implementation to match V8's implementation if it changes again.

While it might not seem like a big issue, to me it indicates that we haven't found the right abstraction.

Now that I've implemented the change you suggested to have an enum rather than a boolean as the return value type for UncaughtExceptionCallback, I have the same concerns. When FLAG_abort_on_uncaught_exception is set, it doesn't make sense to me to do anything than aborting or not aborting (as an exception to the rule). Encoding the different behaviors at that point in an enum means that the UncaughtExceptionCallback in node is written like following:

Isolate::UncaughtExceptionPolicy OnUncaughtException(Isolate* isolate) {
  if (need_to_not_abort) {
   return Isolate::UncaughtExceptionPolicy::NO_ABORT;
  } else {
   return Isolate::UncaughtExceptionPolicy::ABORT;
  }
}

My concern with this implementation is that it's written like if node could do something else than aborting or not aborting, like swallowing the exception. But in reality V8 will abort or exit, but nothing else would make sense.

In a perfect world --abort_on_uncaught_exception wouldn't exist in V8 and just be handled by the embedder.

I don't know. node is maybe the only embedder taking advantage of this command line switch today, but to me it seems general enough to be used by any other embedder. For instance, even though I haven't done it yet, using d8 with --abort-on-uncaught-exception would be a good way for me to test mdb_v8 with recent V8 versions that haven't been integrated into node.

mdb_v8 is also a post-mortem debugger for any V8 embedder, not just node. These examples still revolve around mdb_v8, but I think it illustrates that --abort-on-uncaught-exception is a sound concept for any V8 embedder.

So I'm back to thinking that having the UncaughtExceptionCallback return a bool is better than having it return an enum, but as I said at the beginning of this comment I'm open to any further feedback. It might also help to continue the conversation on IRC for a bit, I'm "jgi" in #io.js, #libuv and #v8.

@misterdjules misterdjules force-pushed the fix-abort-on-uncaught-exception branch 2 times, most recently from f5d2cf0 to 76b7c6f Compare September 29, 2015 00:03
var self = this;

function emitError() {
var handled = self.emit('error', er);
Copy link
Contributor

Choose a reason for hiding this comment

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

const

Copy link
Author

Choose a reason for hiding this comment

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

It would be great to automate this rule with our linting tools. Is this something that has been investigated before?

Using the prefer-const rule does not apply to function-scoped variable declarations, so we would need to use the no-var eslint rule too, which I assume would require a significant amount of work. I'm also not sure about all the implications since I'm not familiar with ES6.

I'm not a big fan of having implicit coding style rules, because it becomes frustrating for maintainers to enforce them every time, and for contributors because they don't have any tool to check that their code complies to the guidelines.

@thefourtheye @nodejs/tsc Any thoughts on this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Rod suggested sometime back to think more about suggesting const. So cc ing @rvagg

Copy link
Author

Choose a reason for hiding this comment

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

Created #3118 to discuss this.

Copy link
Contributor

Choose a reason for hiding this comment

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

@misterdjules Cool. Thanks :-)

@misterdjules
Copy link
Author

FYI submitted https://codereview.chromium.org/1375933003/ to V8, which contains the V8 related changes of this PR.

@davepacheco
Copy link
Contributor

+1 to this approach over #3038.

Backport 1ee712ab8687e5f4dec93d45da068d37d28feb8b from V8 upstream.

Original commit message:

  Add SetAbortOnUncaughtExceptionCallback API

  The --abort-on-uncaught-exception command line switch makes
  Isolate::Throw abort if the error being thrown cannot be caught by a
  try/catch block.

  Embedders may want to use other mechanisms than try/catch blocks to
  handle uncaught exceptions. For instance, Node.js has "domain" objects
  that have error handlers that can handle uncaught exception like
  following:

  var d = domain.create();

  d.on('error', function onError(err) {
    console.log('Handling error');
  });

  d.run(function() {
    throw new Error("boom");
  });

  These error handlers are called by isolates' message listeners.

  If --abort-on-uncaught-exception is *not* used, the isolate's
  message listener will be called, which will in turn call the domain's
  error handler. The process will output 'Handling error' and will exit
  successfully (not due to an uncaught exception). This is the behavior
  that Node.js users expect.

  However, if --abort-on-uncaught-exception is used and when throwing an
  error within a domain that has an error handler, the process will abort
  and the domain's error handler will not be called. This is not the
  behavior that Node.js users expect.

  Having a SetAbortOnUncaughtExceptionCallback API allows embedders to
  determine when it's not appropriate to abort and instead handle the
  exception via the isolate's message listener.

  In the example above, Node.js would set a custom callback with
  SetAbortOnUncaughtExceptionCallback that would be implemented as
  following (the sample code has been simplified to remove what's not
  relevant to this change):

  bool ShouldAbortOnUncaughtException(Isolate* isolate) {
    return !IsDomainActive();
  }

  Now when --abort-on-uncaught-exception is used, Isolate::Throw would
  call that callback and determine that it should not abort if a domain
  with an error handler is active. Instead, the isolate's message listener
  would be called and the error would be handled by the domain's error
  handler.

  I believe this can also be useful for other embedders.

  BUG=

  R=bmeurer@chromium.org

  Review URL: https://codereview.chromium.org/1375933003

  Cr-Commit-Position: refs/heads/master@{nodejs#31111}
@misterdjules misterdjules force-pushed the fix-abort-on-uncaught-exception branch from 76b7c6f to 63d3ec8 Compare October 5, 2015 20:27
@misterdjules
Copy link
Author

V8-related changes landed upstream, see https://codereview.chromium.org/1375933003/.

As a result, I split the changes in two commits: one that back ports the V8 change, and one with node-only changes.

The author of the commit with node changes was changed to @whitlockjc because he was the author of the back ported change, that is nodejs/node-v0.x-archive#25835.

/cc @nodejs/post-mortem @nodejs/collaborators @nodejs/tsc

@jasnell jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Oct 8, 2015
@jasnell
Copy link
Member

jasnell commented Oct 8, 2015

@bnoordhuis good point. I've added the appropriate tag and will get the changelog for 4.2 updated appropriately. @misterdjules, will you be working on getting the change upstreamed?

@misterdjules
Copy link
Author

@jasnell The change has already been upstreamed.

@bnoordhuis Are you suggesting that we do something to get that change into V8's upstream 4.5 branch?

@bnoordhuis
Copy link
Member

@misterdjules Yes. There's a tools/release/merge_to_branch.py script you can use to cherry-pick a commit from master to a release branch. I don't know what the V8's team stance is on introducing new APIs in stable branches but I'd at least give it a try.

@misterdjules
Copy link
Author

@bnoordhuis I didn't know that, thanks! I'll give it a try and keep you updated.

@misterdjules
Copy link
Author

@misterdjules
Copy link
Author

It seems that the V8 related changes won't make it into a release until V8 4.8: https://code.google.com/p/v8/issues/detail?id=4496.

misterdjules pushed a commit that referenced this pull request Oct 25, 2015
Backport 1ee712ab8687e5f4dec93d45da068d37d28feb8b from V8 upstream.

Original commit message:

  Add SetAbortOnUncaughtExceptionCallback API

  The --abort-on-uncaught-exception command line switch makes
  Isolate::Throw abort if the error being thrown cannot be caught by a
  try/catch block.

  Embedders may want to use other mechanisms than try/catch blocks to
  handle uncaught exceptions. For instance, Node.js has "domain" objects
  that have error handlers that can handle uncaught exception like
  following:

  var d = domain.create();

  d.on('error', function onError(err) {
    console.log('Handling error');
  });

  d.run(function() {
    throw new Error("boom");
  });

  These error handlers are called by isolates' message listeners.

  If --abort-on-uncaught-exception is *not* used, the isolate's
  message listener will be called, which will in turn call the domain's
  error handler. The process will output 'Handling error' and will exit
  successfully (not due to an uncaught exception). This is the behavior
  that Node.js users expect.

  However, if --abort-on-uncaught-exception is used and when throwing an
  error within a domain that has an error handler, the process will abort
  and the domain's error handler will not be called. This is not the
  behavior that Node.js users expect.

  Having a SetAbortOnUncaughtExceptionCallback API allows embedders to
  determine when it's not appropriate to abort and instead handle the
  exception via the isolate's message listener.

  In the example above, Node.js would set a custom callback with
  SetAbortOnUncaughtExceptionCallback that would be implemented as
  following (the sample code has been simplified to remove what's not
  relevant to this change):

  bool ShouldAbortOnUncaughtException(Isolate* isolate) {
    return !IsDomainActive();
  }

  Now when --abort-on-uncaught-exception is used, Isolate::Throw would
  call that callback and determine that it should not abort if a domain
  with an error handler is active. Instead, the isolate's message listener
  would be called and the error would be handled by the domain's error
  handler.

  I believe this can also be useful for other embedders.

  BUG=

  R=bmeurer@chromium.org

  Review URL: https://codereview.chromium.org/1375933003

  Cr-Commit-Position: refs/heads/master@{#31111}

Ref: #3036
PR-URL: #3481
Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com>
ofrobots pushed a commit to ofrobots/node that referenced this pull request Nov 12, 2015
Backport 1ee712ab8687e5f4dec93d45da068d37d28feb8b from V8 upstream.

Original commit message:

  Add SetAbortOnUncaughtExceptionCallback API

  The --abort-on-uncaught-exception command line switch makes
  Isolate::Throw abort if the error being thrown cannot be caught by a
  try/catch block.

  Embedders may want to use other mechanisms than try/catch blocks to
  handle uncaught exceptions. For instance, Node.js has "domain" objects
  that have error handlers that can handle uncaught exception like
  following:

  var d = domain.create();

  d.on('error', function onError(err) {
    console.log('Handling error');
  });

  d.run(function() {
    throw new Error("boom");
  });

  These error handlers are called by isolates' message listeners.

  If --abort-on-uncaught-exception is *not* used, the isolate's
  message listener will be called, which will in turn call the domain's
  error handler. The process will output 'Handling error' and will exit
  successfully (not due to an uncaught exception). This is the behavior
  that Node.js users expect.

  However, if --abort-on-uncaught-exception is used and when throwing an
  error within a domain that has an error handler, the process will abort
  and the domain's error handler will not be called. This is not the
  behavior that Node.js users expect.

  Having a SetAbortOnUncaughtExceptionCallback API allows embedders to
  determine when it's not appropriate to abort and instead handle the
  exception via the isolate's message listener.

  In the example above, Node.js would set a custom callback with
  SetAbortOnUncaughtExceptionCallback that would be implemented as
  following (the sample code has been simplified to remove what's not
  relevant to this change):

  bool ShouldAbortOnUncaughtException(Isolate* isolate) {
    return !IsDomainActive();
  }

  Now when --abort-on-uncaught-exception is used, Isolate::Throw would
  call that callback and determine that it should not abort if a domain
  with an error handler is active. Instead, the isolate's message listener
  would be called and the error would be handled by the domain's error
  handler.

  I believe this can also be useful for other embedders.

  BUG=

  R=bmeurer@chromium.org

  Review URL: https://codereview.chromium.org/1375933003

  Cr-Commit-Position: refs/heads/master@{nodejs#31111}

Ref: nodejs#3036
PR-URL: nodejs#3481
Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com>
misterdjules pushed a commit that referenced this pull request Nov 13, 2015
Backport 1ee712ab8687e5f4dec93d45da068d37d28feb8b from V8 upstream.

Original commit message:

  Add SetAbortOnUncaughtExceptionCallback API

  The --abort-on-uncaught-exception command line switch makes
  Isolate::Throw abort if the error being thrown cannot be caught by a
  try/catch block.

  Embedders may want to use other mechanisms than try/catch blocks to
  handle uncaught exceptions. For instance, Node.js has "domain" objects
  that have error handlers that can handle uncaught exception like
  following:

  var d = domain.create();

  d.on('error', function onError(err) {
    console.log('Handling error');
  });

  d.run(function() {
    throw new Error("boom");
  });

  These error handlers are called by isolates' message listeners.

  If --abort-on-uncaught-exception is *not* used, the isolate's
  message listener will be called, which will in turn call the domain's
  error handler. The process will output 'Handling error' and will exit
  successfully (not due to an uncaught exception). This is the behavior
  that Node.js users expect.

  However, if --abort-on-uncaught-exception is used and when throwing an
  error within a domain that has an error handler, the process will abort
  and the domain's error handler will not be called. This is not the
  behavior that Node.js users expect.

  Having a SetAbortOnUncaughtExceptionCallback API allows embedders to
  determine when it's not appropriate to abort and instead handle the
  exception via the isolate's message listener.

  In the example above, Node.js would set a custom callback with
  SetAbortOnUncaughtExceptionCallback that would be implemented as
  following (the sample code has been simplified to remove what's not
  relevant to this change):

  bool ShouldAbortOnUncaughtException(Isolate* isolate) {
    return !IsDomainActive();
  }

  Now when --abort-on-uncaught-exception is used, Isolate::Throw would
  call that callback and determine that it should not abort if a domain
  with an error handler is active. Instead, the isolate's message listener
  would be called and the error would be handled by the domain's error
  handler.

  I believe this can also be useful for other embedders.

  BUG=

  R=bmeurer@chromium.org

  Review URL: https://codereview.chromium.org/1375933003

  Cr-Commit-Position: refs/heads/master@{#31111}

Ref: #3036
PR-URL: #3481
Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com>
@MylesBorins
Copy link
Contributor

landed in v4.x-staging as 74f4435...546e833

@misterdjules
Copy link
Author

@thealphanerd Just to make sure we're on the same page, do you mean it had already landed in v4.x-staging, or that you just landed it in v4.x-staging?

Usually we write "landed in some-branch as some-commit [, some-other commits]" when we just land commits, not as an indication that it had already landed before.

@MylesBorins
Copy link
Contributor

@misterdjules I was documenting that it had landed. I was going through all commits with the land-on-4.x tag as I was landing stuff. Added in that comment to document what had landed on LTS in threads where it was missed.

I can use different language in the future to specify that I didn't land it, or just not make notes at all. Mostly was just trying to keep things consistent

@misterdjules
Copy link
Author

@thealphanerd OK, that's great, thanks for doing that! I would suggest using a slightly different language, just to not confuse those messages with the ones we use when creating new commits.

Something like "This had already landed in v4.x as...".

@MylesBorins
Copy link
Contributor

Makes sense to me. Will use different language if doing this in the future 😄

ofrobots pushed a commit to ofrobots/node that referenced this pull request Dec 1, 2015
Backport 1ee712ab8687e5f4dec93d45da068d37d28feb8b from V8 upstream.

Original commit message:

  Add SetAbortOnUncaughtExceptionCallback API

  The --abort-on-uncaught-exception command line switch makes
  Isolate::Throw abort if the error being thrown cannot be caught by a
  try/catch block.

  Embedders may want to use other mechanisms than try/catch blocks to
  handle uncaught exceptions. For instance, Node.js has "domain" objects
  that have error handlers that can handle uncaught exception like
  following:

  var d = domain.create();

  d.on('error', function onError(err) {
    console.log('Handling error');
  });

  d.run(function() {
    throw new Error("boom");
  });

  These error handlers are called by isolates' message listeners.

  If --abort-on-uncaught-exception is *not* used, the isolate's
  message listener will be called, which will in turn call the domain's
  error handler. The process will output 'Handling error' and will exit
  successfully (not due to an uncaught exception). This is the behavior
  that Node.js users expect.

  However, if --abort-on-uncaught-exception is used and when throwing an
  error within a domain that has an error handler, the process will abort
  and the domain's error handler will not be called. This is not the
  behavior that Node.js users expect.

  Having a SetAbortOnUncaughtExceptionCallback API allows embedders to
  determine when it's not appropriate to abort and instead handle the
  exception via the isolate's message listener.

  In the example above, Node.js would set a custom callback with
  SetAbortOnUncaughtExceptionCallback that would be implemented as
  following (the sample code has been simplified to remove what's not
  relevant to this change):

  bool ShouldAbortOnUncaughtException(Isolate* isolate) {
    return !IsDomainActive();
  }

  Now when --abort-on-uncaught-exception is used, Isolate::Throw would
  call that callback and determine that it should not abort if a domain
  with an error handler is active. Instead, the isolate's message listener
  would be called and the error would be handled by the domain's error
  handler.

  I believe this can also be useful for other embedders.

  BUG=

  R=bmeurer@chromium.org

  Review URL: https://codereview.chromium.org/1375933003

  Cr-Commit-Position: refs/heads/master@{nodejs#31111}

Ref: nodejs#3036
PR-URL: nodejs#3481
Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com>
ofrobots pushed a commit to ofrobots/node that referenced this pull request Dec 4, 2015
Backport 1ee712ab8687e5f4dec93d45da068d37d28feb8b from V8 upstream.

Original commit message:

  Add SetAbortOnUncaughtExceptionCallback API

  The --abort-on-uncaught-exception command line switch makes
  Isolate::Throw abort if the error being thrown cannot be caught by a
  try/catch block.

  Embedders may want to use other mechanisms than try/catch blocks to
  handle uncaught exceptions. For instance, Node.js has "domain" objects
  that have error handlers that can handle uncaught exception like
  following:

  var d = domain.create();

  d.on('error', function onError(err) {
    console.log('Handling error');
  });

  d.run(function() {
    throw new Error("boom");
  });

  These error handlers are called by isolates' message listeners.

  If --abort-on-uncaught-exception is *not* used, the isolate's
  message listener will be called, which will in turn call the domain's
  error handler. The process will output 'Handling error' and will exit
  successfully (not due to an uncaught exception). This is the behavior
  that Node.js users expect.

  However, if --abort-on-uncaught-exception is used and when throwing an
  error within a domain that has an error handler, the process will abort
  and the domain's error handler will not be called. This is not the
  behavior that Node.js users expect.

  Having a SetAbortOnUncaughtExceptionCallback API allows embedders to
  determine when it's not appropriate to abort and instead handle the
  exception via the isolate's message listener.

  In the example above, Node.js would set a custom callback with
  SetAbortOnUncaughtExceptionCallback that would be implemented as
  following (the sample code has been simplified to remove what's not
  relevant to this change):

  bool ShouldAbortOnUncaughtException(Isolate* isolate) {
    return !IsDomainActive();
  }

  Now when --abort-on-uncaught-exception is used, Isolate::Throw would
  call that callback and determine that it should not abort if a domain
  with an error handler is active. Instead, the isolate's message listener
  would be called and the error would be handled by the domain's error
  handler.

  I believe this can also be useful for other embedders.

  BUG=

  R=bmeurer@chromium.org

  Review URL: https://codereview.chromium.org/1375933003

  Cr-Commit-Position: refs/heads/master@{nodejs#31111}

Ref: nodejs#3036
Ref: nodejs#3481
PR-URL: nodejs#4106
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: rvagg - Rod Vagg <rod@vagg.org>
misterdjules pushed a commit that referenced this pull request Dec 4, 2015
Backport 1ee712ab8687e5f4dec93d45da068d37d28feb8b from V8 upstream.

Original commit message:

  Add SetAbortOnUncaughtExceptionCallback API

  The --abort-on-uncaught-exception command line switch makes
  Isolate::Throw abort if the error being thrown cannot be caught by a
  try/catch block.

  Embedders may want to use other mechanisms than try/catch blocks to
  handle uncaught exceptions. For instance, Node.js has "domain" objects
  that have error handlers that can handle uncaught exception like
  following:

  var d = domain.create();

  d.on('error', function onError(err) {
    console.log('Handling error');
  });

  d.run(function() {
    throw new Error("boom");
  });

  These error handlers are called by isolates' message listeners.

  If --abort-on-uncaught-exception is *not* used, the isolate's
  message listener will be called, which will in turn call the domain's
  error handler. The process will output 'Handling error' and will exit
  successfully (not due to an uncaught exception). This is the behavior
  that Node.js users expect.

  However, if --abort-on-uncaught-exception is used and when throwing an
  error within a domain that has an error handler, the process will abort
  and the domain's error handler will not be called. This is not the
  behavior that Node.js users expect.

  Having a SetAbortOnUncaughtExceptionCallback API allows embedders to
  determine when it's not appropriate to abort and instead handle the
  exception via the isolate's message listener.

  In the example above, Node.js would set a custom callback with
  SetAbortOnUncaughtExceptionCallback that would be implemented as
  following (the sample code has been simplified to remove what's not
  relevant to this change):

  bool ShouldAbortOnUncaughtException(Isolate* isolate) {
    return !IsDomainActive();
  }

  Now when --abort-on-uncaught-exception is used, Isolate::Throw would
  call that callback and determine that it should not abort if a domain
  with an error handler is active. Instead, the isolate's message listener
  would be called and the error would be handled by the domain's error
  handler.

  I believe this can also be useful for other embedders.

  BUG=

  R=bmeurer@chromium.org

  Review URL: https://codereview.chromium.org/1375933003

  Cr-Commit-Position: refs/heads/master@{#31111}

Ref: #3036
Ref: #3481
PR-URL: #4106
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: rvagg - Rod Vagg <rod@vagg.org>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
Backport 1ee712ab8687e5f4dec93d45da068d37d28feb8b from V8 upstream.

Original commit message:

  Add SetAbortOnUncaughtExceptionCallback API

  The --abort-on-uncaught-exception command line switch makes
  Isolate::Throw abort if the error being thrown cannot be caught by a
  try/catch block.

  Embedders may want to use other mechanisms than try/catch blocks to
  handle uncaught exceptions. For instance, Node.js has "domain" objects
  that have error handlers that can handle uncaught exception like
  following:

  var d = domain.create();

  d.on('error', function onError(err) {
    console.log('Handling error');
  });

  d.run(function() {
    throw new Error("boom");
  });

  These error handlers are called by isolates' message listeners.

  If --abort-on-uncaught-exception is *not* used, the isolate's
  message listener will be called, which will in turn call the domain's
  error handler. The process will output 'Handling error' and will exit
  successfully (not due to an uncaught exception). This is the behavior
  that Node.js users expect.

  However, if --abort-on-uncaught-exception is used and when throwing an
  error within a domain that has an error handler, the process will abort
  and the domain's error handler will not be called. This is not the
  behavior that Node.js users expect.

  Having a SetAbortOnUncaughtExceptionCallback API allows embedders to
  determine when it's not appropriate to abort and instead handle the
  exception via the isolate's message listener.

  In the example above, Node.js would set a custom callback with
  SetAbortOnUncaughtExceptionCallback that would be implemented as
  following (the sample code has been simplified to remove what's not
  relevant to this change):

  bool ShouldAbortOnUncaughtException(Isolate* isolate) {
    return !IsDomainActive();
  }

  Now when --abort-on-uncaught-exception is used, Isolate::Throw would
  call that callback and determine that it should not abort if a domain
  with an error handler is active. Instead, the isolate's message listener
  would be called and the error would be handled by the domain's error
  handler.

  I believe this can also be useful for other embedders.

  BUG=

  R=bmeurer@chromium.org

  Review URL: https://codereview.chromium.org/1375933003

  Cr-Commit-Position: refs/heads/master@{nodejs#31111}

Ref: nodejs#3036
Ref: nodejs#3481
PR-URL: nodejs#4106
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: rvagg - Rod Vagg <rod@vagg.org>
@misterdjules misterdjules deleted the fix-abort-on-uncaught-exception 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. semver-minor PRs that contain new features and should be released in the next minor version.
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
7 participants