node.cc: break on uncaught exceptions #5713

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
3 participants

bajtos commented Jun 17, 2013

All TryCatch blocks used in node.cc are now verbose, so that V8 debugger
can break on uncaught exceptions.

See comment in deps/v8/include/v8.h for explanation:

By default, exceptions that are caught by an external exception
handler are not reported. Call SetVerbose with true on an
external exception handler to have exceptions caught by the
handler reported as if they were not caught.

The flag is used by Isolate::ShouldReportException(), which is called
by Isolate::DoThrow() to decide whether an exception is considered uncaught.

/cc: @bnoordhuis

Thank you for contributing this pull request! Here are a few pointers to make sure your submission will be considered for inclusion.

Commit bajtos/node@8b7cfe0 has the following error(s):

  • Commit message line too long: 12

You can fix all these things without opening another issue.

Please see CONTRIBUTING.md for more information

bajtos commented Jun 17, 2013

@bnoordhuis I have submitted a first draft for review. I am not happy with duplicating SetVerbose(true) all over the place.

Perhaps I could add a lightweight descendant of TryCatch called NodeTryCatch that will call SetVerbose(true) in the constructor?

Any better idea?

Owner

bnoordhuis commented Jun 17, 2013

Perhaps I could add a lightweight descendant of TryCatch called NodeTryCatch that will call SetVerbose(true) in the constructor?

That doesn't really bother me. There are only a few TryCatch call sites (you missed one in src/node_script.cc by the way) and there should only be fewer over time.

I'm not sure what the repercussions are of this change but I guess we can land it in master and try it out. Is this the final version of your PR?

bajtos commented Jun 18, 2013

Perhaps I could add a lightweight descendant of TryCatch called NodeTryCatch that will call SetVerbose(true) in the constructor?

That doesn't really bother me. There are only a few TryCatch call sites (you missed one in src/node_script.cc by the way) and there should only be fewer over time.

Our usual disagreement about coding style, heh? My concern is that it was already easy to miss one of the six places required to be changed, so I'd rather make sure the next person doing a similar change can avoid this risk.

Anyway, you are the boss.

I'm not sure what the repercussions are of this change but I guess we can land it in master and try it out.

Yeah, I am not entirely sure either. My understanding is that this change affects only Isolate::DoThrow() - see the condition

