src: fix --abort-on-uncaught-exception #3036

misterdjules · 2015-09-24T01:33:16Z

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 · 2015-09-24T01:37:58Z

/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.

thefourtheye · 2015-09-24T01:52:09Z

src/async-wrap.cc

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


extra semi-colon at the end.

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 · 2015-09-25T21:41:58Z

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

misterdjules · 2015-09-25T21:43:41Z

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.

bnoordhuis · 2015-09-26T10:58:38Z

deps/v8/include/v8.h

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


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.

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.

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.

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.

thefourtheye · 2015-09-29T01:51:22Z

lib/domain.js

+  var self = this;
+
+  function emitError() {
+    var handled = self.emit('error', er);


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?

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

Created #3118 to discuss this.

@misterdjules Cool. Thanks :-)

misterdjules · 2015-09-30T22:17:54Z

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

davepacheco · 2015-10-02T22:42:22Z

+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 · 2015-10-05T20:31:38Z

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 · 2015-10-08T15:15:01Z

@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 · 2015-10-08T16:07:17Z

@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 · 2015-10-08T16:11:39Z

@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 · 2015-10-08T16:32:16Z

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

misterdjules · 2015-10-16T18:12:58Z

Created https://code.google.com/p/v8/issues/detail?id=4496&thanks=4496&ts=1445018834 and emailed hablich@chromium.org regarding errors when running tools/release/merge_to_branch.py, I'll keep you updated.

misterdjules · 2015-10-19T17:20:46Z

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.

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>

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>

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 · 2015-11-17T00:16:01Z

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

misterdjules · 2015-11-17T17:04:42Z

@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 · 2015-11-17T17:11:23Z

@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 · 2015-11-17T17:49:19Z

@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 · 2015-11-17T18:03:38Z

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

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>

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>

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>

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 added the post-mortem Issues and PRs related to the post-mortem diagnostics of Node.js. label Sep 24, 2015

thefourtheye reviewed Sep 24, 2015
View reviewed changes

This was referenced Sep 24, 2015

src: fix abort-on-uncaught-exception #3038

Closed

node does not abort at the right time when using --abort-on-uncaught-exception #3035

Closed

domains: port fix abort on uncaught to v0.12 nodejs/node-v0.x-archive#25835

Closed

misterdjules force-pushed the fix-abort-on-uncaught-exception branch from 64d4e5d to 53e33df Compare September 25, 2015 21:41

bnoordhuis reviewed Sep 26, 2015
View reviewed changes

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

thefourtheye reviewed Sep 29, 2015
View reviewed changes

misterdjules mentioned this pull request Sep 29, 2015

enabling eslint's prefer-const rule #3118

Closed

misterdjules force-pushed the fix-abort-on-uncaught-exception branch from 76b7c6f to 63d3ec8 Compare October 5, 2015 20:27

misterdjules added the land-on-v4.x label Oct 5, 2015

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

misterdjules mentioned this pull request Oct 30, 2015

Domains inhibit uncaughtException error handling #3607

Closed

MylesBorins removed the land-on-v4.x label Nov 17, 2015

saper mentioned this pull request Jan 6, 2016

Exit status 3221225477 returned from node scripts/build.js / Windows XP sass/node-sass#1283

Closed

rnchamberlain mentioned this pull request Mar 15, 2017

illegal hardware exception nodejs/node-report#6

Closed

misterdjules deleted the fix-abort-on-uncaught-exception branch July 24, 2017 17:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: fix --abort-on-uncaught-exception #3036

src: fix --abort-on-uncaught-exception #3036

misterdjules commented Sep 24, 2015

misterdjules commented Sep 24, 2015

thefourtheye Sep 24, 2015

misterdjules Sep 25, 2015

misterdjules commented Sep 25, 2015

misterdjules commented Sep 25, 2015

bnoordhuis Sep 26, 2015

misterdjules Sep 28, 2015

bnoordhuis Sep 28, 2015

misterdjules Sep 28, 2015

thefourtheye Sep 29, 2015

misterdjules Sep 29, 2015

thefourtheye Sep 29, 2015

misterdjules Sep 29, 2015

thefourtheye Sep 30, 2015

misterdjules commented Sep 30, 2015

davepacheco commented Oct 2, 2015

misterdjules commented Oct 5, 2015

jasnell commented Oct 8, 2015

misterdjules commented Oct 8, 2015

bnoordhuis commented Oct 8, 2015

misterdjules commented Oct 8, 2015

misterdjules commented Oct 16, 2015

misterdjules commented Oct 19, 2015

MylesBorins commented Nov 17, 2015

misterdjules commented Nov 17, 2015

MylesBorins commented Nov 17, 2015

misterdjules commented Nov 17, 2015

MylesBorins commented Nov 17, 2015

src: fix --abort-on-uncaught-exception #3036

src: fix --abort-on-uncaught-exception #3036

Conversation

misterdjules commented Sep 24, 2015

misterdjules commented Sep 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

misterdjules commented Sep 25, 2015

misterdjules commented Sep 25, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

misterdjules commented Sep 30, 2015

davepacheco commented Oct 2, 2015

misterdjules commented Oct 5, 2015

jasnell commented Oct 8, 2015

misterdjules commented Oct 8, 2015

bnoordhuis commented Oct 8, 2015

misterdjules commented Oct 8, 2015

misterdjules commented Oct 16, 2015

misterdjules commented Oct 19, 2015

MylesBorins commented Nov 17, 2015

misterdjules commented Nov 17, 2015

MylesBorins commented Nov 17, 2015

misterdjules commented Nov 17, 2015

MylesBorins commented Nov 17, 2015