Fix behaviour on exception to match node default #36

rnchamberlain · 2017-01-12T14:47:18Z

This patch switches nodereport behaviour on uncaught exceptions to match default Node behaviour. Previously nodereport was triggering an abort/core dump unless the .setCoreDump() API or the NODEREPORT_COREDUMP env var were used to switch that off.

We now detect whether the user specified the -abort-on-uncaught-exception option and honor that.

Fixes #6

The .setCoreDump() API and NODEREPORT_COREDUMP env var are now redundant and are removed. So this will be a semver major change - and go with proposed rename to "node-report".

Note: on Node v4 and v6, if false is returned from the callback set via isolate->SetAbortOnUncaughtExceptionCallback() - ie. no abort is requested - Node terminates silently. On Node v7 it prints the usual exception message and stack trace. As a workaround for this on v4 and v6, the nodereport code will print the exception stack trace.

This patch switches nodereport behaviour on uncaught exceptions to match node default behaviour. Previously nodereport was triggering an abort/core dump, unless the .setCoreDump() API or the NODEREPORT_COREDUMP env var was used to switch that off. We now detect whether the user specified the -abort-on-uncaught-exception option and honor that. The .setCoreDump() API and NODEREPORT_COREDUMP env var are redundant and are removed. This will be a semver major change.

gdams

LGTM

richardlau

A few questions but generally LGTM.

If we're specifically printing the exception backtrace, should we be checking in the tests that that happens? We may not be able to anticipate the exact contents of the backtrace, but we should at least be able to validate one was produced.

richardlau · 2017-01-12T17:34:00Z

src/module.cc

+  // On Node v4 and v6 we need to print the exception backtrace to stderr, as
+  // Node does not do so (unless we trigger an abort, as above)
+  if ((version_string.find("v4") != std::string::npos) ||
+      (version_string.find("v6") != std::string::npos)) {


version_string potentially has more than just the Node.js version (i.e. It may also contain the versions of dependencies). For now I think this will work, since the Node.js version should normally appear first (and at the moment no other version string contains v but we cannot guarantee that in the future).

Yes that is a bit fragile. Maybe I should limit the search to the first part of the string, or search for "Node.js version: v4" etc. Alternatively I could save the Node version in a separate dedicated variable during the parsing in SetVersionString(), and use that instead.

Is the behavior dependent on Node.js or is it V8 behavior? Could the check be done using V8::GetVersion()?

Good point, it is actually V8 behaviour, so I have changed the check to use V8::GetVersion().

richardlau · 2017-01-12T17:37:41Z

src/module.cc

 }

-#ifndef _WIN32
+#ifdef _WIN32
+static void PrintStackFromStackTrace(Isolate* isolate, FILE* fp) {


Is there any potential overlap (i.e. code-reuse) between this and the similar but not quite identical function in node_report.cc?

Maybe swap the isolate and fp parameters for consistency?

It's a simplified version of the function in node-report.cc. The stack trace in a NodeReport has quite a bit more detail, e.g. the pc value on each line and the JavaScript state value. I did consider sharing the code but I think the resulting single function would have been more complicated. The order of the parameters matches Message::PrintCurrentStackTrace() which is called on the next line in OnUncaughtException()

richardlau · 2017-01-12T18:00:39Z

src/module.cc

+        fprintf(fp, "%s:%i:%i\n", *script_name, line_number, column);
+      } else {
+        fprintf(fp, "%s (%s:%i:%i)\n", *fn_name_s, *script_name, line_number, column);
+      }


PrintStackFrame in node_report.cc also checks frame->IsConstructor() -- Does this need to do the same?

As above, the stack trace in a NodeReport file identifies constructors, but that is not needed in the simpler version we are outputting to stderr.

richardlau · 2017-01-12T18:03:07Z

test/test-exception.js