// Generate the message if required.
if (report_exception || try_catch_needs_message) {

Since try_catch_need_message is true by default, the condition is fullfilled regardless of report_exception (that was false before this change and it is true now). Hopefully this means that my change does not introduce any performance regression when running without a debugger.

Is this the final version of your PR?

I added the missing bit in src/node_script.cc and rebased on master's HEAD, the PR is ready for merge now.

Owner

bnoordhuis commented Jun 18, 2013

My concern is that it was already easy to miss one of the six places required to be changed, so I'd rather make sure the next person doing a similar change can avoid this risk.

It's nothing git grep -w TryCatch src/ won't catch.

This PR breaks a number of tests (see here.) The tests that break locally for me all seem to involve child processes, oddly enough. Might just be coincidence.

bajtos commented Jun 18, 2013

This PR breaks a number of tests.

Oh well. There are two additional lines printed for unhandled exception now. Apparently the verbose flag is used on other places too.

E.g. for test/message/timeout_throw.js

Old output:

/Users/bajtos/src/node/test/message/timeout_throw.js:26
  undefined_reference_error_maker;
  ^
ReferenceError: undefined_reference_error_maker is not defined
    at null._onTimeout (/Users/bajtos/src/node/test/message/timeout_throw.js:26:3)
    at Timer.listOnTimeout [as ontimeout] (timers.js:105:15)

New output:

/Users/bajtos/src/node/test/message/timeout_throw.js:1290: Uncaught ReferenceError: undefined_reference_error_maker is not defined

/Users/bajtos/src/node/test/message/timeout_throw.js:26
  undefined_reference_error_maker;
  ^
ReferenceError: undefined_reference_error_maker is not defined
    at null._onTimeout (/Users/bajtos/src/node/test/message/timeout_throw.js:26:3)
    at Timer.listOnTimeout [as ontimeout] (timers.js:105:15)

The line-numer 1290 is weird, since timeout_throw.js has only 27 lines.

bajtos commented Jun 18, 2013

It's V8 reporting the exception to stdout/stderr, I guess that was the original intention behind SetVerbose flag.

See deps/v8/src/isolate.cc:1392:

// Save the message for reporting if the the exception remains uncaught.
thread_local_top()->has_pending_message_ = report_exception;

I am not sure what is the correct solution here.

  • Supress logging of uncaught exceptions in V8, since we are logging them ourselves? (But then are we sure the only uncaught exceptions are those coming from node.js code?)
  • Add yet another flag to TryCatch to distinguish between "this exception should be considered as uncaught by debugger" and "this exception should be logged to stdout/stderr"?
  • Any other options?

What are your thoughts, Ben?

@bajtos bajtos node.cc: break on uncaught exception
Most TryCatch blocks have SetVerbose flag on, this tells V8 to report
uncaught exceptions to debugger.

FatalException handler is called from V8 Message listener instead from
the place where TryCatch was used. Otherwise uncaught exceptions are logged
twice.

See comment in `deps/v8/include/v8.h` for explanation of SetVerbose flag:
>  By default, exceptions that are caught by an external exception
>  handler are not reported.  Call SetVerbose with true on an
>  external exception handler to have exceptions caught by the
>  handler reported as if they were not caught.

The flag is used by `Isolate::ShouldReportException()`, which is called
by `Isolate::DoThrow()` to decide whether an exception is considered
uncaught.
218d4c5

@bnoordhuis bnoordhuis commented on an outdated diff Jun 25, 2013

@@ -1131,7 +1127,7 @@ ssize_t DecodeWrite(char *buf,
return StringBytes::Write(buf, buflen, val, encoding, NULL);
}
-void DisplayExceptionLine (TryCatch &try_catch) {
+void DisplayExceptionLine (Handle<Message> message) {
@bnoordhuis

bnoordhuis Jun 25, 2013

Owner

While you're here... style: no space between function name and parenthesis.

@bnoordhuis bnoordhuis commented on an outdated diff Jun 25, 2013

- String::Utf8Value trace(try_catch.StackTrace());
+ Local<Value> traceValue(er->ToObject()->Get(String::New("stack")));
@bnoordhuis

bnoordhuis Jun 25, 2013

Owner

We predominantly use snake_case rather than camelCase in C++ code.

@bnoordhuis bnoordhuis commented on an outdated diff Jun 25, 2013

exit(8);
}
}
+void FatalException(TryCatch &try_catch) {
@bnoordhuis

bnoordhuis Jun 25, 2013

Owner

Style: TryCatch& try_catch

@bnoordhuis bnoordhuis commented on an outdated diff Jun 25, 2013

@@ -2416,10 +2434,14 @@ void Load(Handle<Object> process_l) {
TryCatch try_catch;
+ // try_catch must be not verbose to disable FatalException() handler
+ // Load exceptions cannot be ignored (handled) by _fatalException
@bnoordhuis

bnoordhuis Jun 25, 2013

Owner

This comment reads a bit ambiguous. I assume you mean something like "Disable verbose mode to stop FatalException() from trying to handle the exception. Errors this early in the start-up phase are not safe to ignore."

@bnoordhuis bnoordhuis commented on an outdated diff Jun 25, 2013

@@ -2445,11 +2467,15 @@ void Load(Handle<Object> process_l) {
InitPerfCounters(global);
#endif
- f->Call(global, 1, args);
+ // Enable handling of uncaught exceptions
+ // (FatalException(), break on uncaught exception in debugger)
+ //
+ // This is not strictly necessary since it's almost impossible
+ // to attach debugger fast enought to break on exception
@bnoordhuis

bnoordhuis Jun 25, 2013

Owner

s/debugger/a debugger/ (or 'the debugger')

@bnoordhuis bnoordhuis commented on an outdated diff Jun 25, 2013

src/node_script.cc
@@ -411,7 +415,10 @@ void WrappedScript::Initialize(Handle<Object> target) {
: Script::New(code, filename);
if (script.IsEmpty()) {
// FIXME UGLY HACK TO DISPLAY SYNTAX ERRORS.
- if (display_error) DisplayExceptionLine(try_catch);
+ if (display_error) {
+ HandleScope scope(node_isolate);
@bnoordhuis

bnoordhuis Jun 25, 2013

Owner

Why is the HandleScope necessary?

@bnoordhuis bnoordhuis commented on an outdated diff Jun 25, 2013

test/fixtures/uncaught-exceptions/domain.js
@@ -0,0 +1,13 @@
+var domain = require('domain');
+
+var d = domain.create();
+d.on('error', function(err) {
+ console.log('[ignored]', err.stack);
+});
+
+d.run(function() {
+ setImmediate(function() {
+ throw new Error('in domain');
+ });
+});
+
@bnoordhuis

bnoordhuis Jun 25, 2013

Owner

Trailing whitespace.

@bnoordhuis bnoordhuis commented on an outdated diff Jun 25, 2013

TryCatch fatal_try_catch;
+ // Do not call FatalException when _fatalException handler throws
+ fatal_try_catch.SetVerbose(false);
+
// this will return true if the JS layer handled it, false otherwise
Local<Value> caught = fatal_f->Call(process, ARRAY_SIZE(argv), argv);
@bnoordhuis

bnoordhuis Jun 25, 2013

Owner

You could change this to fatal_f->Call(process, 1, &error); and drop argv.

@bajtos bajtos fixup! code cleanup
Fixed issues pointed out in the code review.
1ebae5c

bajtos commented Jun 26, 2013

Thanks for the review, I addressed the issues in 1ebae5c.

@bnoordhuis: What's your opinion on FatalException() checking if TryCatch is verbose?

The current state is this:

  • Existing native modules have is_verbose_ false, so they are not affected.
  • The person adding SetVerbose(true) should be aware that he must not call FatalException
  • FatalException is called twice only if the person above makes a mistake and calls both SetVerbose(true) and FatalException on error.

Given that, is it worth the effort (and extra code) to detect such mistakes and do not log the error second time? Does it happen often that a native module adds it's own TryCatch block?

Owner

bnoordhuis commented Jun 26, 2013

Landed in c16963b, thanks.

Does it happen often that a native module adds it's own TryCatch block?

Depends on your definition of 'often'. It's not common but it's not extremely rare either.

Given that, is it worth the effort (and extra code) to detect such mistakes and do not log the error second time?

Probably not. Let's take the reactive approach and not do anything until someone complains.

bnoordhuis closed this Jun 26, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment