-
Notifications
You must be signed in to change notification settings - Fork 7.3k
domains: fix issues with abort on uncaught #8666
Conversation
82a8d75
to
fd61ee3
Compare
Tested on Linux, MacOS, SmartOS and Windows. Initial pre-review done by @tjfontaine, cc @trevnorris @dap. |
Sorry @dap, I meant @davepacheco |
Wow, this is a massive piece of work. Thanks for all the effort. There are some issues. Like, we're not going to float a V8 patch for better domain support. Even more so since this is for the v0.10 branch. |
I think we need to float this patch, as it is today, people who are using domains heavily, but also want to be able to use We're obviously not going to be able to upstream this particular change to V8 as it's not the most appropriate, it is fairly reasonable for the V0.10 branch. The upstream change to V8 is probably to remove the need for V8 to know about |
@geek would you like to comment on this? |
@trevnorris without this patch you have to take your pick between post mortem debugging with core files and being able to use domains. |
@tjfontaine @geek While this might fix the individual issue for v0.10, don't forget it still won't work for all cases in v0.12. V8's Promises will swallow errors, not allowing them to propagate to a domain. |
@trevnorris good point. @tjfontaine can we get this merged for 0.10.34!?!? |
@tjfontaine or @trevnorris can this get merged soon? |
This LGTM for v0.10, and floating a patch seems like a necessary evil given the nature of the problem ("domains or postmortem debugging, pick one"). My questions:
In short, I could see this problem blocking v0.12; but this patch (assuming we're not planning on upgrading v0.10's V8 in the future) seems like a sane solution. |
Node already uses |
Additional info from a chat on IRC:
I.E. There's no reason to add anything to V8. |
Re: To summarize the behaviors we're examining (please point out any mistakes you see here!): The current behavior is: Any time an exception propagates to the top of the JS stack, the process aborts. If there are This patch's behavior: Domains will be run if active, The proposed fix from IRC, as I understand it, is as follows: Check for
This approach's behavior: If domains are active, they will be run. If the last domain emits an unhandled error, the process will abort. EDIT:
|
There are unfortunately two issues being solved by this fix, mostly because it was difficult to separate them. I will enumerate the two problems such that's more clear what's happening. Problem 1: code explicitly Problem 2: if the last error handler in a stack of domains The two problems are not actually related, to clarify what we're trying to solve for in problem 1 is that code that is not covered by a domain in a process that is run with While testing it, it's easiest to achieve that by simply throwing in the last error handler in the stack, which is a case that is clearly broken by re-throwing the wrong error object. For people doing post-mortem debugging (mdb, lldb, gdb -- whatever) it's not as important, because what's important for them is that the stack is preserved not about what error object is preserved. However for those who may hit this issue, where the last domain handler is throwing, and you're trying to debug without post-mortem help, it becomes difficult to understand the behavior because you're seeing the wrong error output on stderr. As a concrete example of this -- try debugging the crypto domain test transient failure that I fixed in: 2afa3d8 without also having this change. |
Also, to be clear, the The point of aborting here is to preserve as much of the "crime scene" as possible, where debugging your application is actually much harder than simply looking at the Error object's message and stack trace. |
Here's a fugly patch that covers all the cases I can think of, and doesn't require V8 to be patched: https://gist.github.com/trevnorris/be1dee9d3fb4bc467b1c EDIT: I was lazy about it, and there are two tests that fail but doesn't look like anything that can't be worked out. |
There are two kinds of exception handling that can exist in the V8 world.
Because you can have nested exception handling, v8 keeps track of this by inspecting the call stack to decide if an exception "can be caught" and what type of exception handler is being used to catch it. When
The purpose of So currently if the most recent try catch handler pushed onto the call stack is an external try catch, v8 will trigger the
What we want to happen is for v8 to only There are only two ways to try and handle this:
The first solution is attractive, which is what your change does, but is not sufficient. Consider that the code may call into C++, and that code may put an external handler on the stack. If the code beyond that then throws, it will also The only way to guarantee this for all code paths, with the least amount of impact for those involved, is to modify v8 such that it's asking the embedder if it should abort. |
I just pushed a new commit that does two things:
I haven't squashed it yet so that we can review this addition separately. I will be glad to squash these commits into one if we feel that they should both be landed. |
dca26a2
to
7dfc50c
Compare
@@ -1534,7 +1546,8 @@ Isolate::Isolate() | |||
date_cache_(NULL), | |||
context_exit_happened_(false), | |||
deferred_handles_head_(NULL), | |||
optimizing_compiler_thread_(this) { | |||
optimizing_compiler_thread_(this), | |||
abort_on_uncaught_exception_callback_(0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: change to NULL
.
Do not abort the process if an error is thrown from within a domain, an error handler is setup for the domain and --abort-on-uncaught-exception was passed on the command line. However, if an error is thrown from within the top-level domain's error handler and --abort-on-uncaught-exception was passed on the command line, make the process abort. Fixes nodejs#8631. Also fixes nodejs#8630.
7dfc50c
to
fa0a8f1
Compare
@trevnorris Updated the changes according to your comments, thanks again! |
Add v8::Isolate::SetAbortOnUncaughtException() so the user can be notified when an uncaught exception has bubbled. PR-URL: #8666 Reviewed-by: Trevor Norris <trev.norris@gmail.com>
Do not abort the process if an error is thrown from within a domain, an error handler is setup for the domain and --abort-on-uncaught-exception was passed on the command line. However, if an error is thrown from within the top-level domain's error handler and --abort-on-uncaught-exception was passed on the command line, make the process abort. Fixes: #8631 Fixes: #8630 PR-URL: #8666 Reviewed-by: Trevor Norris <trev.norris@gmail.com>
Add v8::Isolate::SetAbortOnUncaughtException() so the user can be notified when an uncaught exception has bubbled. PR-URL: joyent#8666 Reviewed-by: Trevor Norris <trev.norris@gmail.com> Port fbff705 to deps/v8ppc and deps/v8z
Add v8::Isolate::SetAbortOnUncaughtException() so the user can be notified when an uncaught exception has bubbled. PR-URL: nodejs#8666 Reviewed-by: Trevor Norris <trev.norris@gmail.com>
Do not abort the process if an error is thrown from within a domain, an error handler is setup for the domain and --abort-on-uncaught-exception was passed on the command line. However, if an error is thrown from within the top-level domain's error handler and --abort-on-uncaught-exception was passed on the command line, make the process abort. Fixes: nodejs#8631 Fixes: nodejs#8630 PR-URL: nodejs#8666 Reviewed-by: Trevor Norris <trev.norris@gmail.com>
Add v8::Isolate::SetAbortOnUncaughtException() so the user can be notified when an uncaught exception has bubbled. PR-URL: nodejs/node-v0.x-archive#8666 Reviewed-by: Trevor Norris <trev.norris@gmail.com> Signed-off-by: Jeroen Ooms <jeroenooms@gmail.com>
Do not abort the process if an error is thrown from within a domain, an
error handler is setup for the domain and --abort-on-uncaught-exception
was passed on the command line.
However, if an error is thrown from within the top-level domain's error
handler and --abort-on-uncaught-exception was passed on the command
line, make the process abort.
Fixes #8631. Also fixes #8630.