Skip to content
This repository was archived by the owner on Jun 18, 2021. It is now read-only.

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

Closed
wants to merge 6 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
15fd45a
Allow Error object to be passed to node-report
hhellyer Mar 29, 2017
c73968a
Add a test case for passing an Error object to triggerReport
hhellyer Apr 21, 2017
5b75c5e
Wrap text in README.md at 80 characters.
hhellyer Apr 25, 2017
e5edd5f
Fix formatting and style issues from review.
hhellyer Apr 25, 2017
cee0a31
Fix output spacing and example code comments from code review.
hhellyer May 3, 2017
7130888
Fix code review comments:
hhellyer May 4, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,26 @@ 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.
If both a filename and `Error` object are passed to `triggerReport()` the
`Error` object should be the second parameter.

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

## Configuration

Additional configuration is available using the following APIs:
Expand Down
26 changes: 20 additions & 6 deletions src/module.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ NAN_METHOD(TriggerReport) {
Nan::HandleScope scope;
v8::Isolate* isolate = info.GetIsolate();
char filename[NR_MAXNAME + 1] = "";
MaybeLocal<Value> error;
int err_index = 0;

if (info[0]->IsString()) {
// Filename parameter supplied
Expand All @@ -60,9 +62,16 @@ NAN_METHOD(TriggerReport) {
} else {
Nan::ThrowError("node-report: filename parameter is too long");
}
err_index++;
}

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

if (nodereport_events & NR_APICALL) {
TriggerNodeReport(isolate, kJavaScript, "JavaScript API", __func__, filename);
TriggerNodeReport(isolate, kJavaScript, "JavaScript API", __func__, filename, error);
// Return value is the report filename
info.GetReturnValue().Set(Nan::New(filename).ToLocalChecked());
}
Expand All @@ -77,7 +86,12 @@ NAN_METHOD(GetReport) {
v8::Isolate* isolate = info.GetIsolate();
std::ostringstream out;

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

GetNodeReport(isolate, kJavaScript, "JavaScript API", __func__, error, out);
// Return value is the contents of a report as a string.
info.GetReturnValue().Set(Nan::New(out.str()).ToLocalChecked());
}
Expand Down Expand Up @@ -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, MaybeLocal<Value>());
}
fflush(stderr);
raise(SIGABRT);
Expand All @@ -165,7 +179,7 @@ static void OnFatalError(const char* location, const char* message) {
bool OnUncaughtException(v8::Isolate* isolate) {
// Trigger report if requested
if (nodereport_events & NR_EXCEPTION) {
TriggerNodeReport(isolate, kException, "exception", __func__, nullptr);
TriggerNodeReport(isolate, kException, "exception", __func__, nullptr, MaybeLocal<Value>());
}
if ((commandline_string.find("abort-on-uncaught-exception") != std::string::npos) ||
(commandline_string.find("abort_on_uncaught_exception") != std::string::npos)) {
Expand Down Expand Up @@ -231,7 +245,7 @@ static void SignalDumpInterruptCallback(Isolate* isolate, void* data) {
fprintf(stdout, "node-report: SignalDumpInterruptCallback triggering report\n");
}
TriggerNodeReport(isolate, kSignal_JS,
node::signo_string(report_signal), __func__, nullptr);
node::signo_string(report_signal), __func__, nullptr, MaybeLocal<Value>());
}
report_signal = 0;
}
Expand All @@ -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, MaybeLocal<Value>());
}
report_signal = 0;
}
Expand Down
51 changes: 41 additions & 10 deletions src/node_report.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,10 +82,11 @@ typedef struct tm TIME_TYPE;
#endif