-    tap.equal(signal, expectedSignal,
-              'Process should exit with expected signal ');
+  child.on('exit', (code) => {
+    tap.plan(2);


We're no longer checking the expected exit code?

Good point, I have added a check for the expected exit code, also a basic check that the top function in the stack trace was there.

Behaviour differences on uncaught exception are down to V8 not Node, so changing check to reflect that. Also improve the exception test to validate the exit code and the stack trace, and throw an Error (which is more representative of real usage).

rnchamberlain · 2017-01-18T10:57:53Z

Added commit in response to the @richardlau questions

richardlau

v8 version check doesn't look right.

Did a quick check with Node.js v4.7.2 on Linux to confirm and the updated test-exception.js fails.

test/test-exception.js .............................. 17/19
  not ok Check for expected process exit code
    +++ found
    --- wanted
    -1
    +0
    compare: ===
    at:
      line: 31
      column: 11
      file: test/test-exception.js
      type: ChildProcess
    stack: |
      ChildProcess.<anonymous> (test/test-exception.js:31:11)
    source: |
      tap.equal(code, 1, 'Check for expected process exit code');

  not ok Check for expected stack trace frame in stderr
    found: |

      Writing Node.js report to file: NodeReport.20170118.141343.83088.001.txt
      Node.js report completed
    pattern: /myException/
    at:
      line: 33
      column: 9
      file: test/test-exception.js
      type: ChildProcess
    stack: |
      ChildProcess.<anonymous> (test/test-exception.js:33:9)
    source: |
      tap.match(stderr, /myException/,

richardlau · 2017-01-18T19:05:48Z

src/module.cc

+  // to stderr, as V8 does not do so (unless we trigger an abort, as above).
+  int v8_major, v8_minor;
+  if (sscanf(v8::V8::GetVersion(), "%d.%d", &v8_major, &v8_minor) == 2) {
+    if (v8_major <= 5 && v8_minor < 4) {


Based on the preceding comment, this check doesn't look correct, e.g. v8 v4.5 (v8_major == 4, v8_minor == 5) would fail the check.

Doh, yes that check is wrong, fixed now, thanks.

richardlau · 2017-01-18T19:11:58Z

test/test-exception.js

-    tap.plan(2);
+    tap.plan(4);
+    const v8_version = (process.versions.v8).match(/\d+/g);
+    if (v8_version[0] <= 5 && v8_version[1] < 4) {


Same as for the equivalent check in the C++ code -- v8 v4.5 would fail this check.

Fixed here as well

This patch switches nodereport behaviour on uncaught exceptions to match node default behaviour. Previously nodereport was triggering an abort/core dump, unless the .setCoreDump() API or the NODEREPORT_COREDUMP env var was used to switch that off. We now detect whether the user specified the -abort-on-uncaught-exception option and honor that. The .setCoreDump() API and NODEREPORT_COREDUMP env var are redundant and are removed. This will be a semver major change. PR-URL: #36 Reviewed-By: George Adams <george.adams@uk.ibm.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>

rnchamberlain · 2017-01-20T11:36:49Z

Landed as 1930ce1

gdams approved these changes Jan 12, 2017

View reviewed changes

richardlau approved these changes Jan 12, 2017

View reviewed changes

richardlau suggested changes Jan 18, 2017

View reviewed changes

Correct v8 version check in module and testcase

5cc64bc

richardlau approved these changes Jan 19, 2017

View reviewed changes

rnchamberlain closed this Jan 20, 2017

rnchamberlain deleted the exception-default branch January 20, 2017 11:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix behaviour on exception to match node default #36

Fix behaviour on exception to match node default #36

rnchamberlain commented Jan 12, 2017

gdams left a comment

richardlau left a comment

richardlau Jan 12, 2017

rnchamberlain Jan 13, 2017

richardlau Jan 13, 2017

rnchamberlain Jan 18, 2017

richardlau Jan 12, 2017

rnchamberlain Jan 18, 2017

richardlau Jan 12, 2017

rnchamberlain Jan 18, 2017

richardlau Jan 12, 2017

rnchamberlain Jan 18, 2017

rnchamberlain commented Jan 18, 2017

richardlau left a comment

richardlau Jan 18, 2017

rnchamberlain Jan 19, 2017

richardlau Jan 18, 2017

rnchamberlain Jan 19, 2017

rnchamberlain commented Jan 20, 2017

Fix behaviour on exception to match node default #36

Fix behaviour on exception to match node default #36

Conversation

rnchamberlain commented Jan 12, 2017

gdams left a comment

Choose a reason for hiding this comment

richardlau left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnchamberlain commented Jan 18, 2017

richardlau left a 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

rnchamberlain commented Jan 20, 2017