Allow Error object to be passed to node-report #82

hhellyer · 2017-04-24T12:18:23Z

Add an optional parameter to triggerReport and getReport so an
Error object can be passed. If it is passed the error message
and stack trace it contains will be added to the node-report
output in a new "JavaScript Exception Details" section.

This makes node-report more useful in custom error handlers as
it will include the stack trace of the original error as well
as the stack trace where the error was handled.

Add an optional parameter to triggerReport and getReport so an Error object can be passed. If it is passed the error message and stack trace it contains will be added to the node-report output in a new "JavaScript Exception Details" section. This makes node-report more useful in custom error handlers as it will include the stack trace of the original error as well as the stack trace where the error was handled.

Update the API docs for getReport and triggerReport. Fix crash when error object does not include a stack trace.

sam-github · 2017-04-24T17:30:14Z

README.md

@@ -69,6 +69,18 @@ can be specified as a parameter on the `triggerReport()` call.
 nodereport.triggerReport("myReportName");
 ```

+Both triggerReport() and getReport() can take an optional Error object as a parameter. If an Error object is provided the message and stack trace from the  object will be included in the report in the `JavaScript Exception Details` section. When using node-report to handle errors in a callback or an exception handler this allows the report to include the location of the original error as well as where it was handled.


Wrap to 80 columns. markup triggerReport() and getReport() with backticks

I've wrapped Error as well since it's naming a particular type.

sam-github · 2017-04-24T17:32:37Z

src/node_report.cc

@@ -842,13 +871,14 @@ static void PrintStackFrame(std::ostream& out, Isolate* isolate, Local<StackFram
  char buf[64];

  // First print the frame index and the instruction address
+  if( pc != nullptr ) {


this doesn't seem related

When we get the stack frames for the current stack we also call isolate->GetStackSample to get the pc addresses. We can't get those for stack frames that come from an Error we are passed so we pass PrintStackFrame a nullptr for the pc value. (We already pass nullptr if we couldn't get a stack sample for a frame from the current stack.)

This additional check just stops us printing a pc address of 0x0 for every stack frame of the error we are passed and means we can use the same PrintStackFrame function for the current stack and the stack from the Error object.

sam-github · 2017-04-24T17:35:39Z

test/test-api-pass-error.js

+  const spawn = require('child_process').spawn;
+  const tap = require('tap');
+
+  const child = spawn(process.execPath, [__filename, 'child']);


I don't see why the test is spawning a child to call triggerReport(), why not just call it directly in the test? It looks like boilerplate copied from a test that must exist in order to get the report that doesn't apply here. If it is needed, probably needs a comment explaining why.

I believe most of the tests use spawn so the standard validation checks in common.js can check a known set of process arguments. I think @richardlau would be the person to ask if you want the history.

The test is a direct copy of test-api.js with a small change to how the triggerReport() call is made and an extra parameter to the common.validate() function. All the tests work along similar lines so I'll leave it how it is to make sure it fits the current model. If we want to change that we should do it under a separate PR so all the tests are consistent.

Yes, part of the standard validation checks in common.js is to check the command line, so our tests spawn as it's the only way to be certain what to expect.

Although the command line check can be skipped by not setting commandline on the options object passed to validate.

@sam-github Do you want this changed before you approve? I'm happy to raise a separate PR about changing the tests.

sam-github · 2017-04-24T17:37:10Z

Commits should be squashed before merge.

hhellyer · 2017-04-25T08:28:16Z

@rsam - Squash and merge is always my default choice. I'd assume who ever merges this (which might be me) would do the same thing. I don't like to do a force push to squash things down as it can mess things up for the reviewers.
Is there anything I should have put in the PR to indicate I wanted a squash? (That's more of a git etiquette question.)

cjihrig · 2017-04-25T14:18:14Z

README.md

@@ -69,6 +69,25 @@ can be specified as a parameter on the `triggerReport()` call.
 nodereport.triggerReport("myReportName");
 ```

+Both `triggerReport()` and `getReport()` can take an optional `Error` object
+as a parameter. If an `Error` object is provided the message and stack trace


Can you add a comma after "provided"

cjihrig · 2017-04-25T14:18:29Z

README.md

+When using node-report to handle errors in a callback or an exception handler
+this allows the report to include the location of the original error as well
+as where it was handled.
+If both a filename and Error object are passed to `triggerReport()` the


"Error" should be in backticks for consistency.

cjihrig · 2017-04-25T14:18:52Z

README.md