// Internal/static function declarations
static void WriteNodeReport(Isolate* isolate, DumpEvent event, const char* message, const char* location, char* filename, std::ostream &out, TIME_TYPE* time);
static void WriteNodeReport(Isolate* isolate, DumpEvent event, const char* message, const char* location, char* filename, std::ostream &out, MaybeLocal<Value> error, TIME_TYPE* time);
static void PrintCommandLine(std::ostream& out);
static void PrintVersionInformation(std::ostream& out);
static void PrintJavaScriptStack(std::ostream& out, Isolate* isolate, DumpEvent event, const char* location);
static void PrintJavaScriptErrorStack(std::ostream& out, Isolate* isolate, MaybeLocal<Value> error);
static void PrintStackFromStackTrace(std::ostream& out, Isolate* isolate, DumpEvent event);
static void PrintStackFrame(std::ostream& out, Isolate* isolate, Local<StackFrame> frame, int index, void* pc);
static void PrintNativeStack(std::ostream& out);
Expand Down Expand Up @@ -378,8 +379,9 @@ void SetCommandLine() {
* const char* message
* const char* location
* char* name - in/out - returns the report filename
* MaybeLocal<Value> error - JavaScript Error object.
******************************************************************************/
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, MaybeLocal<Value> error) {
// Recursion check for report in progress, bail out
if (report_active) return;
report_active = true;
Expand Down Expand Up @@ -460,7 +462,7 @@ void TriggerNodeReport(Isolate* isolate, DumpEvent event, const char* message, c
// Pass our stream about by reference, not by copying it.
std::ostream &out = outfile.is_open() ? outfile : *outstream;

WriteNodeReport(isolate, event, message, location, filename, out, &tm_struct);
WriteNodeReport(isolate, event, message, location, filename, out, error, &tm_struct);

// Do not close stdout/stderr, only close files we opened.
if(outfile.is_open()) {
Expand All @@ -474,7 +476,7 @@ void TriggerNodeReport(Isolate* isolate, DumpEvent event, const char* message, c

}

void GetNodeReport(Isolate* isolate, DumpEvent event, const char* message, const char* location, std::ostream& out) {
void GetNodeReport(Isolate* isolate, DumpEvent event, const char* message, const char* location, MaybeLocal<Value> error, std::ostream& out) {
// Obtain the current time and the pid (platform dependent)
TIME_TYPE tm_struct;
#ifdef _WIN32
Expand All @@ -484,7 +486,7 @@ void GetNodeReport(Isolate* isolate, DumpEvent event, const char* message, const
gettimeofday(&time_val, nullptr);
localtime_r(&time_val.tv_sec, &tm_struct);
#endif
WriteNodeReport(isolate, event, message, location, nullptr, out, &tm_struct);
WriteNodeReport(isolate, event, message, location, nullptr, out, error, &tm_struct);
}

static void walkHandle(uv_handle_t* h, void* arg) {
Expand Down Expand Up @@ -531,7 +533,7 @@ static void walkHandle(uv_handle_t* h, void* arg) {
*out << buf;
}

static void WriteNodeReport(Isolate* isolate, DumpEvent event, const char* message, const char* location, char* filename, std::ostream &out, TIME_TYPE* tm_struct) {
static void WriteNodeReport(Isolate* isolate, DumpEvent event, const char* message, const char* location, char* filename, std::ostream &out, MaybeLocal<Value> error, TIME_TYPE* tm_struct) {

#ifdef _WIN32
DWORD pid = GetCurrentProcessId();
Expand Down Expand Up @@ -589,6 +591,11 @@ static void WriteNodeReport(Isolate* isolate, DumpEvent event, const char* messa
PrintNativeStack(out);
out << std::flush;

// Print the stack trace and message from the Error object.
// (If one was provided.)
PrintJavaScriptErrorStack(out, isolate, error);
out << std::flush;

// Print V8 Heap and Garbage Collector information
PrintGCStatistics(out, isolate);
out << std::flush;
Expand Down Expand Up @@ -791,6 +798,29 @@ static void PrintJavaScriptStack(std::ostream& out, Isolate* isolate, DumpEvent
#endif
}

static void PrintJavaScriptErrorStack(std::ostream& out, Isolate* isolate, MaybeLocal<Value> error) {
if (error.IsEmpty() || !error.ToLocalChecked()->IsNativeError()) {
return;
}

out << "\n================================================================================";
out << "\n==== JavaScript Exception Details ==============================================\n\n";
Local<Message> message = v8::Exception::CreateMessage(isolate, error.ToLocalChecked());
Nan::Utf8String message_str(message->Get());

out << *message_str << "\n\n";

Local<StackTrace> stack = v8::Exception::GetStackTrace(error.ToLocalChecked());
if (stack.IsEmpty()) {
out << "No stack trace available from Exception::GetStackTrace()\n";
return;
}
// Print the stack trace, samples are not available as the exception isn't from the current stack.
for (int i = 0; i < stack->GetFrameCount(); i++) {
PrintStackFrame(out, isolate, stack->GetFrame(i), i, nullptr);
}
}

/*******************************************************************************
* Function to print stack using GetStackSample() and StackTrace::StackTrace()
*
Expand Down Expand Up @@ -842,13 +872,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) {
#ifdef _WIN32
snprintf( buf, sizeof(buf), "%2d: [pc=0x%p] ", i, pc);
out << buf;
snprintf( buf, sizeof(buf), "%2d: [pc=0x%p] ", i, pc);
#else
snprintf( buf, sizeof(buf), "%2d: [pc=%p] ", i, pc);
out << buf;
snprintf( buf, sizeof(buf), "%2d: [pc=%p] ", i, pc);
#endif
out << buf;
}

// Now print the JavaScript function name and source information
if (frame->IsEval()) {
Expand Down
5 changes: 3 additions & 2 deletions src/node_report.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ using v8::String;
using v8::Value;
using v8::StackTrace;
using v8::StackFrame;
using v8::MaybeLocal;

// Bit-flags for node-report trigger options
#define NR_EXCEPTION 0x01
Expand All @@ -32,8 +33,8 @@ using v8::StackFrame;

enum DumpEvent {kException, kFatalError, kSignal_JS, kSignal_UV, kJavaScript};

void TriggerNodeReport(Isolate* isolate, DumpEvent event, const char* message, const char* location, char* name);
void GetNodeReport(Isolate* isolate, DumpEvent event, const char* message, const char* location, std::ostream& out);
void TriggerNodeReport(Isolate* isolate, DumpEvent event, const char* message, const char* location, char* name, v8::MaybeLocal<v8::Value> error);
void GetNodeReport(Isolate* isolate, DumpEvent event, const char* message, const char* location, v8::MaybeLocal<v8::Value> error, std::ostream& out);

unsigned int ProcessNodeReportEvents(const char* args);
unsigned int ProcessNodeReportSignal(const char* args);
Expand Down
13 changes: 12 additions & 1 deletion test/common.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,11 +58,18 @@ exports.validateContent = function validateContent(data, t, options) {
const expectedVersions = options ?
options.expectedVersions || nodeComponents :
nodeComponents;
var plan = REPORT_SECTIONS.length + nodeComponents.length + 5;
const expectedException = options.expectedException;
if (options.expectedException) {
REPORT_SECTIONS.push('JavaScript Exception Details');
}

let plan = REPORT_SECTIONS.length + nodeComponents.length + 5;
if (options.commandline) plan++;
if (options.expectedException) plan++;
const glibcRE = /\(glibc:\s([\d.]+)/;
const nodeReportSection = getSection(reportContents, 'Node Report');
const sysInfoSection = getSection(reportContents, 'System Information');
const exceptionSection = getSection(reportContents, 'JavaScript Exception Details');
const libcPath = getLibcPath(sysInfoSection);
const libcVersion = getLibcVersion(libcPath);
if (glibcRE.test(nodeReportSection) && libcVersion) plan++;
Expand All @@ -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) {
t.match(exceptionSection, new RegExp('Uncaught Error: ' + options.expectedException),
'Node Report JavaScript Exception contains expected message');
}
if (options && options.commandline) {
if (this.isWindows()) {
// On Windows we need to strip double quotes from the command line in
Expand Down
29 changes: 29 additions & 0 deletions test/test-api-pass-error.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
'use strict';

// Testcase for passing an error object to the API call.

if (process.argv[2] === 'child') {
const nodereport = require('../');
try {
throw new Error("Testing error handling");
} catch (err) {
nodereport.triggerReport(err);
}
} else {
const common = require('./common.js');
const spawn = require('child_process').spawn;
const tap = require('tap');

const child = spawn(process.execPath, [__filename, 'child']);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

child.on('exit', (code) => {
tap.plan(3);
tap.equal(code, 0, 'Process exited cleanly');
const reports = common.findReports(child.pid);
tap.equal(reports.length, 1, 'Found reports ' + reports);
const report = reports[0];
common.validate(tap, report, {pid: child.pid,
commandline: child.spawnargs.join(' '),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

expectedException: "Testing error handling",
});
});
}