+
+```js
+fs.stat('/usr/local/fake/fake.txt', (err, stats) => {
+  if(err) {


Space after "if"

cjihrig · 2017-04-25T14:19:51Z

src/module.cc

  }
+
+  // We need to pass the javascript object so we can query if for a stack trace.
+  if( info[err_index]->IsNativeError()) {


Space after if. No space after (.

cjihrig · 2017-04-25T14:20:30Z

src/module.cc

@@ -77,7 +86,12 @@ NAN_METHOD(GetReport) {
  v8::Isolate* isolate = info.GetIsolate();
  std::ostringstream out;

-  GetNodeReport(isolate, kJavaScript, "JavaScript API", __func__, out);
+  v8::MaybeLocal<v8::Value> error;
+  if( info[0]->IsNativeError()) {


Same comments here on spacing.

cjihrig · 2017-04-25T14:21:36Z

src/module.cc

@@ -156,7 +170,7 @@ static void OnFatalError(const char* location, const char* message) {
  }
  // Trigger report if requested
  if (nodereport_events & NR_FATALERROR) {
-    TriggerNodeReport(Isolate::GetCurrent(), kFatalError, message, location, nullptr);
+    TriggerNodeReport(Isolate::GetCurrent(), kFatalError, message, location, nullptr, v8::MaybeLocal<v8::Value>());


Does this project wrap lines at 80 characters? If so, there are a number of long lines in this PR.

Of our six outstanding PR's four of them concern code formatting and the conversations got a bit bogged down. After the two outstanding functional PR's are merged I should probably do something like just run clang-format over the C source to standardise it a bit.

At the moment we are well over 80 in node_report.cc as it writes strings of 80 "=" char strings as section dividers in several places.

@cjihrig - Do you want me to push a fix to this or would you rather I dealt with long lines throughout the project in a separate PR?

If the project doesn't enforce 80 character lines, or there are a bunch of other offenders, it's probably simpler to not hold up this PR and deal with them all in another PR. Your call.

There's other places with the same problem so I should probably get the various PRs we have for linting C/C++ and JavaScript tidied up. That way we can do the all code formatting in one patch without muddling them up with functional changes.

cjihrig · 2017-04-25T14:24:46Z

test/common.js

@@ -84,6 +91,10 @@ exports.validateContent = function validateContent(data, t, options) {
            new RegExp('Node.js version: ' + process.version),
            'Node Report header section contains expected Node.js version');
  }
+  if( options && options.expectedException) {


Spacing around if(

cjihrig · 2017-04-25T14:25:08Z

test/test-api-pass-error.js

+  try {
+    throw new Error("Testing error handling");
+  } catch (err) {
+	  nodereport.triggerReport(err);


Indentation looks off here.

@rnchamberlain what happened to #46? It would prevent this.

cjihrig · 2017-04-25T14:25:30Z

test/test-api-pass-error.js

+	  nodereport.triggerReport(err);
+  }
+} else {
+  const common = require('./common.js');


I think this can just be common instead of common.js

I tested, it looks like it can. @sam-github had other comments about the test structure. At the moment it matches all the other tests and if we want to fix one up we should probably do all of them under a separate PR so I'm going to leave this for now.

sam-github · 2017-04-25T14:53:14Z

@hhellyer giving the follow-on commits a commit message made it unclear to me whether they would be squashed or not. I do fixup commits with git commit --fixup HEAD -a, or manually put FIXUP at the start of the commit message to make clear that the commit is supposed to be squashed into the previous.

mhdawson · 2017-04-25T15:15:55Z

src/node_report.cc

@@ -842,13 +871,14 @@ static void PrintStackFrame(std::ostream& out, Isolate* isolate, Local<StackFram
  char buf[64];

  // First print the frame index and the instruction address
+  if( pc != nullptr ) {


Spacing, should be if (pc != nullptr) {

hhellyer · 2017-04-25T15:55:00Z

I did a CI run to confirm these changes worked across all platforms:
https://ci.nodejs.org/view/post-mortem/job/nodereport-continuous-integration/153/
(The failures are issues #83 and #84 and are present when building from master.)

sam-github · 2017-04-25T16:07:13Z

test/test-api-pass-error.js

+    tap.equal(reports.length, 1, 'Found reports ' + reports);
+    const report = reports[0];
+    common.validate(tap, report, {pid: child.pid,
+      commandline: child.spawnargs.join(' '),


I was half convinced by the argument for spawning above, then I read test/common.js, and now I'm not. The command line is just checked against what is passed in here, which could just as easily be process.argv.join(" ") as child.spawnargs.join(" "). Same comment for test/test-api.js, which I suspect was simply copied from the other non-API tests. The non-API tests obviously have to spawn, if you are testing the report generated by a fatal exception you need your child to be the one that dies with the fatal exception, but for API testing, I'm not seeing the need. For that matter, I don't think this test needs its own file. test/test-api.js should be changed to test both variants of triggerReport() and to not spawn children, making it a much more typical API test.

One of the problems we had with process.argv (#58 (comment)) is that Node.js rewrites argv (e.g. it resolves argv[0]). When we spawn we have control over the command line, so we know that if we spawn with a resolved path (e.g. process.execPath) the operating system will match what is reported by Node.js.

But as I pointed out elsewhere the command line check is optional -- We can leave out commandline from the options passed to common.validate and the check will be skipped.

rnchamberlain · 2017-05-02T15:54:15Z

@rsam @hhellyer re the squash/merge thing, for the node-report project we follow the standard Node.js process documented here for landing PRs:
https://github.com/nodejs/node/blob/master/COLLABORATOR_GUIDE.md#landing-pull-requests

So that includes a git rebase -i to squash all the commits for a PR. Maybe I should add that link to the local docs, here: https://github.com/nodejs/node-report/blob/master/MAINTAINER.md

rnchamberlain · 2017-05-03T14:49:05Z

This is really nice, giving the user the exception message, and the stack trace at the point where the exception occurred. The example in the readme (calling fs.stat() on a bogus file) does not actually show a stack trace in the report, presumably because the error handling in fs does not add one (maybe because it's an async call). A better example might be this:

try {
  process.chdir('/foo/foo');
} catch (err) {
  nodereport.triggerReport(err);
}

The report then shows:

================================================================================
==== JavaScript Exception Details ==============================================

Uncaught Error: ENOENT: no such file or directory, uv_chdir
my_listener (/home/rchamberlain/test/api_test.js:18:21)
emitTwo (events.js:106:13)
emit (events.js:194:7)
parserOnIncoming (_http_server.js:565:12)
parserOnHeadersComplete (_http_common.js:99:23)

I think it would be worth adding a blank line in there to separate the exception message from the stack trace.

rnchamberlain · 2017-05-03T15:42:06Z

LGTM

rnchamberlain · 2017-05-04T09:03:17Z

@sam-github @cjihrig is this PR OK now, or more work needed? thanks

sam-github · 2017-05-04T09:22:36Z

I'm in the collab summit, not much time to look at this

The example in the readme (calling fs.stat() on a bogus file) does not actually show a stack trace in the report, presumably because the error handling in fs does not add one

This sounds wrong... README got changed, but are you saying that

fs.stat('-no-such', (err)=> nodereport.triggerReport(err))

does not see a stack? That sounds very fishy... could be because of something about fs, but is fs alone in this? Is it fs.stat specifically? Why is the sync return from process.chdir() different? Would fs.statSync()'s err work better?

I think that should be understood better before merging this. Its possibly a issue in node (and maybe can be fixed/changed there), but it might be that nodereport itself isn't interacting well with node's err objects (how v8 caches stacks so they only get created on demand, for example).

hhellyer · 2017-05-04T09:31:51Z

@rsam - We think it's because the stat happens in native code asynchronously (probably in libuv) then calls the callback with the result or error. There is no JavaScript stack at the point of failure. So we changed to what we hoped was a clearer example.

You can reproduce that in the node repl with:
> fs.stat("/foo/bar", (e,r) => {console.log(e)});

The sync version does produce a stack:
> fs.statSync("/foo/bar");

cjihrig · 2017-05-04T12:05:20Z

src/module.cc

  }
+
+  // We need to pass the javascript object so we can query if for a stack trace.


javascript -> JavaScript and if -> it

cjihrig · 2017-05-04T12:10:55Z

src/node_report.cc

@@ -379,7 +380,7 @@ void SetCommandLine() {
 *   const char* location
 *   char* name - in/out - returns the report filename
 ******************************************************************************/
-void TriggerNodeReport(Isolate* isolate, DumpEvent event, const char* message, const char* location, char* name) {
+void TriggerNodeReport(Isolate* isolate, DumpEvent event, const char* message, const char* location, char* name, v8::MaybeLocal<v8::Value> error) {


The comment before this line documents the parameters. error should probably be added there.

cjihrig · 2017-05-04T12:19:07Z

src/module.cc

@@ -246,7 +260,7 @@ static void SignalDumpAsyncCallback(uv_async_t* handle) {
        fprintf(stdout, "node-report: SignalDumpAsyncCallback triggering NodeReport\n");
      }
      TriggerNodeReport(Isolate::GetCurrent(), kSignal_UV,
-                        node::signo_string(report_signal), __func__, nullptr);
+                        node::signo_string(report_signal), __func__, nullptr, v8::MaybeLocal<v8::Value>());


It would be nice to not have all of the v8::MaybeLocal<v8::Value>()s around.

I've added a using v8::MaybeLocal so at least we can do MaybeLocal<Value>() - we already had a using v8::Value and this does make the new code in node_report.cc more consistent with the existing code.

I checked the Node.js source code to see if there was a better solution but that passes MaybeLocal<Value>() in lots of locations as well.

What about passing nullptr when it's not actually used?

The argument is pass-by-value not a pointer. IIRC You can't create an empty Local<Value> which is why I used MaybeLocal<Value> instead to allow us to pass no value object when there wasn't one available.

Aren't you introducing the argument in this PR though? If so, you could make it a pointer.

I believe from the documentation for Local (and MaybeLocal) in deps/v8.h that Locals should be passed by value. The local is managing the reference to an object owned by V8's garbage collector, I'll admit I haven't dug that far into the implementation details but I've not seen any examples in the Node.js code or other modules where Locals are passed by reference.

In my head I have them in a similar category to std::shared_ptr (and other _ptr's) in C++ where you pass by value something which manages does the actual pointer management for you. I don't believe that they are actually large objects to pass by value, I don't think they really contain fields other than the reference itself.

If I'm wrong I'm happy to change it but I'm a bit wary that passing these by reference is really just a good way to introduce bugs and I'd like to be sure I'm handling these the right way.

I haven't heard that about Locals, but I could be wrong. I did find this though - https://github.com/nodejs/node/blob/6bcf65d4a788a73b3c3f27d75796609f948f7885/src/async-wrap-inl.h#L57

My reference is here:
https://github.com/nodejs/node/blob/master/deps/v8/include/v8.h#L190

I think pass by value is correct and I don't think briefly stack allocating an empty MaybeLocal is remotely expensive so I'm happy with it but also happy to learn the "correct" answer.

(I'm wary of following async-wrap as an example, I suspect it may be special as it probably is quite performance critical which isn't true of node-report code.)

OK, I just talked with @fhinkel from V8 about it and they agreed to not use a reference.

- Correct spelling in comments - Update documentation comment for TriggerNodeReport - v8::MaybeLocal<v8::Value> -> MaybeLocal<Value>

Add an optional parameter to triggerReport and getReport so an Error object can be passed. If it is passed the error message and stack trace it contains will be added to the node-report output in a new "JavaScript Exception Details" section. This makes node-report more useful in custom error handlers as it will include the stack trace of the original error as well as the stack trace where the error was handled. PR-URL: #82 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Richard Chamberlain <[email protected]>

rnchamberlain · 2017-05-09T10:05:02Z

Landed as b20b236

hhellyer added 2 commits April 24, 2017 11:44

Add a test case for passing an Error object to triggerReport

c73968a

Update the API docs for getReport and triggerReport. Fix crash when error object does not include a stack trace.

hhellyer mentioned this pull request Apr 24, 2017

Need to capture exception message in node-report #77

Closed

sam-github suggested changes Apr 24, 2017

View reviewed changes

Wrap text in README.md at 80 characters.

5b75c5e

cjihrig suggested changes Apr 25, 2017

View reviewed changes

mhdawson reviewed Apr 25, 2017

View reviewed changes

Fix formatting and style issues from review.

e5edd5f

sam-github reviewed Apr 25, 2017

View reviewed changes

Fix output spacing and example code comments from code review.

cee0a31

cjihrig suggested changes May 4, 2017

View reviewed changes

Fix code review comments:

7130888

- Correct spelling in comments - Update documentation comment for TriggerNodeReport - v8::MaybeLocal<v8::Value> -> MaybeLocal<Value>

sam-github approved these changes May 5, 2017

View reviewed changes

cjihrig approved these changes May 5, 2017

View reviewed changes

rnchamberlain closed this May 9, 2017

rnchamberlain mentioned this pull request May 12, 2017

Additional information for libuv handles #73

Closed

hhellyer mentioned this pull request May 19, 2017

Node.js 4.8.3 compilation failure #87

Closed

		}

		// We need to pass the javascript object so we can query if for a stack trace.

Allow Error object to be passed to node-report #82

Allow Error object to be passed to node-report #82

Conversation

hhellyer commented Apr 24, 2017

Choose a reason for hiding this comment

hhellyer Apr 25, 2017 • edited Loading

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

sam-github commented Apr 24, 2017

hhellyer commented Apr 25, 2017

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

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

sam-github commented Apr 25, 2017

Choose a reason for hiding this comment

hhellyer commented Apr 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnchamberlain commented May 2, 2017 • edited Loading

rnchamberlain commented May 3, 2017 • edited Loading

rnchamberlain commented May 3, 2017

rnchamberlain commented May 4, 2017

sam-github commented May 4, 2017

hhellyer commented May 4, 2017

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

hhellyer May 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnchamberlain commented May 9, 2017

hhellyer Apr 25, 2017 •

edited

Loading

rnchamberlain commented May 2, 2017 •

edited

Loading

rnchamberlain commented May 3, 2017 •

edited

Loading

hhellyer May 5, 2017 •

edited

Loading