Skip to content

Commit

Permalink
src: handle report options on fatalerror
Browse files Browse the repository at this point in the history
Follow on to #32207, 3 other options
are also not respected under situations that the isolate is not
available.

PR-URL: #32497
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
  • Loading branch information
sam-github authored and MylesBorins committed Mar 31, 2020
1 parent 979fb15 commit 04237ec
Show file tree
Hide file tree
Showing 5 changed files with 154 additions and 57 deletions.
28 changes: 14 additions & 14 deletions src/node_options.cc
Original file line number Diff line number Diff line change
Expand Up @@ -582,10 +582,6 @@ PerIsolateOptionsParser::PerIsolateOptionsParser(
"generate diagnostic report on uncaught exceptions",
&PerIsolateOptions::report_uncaught_exception,
kAllowedInEnvironment);
AddOption("--report-compact",
"output compact single-line JSON",
&PerIsolateOptions::report_compact,
kAllowedInEnvironment);
AddOption("--report-on-signal",
"generate diagnostic report upon receiving signals",
&PerIsolateOptions::report_on_signal,
Expand All @@ -596,16 +592,6 @@ PerIsolateOptionsParser::PerIsolateOptionsParser(
&PerIsolateOptions::report_signal,
kAllowedInEnvironment);
Implies("--report-signal", "--report-on-signal");
AddOption("--report-filename",
"define custom report file name."
" (default: YYYYMMDD.HHMMSS.PID.SEQUENCE#.txt)",
&PerIsolateOptions::report_filename,
kAllowedInEnvironment);
AddOption("--report-directory",
"define custom report pathname."
" (default: current working directory of Node.js process)",
&PerIsolateOptions::report_directory,
kAllowedInEnvironment);

Insert(eop, &PerIsolateOptions::get_per_env_options);
}
Expand Down Expand Up @@ -663,6 +649,20 @@ PerProcessOptionsParser::PerProcessOptionsParser(
AddOption("--v8-options",
"print V8 command line options",
&PerProcessOptions::print_v8_help);
AddOption("--report-compact",
"output compact single-line JSON",
&PerProcessOptions::report_compact,
kAllowedInEnvironment);
AddOption("--report-directory",
"define custom report pathname."
" (default: current working directory of Node.js process)",
&PerProcessOptions::report_directory,
kAllowedInEnvironment);
AddOption("--report-filename",
"define custom report file name."
" (default: YYYYMMDD.HHMMSS.PID.SEQUENCE#.txt)",
&PerProcessOptions::report_filename,
kAllowedInEnvironment);
AddOption("--report-on-fatalerror",
"generate diagnostic report on fatal (internal) errors",
&PerProcessOptions::report_on_fatalerror,
Expand Down
6 changes: 3 additions & 3 deletions src/node_options.h
Original file line number Diff line number Diff line change
Expand Up @@ -186,10 +186,7 @@ class PerIsolateOptions : public Options {
bool no_node_snapshot = false;
bool report_uncaught_exception = false;
bool report_on_signal = false;
bool report_compact = false;
std::string report_signal = "SIGUSR2";
std::string report_filename;
std::string report_directory;
inline EnvironmentOptions* get_per_env_options();
void CheckOptions(std::vector<std::string>* errors) override;
};
Expand Down Expand Up @@ -236,6 +233,9 @@ class PerProcessOptions : public Options {
bool trace_sigint = false;
std::vector<std::string> cmdline;
bool report_on_fatalerror = false;
bool report_compact = false;
std::string report_directory;
std::string report_filename;

inline PerIsolateOptions* get_per_isolate_options();
void CheckOptions(std::vector<std::string>* errors) override;
Expand Down
48 changes: 32 additions & 16 deletions src/node_report.cc
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ using node::DiagnosticFilename;
using node::Environment;
using node::Mutex;
using node::NativeSymbolDebuggingContext;
using node::PerIsolateOptions;
using node::TIME_TYPE;
using node::worker::Worker;
using v8::HeapSpaceStatistics;
Expand All @@ -45,6 +44,8 @@ using v8::String;
using v8::V8;
using v8::Value;

namespace per_process = node::per_process;

// Internal/static function declarations
static void WriteNodeReport(Isolate* isolate,
Environment* env,
Expand All @@ -70,29 +71,32 @@ static void PrintCpuInfo(JSONWriter* writer);
static void PrintNetworkInterfaceInfo(JSONWriter* writer);

// External function to trigger a report, writing to file.
// The 'name' parameter is in/out: an input filename is used
// if supplied, and the actual filename is returned.
std::string TriggerNodeReport(Isolate* isolate,
Environment* env,
const char* message,
const char* trigger,
const std::string& name,
Local<String> stackstr) {
std::string filename;
std::shared_ptr<PerIsolateOptions> options;
if (env != nullptr) options = env->isolate_data()->options();

// Determine the required report filename. In order of priority:
// 1) supplied on API 2) configured on startup 3) default generated
if (!name.empty()) {
// Filename was specified as API parameter.
filename = name;
} else if (env != nullptr && options->report_filename.length() > 0) {
// File name was supplied via start-up option.
filename = options->report_filename;
} else {
filename = *DiagnosticFilename(env != nullptr ? env->thread_id() : 0,
"report", "json");
std::string report_filename;
{
Mutex::ScopedLock lock(per_process::cli_options_mutex);
report_filename = per_process::cli_options->report_filename;
}
if (report_filename.length() > 0) {
// File name was supplied via start-up option.
filename = report_filename;
} else {
filename = *DiagnosticFilename(env != nullptr ? env->thread_id() : 0,
"report", "json");
}
}

// Open the report file stream for writing. Supports stdout/err,
Expand All @@ -104,9 +108,14 @@ std::string TriggerNodeReport(Isolate* isolate,
} else if (filename == "stderr") {
outstream = &std::cerr;
} else {
std::string report_directory;
{
Mutex::ScopedLock lock(per_process::cli_options_mutex);
report_directory = per_process::cli_options->report_directory;
}
// Regular file. Append filename to directory path if one was specified
if (env != nullptr && options->report_directory.length() > 0) {
std::string pathname = options->report_directory;
if (report_directory.length() > 0) {
std::string pathname = report_directory;
pathname += node::kPathSeparator;
pathname += filename;
outfile.open(pathname, std::ios::out | std::ios::binary);
Expand All @@ -117,8 +126,8 @@ std::string TriggerNodeReport(Isolate* isolate,
if (!outfile.is_open()) {
std::cerr << "\nFailed to open Node.js report file: " << filename;

if (env != nullptr && options->report_directory.length() > 0)
std::cerr << " directory: " << options->report_directory;
if (report_directory.length() > 0)
std::cerr << " directory: " << report_directory;

std::cerr << " (errno: " << errno << ")" << std::endl;
return "";
Expand All @@ -127,7 +136,11 @@ std::string TriggerNodeReport(Isolate* isolate,
std::cerr << "\nWriting Node.js report to file: " << filename;
}

bool compact = env != nullptr ? options->report_compact : true;
bool compact;
{
Mutex::ScopedLock lock(per_process::cli_options_mutex);
compact = per_process::cli_options->report_compact;
}
WriteNodeReport(isolate, env, message, trigger, filename, *outstream,
stackstr, compact);

Expand All @@ -136,7 +149,10 @@ std::string TriggerNodeReport(Isolate* isolate,
outfile.close();
}

std::cerr << "\nNode.js report completed" << std::endl;
// Do not mix JSON and free-form text on stderr.
if (filename != "stderr") {
std::cerr << "\nNode.js report completed" << std::endl;
}
return filename;
}

Expand Down
19 changes: 12 additions & 7 deletions src/node_report_module.cc
Original file line number Diff line number Diff line change
Expand Up @@ -69,47 +69,52 @@ void GetReport(const FunctionCallbackInfo<Value>& info) {
}

static void GetCompact(const FunctionCallbackInfo<Value>& info) {
Environment* env = Environment::GetCurrent(info);
info.GetReturnValue().Set(env->isolate_data()->options()->report_compact);
node::Mutex::ScopedLock lock(node::per_process::cli_options_mutex);
info.GetReturnValue().Set(node::per_process::cli_options->report_compact);
}

static void SetCompact(const FunctionCallbackInfo<Value>& info) {
node::Mutex::ScopedLock lock(node::per_process::cli_options_mutex);
Environment* env = Environment::GetCurrent(info);
Isolate* isolate = env->isolate();
bool compact = info[0]->ToBoolean(isolate)->Value();
env->isolate_data()->options()->report_compact = compact;
node::per_process::cli_options->report_compact = compact;
}

static void GetDirectory(const FunctionCallbackInfo<Value>& info) {
node::Mutex::ScopedLock lock(node::per_process::cli_options_mutex);
Environment* env = Environment::GetCurrent(info);
std::string directory = env->isolate_data()->options()->report_directory;
std::string directory = node::per_process::cli_options->report_directory;
auto result = String::NewFromUtf8(env->isolate(),
directory.c_str(),
v8::NewStringType::kNormal);
info.GetReturnValue().Set(result.ToLocalChecked());
}

static void SetDirectory(const FunctionCallbackInfo<Value>& info) {
node::Mutex::ScopedLock lock(node::per_process::cli_options_mutex);
Environment* env = Environment::GetCurrent(info);
CHECK(info[0]->IsString());
Utf8Value dir(env->isolate(), info[0].As<String>());
env->isolate_data()->options()->report_directory = *dir;
node::per_process::cli_options->report_directory = *dir;
}

static void GetFilename(const FunctionCallbackInfo<Value>& info) {
node::Mutex::ScopedLock lock(node::per_process::cli_options_mutex);
Environment* env = Environment::GetCurrent(info);
std::string filename = env->isolate_data()->options()->report_filename;
std::string filename = node::per_process::cli_options->report_filename;
auto result = String::NewFromUtf8(env->isolate(),
filename.c_str(),
v8::NewStringType::kNormal);
info.GetReturnValue().Set(result.ToLocalChecked());
}

static void SetFilename(const FunctionCallbackInfo<Value>& info) {
node::Mutex::ScopedLock lock(node::per_process::cli_options_mutex);
Environment* env = Environment::GetCurrent(info);
CHECK(info[0]->IsString());
Utf8Value name(env->isolate(), info[0].As<String>());
env->isolate_data()->options()->report_filename = *name;
node::per_process::cli_options->report_filename = *name;
}

static void GetSignal(const FunctionCallbackInfo<Value>& info) {
Expand Down
110 changes: 93 additions & 17 deletions test/report/test-report-fatal-error.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,14 @@
'use strict';

// Testcases for situations involving fatal errors, like Javascript heap OOM

require('../common');
const assert = require('assert');
// Testcase to produce report on fatal error (javascript heap OOM)
const fs = require('fs');
const helper = require('../common/report.js');
const spawnSync = require('child_process').spawnSync;
const tmpdir = require('../common/tmpdir');

if (process.argv[2] === 'child') {

const list = [];
Expand All @@ -16,30 +22,100 @@ if (process.argv[2] === 'child') {
this.id = 128;
this.account = 98454324;
}
} else {
const helper = require('../common/report.js');
const tmpdir = require('../common/tmpdir');
}

// Common args that will cause an out-of-memory error for child process.
const ARGS = [
'--max-old-space-size=20',
__filename,
'child'
];

{
// Verify that --report-on-fatalerror is respected when set.
tmpdir.refresh();
const spawnSync = require('child_process').spawnSync;
let args = ['--report-on-fatalerror',
'--max-old-space-size=20',
__filename,
'child'];
const args = ['--report-on-fatalerror', ...ARGS];
const child = spawnSync(process.execPath, args, { cwd: tmpdir.path });
assert.notStrictEqual(child.status, 0, 'Process exited unexpectedly');

let child = spawnSync(process.execPath, args, { cwd: tmpdir.path });
const reports = helper.findReports(child.pid, tmpdir.path);
assert.strictEqual(reports.length, 1);

const report = reports[0];
helper.validate(report);

// Errors occur in a context where env is not available, so thread ID is
// unknown. Assert this, to verify that the underlying env-less situation is
// actually reached.
assert.strictEqual(require(report).header.threadId, null);
}

{
// Verify that --report-on-fatalerror is respected when not set.
const args = ARGS;
const child = spawnSync(process.execPath, args, { cwd: tmpdir.path });
assert.notStrictEqual(child.status, 0, 'Process exited unexpectedly');
const reports = helper.findReports(child.pid, tmpdir.path);
assert.strictEqual(reports.length, 0);
}

{
// Verify that --report-directory is respected when set.
// Verify that --report-compact is respected when not set.
tmpdir.refresh();
const dir = '--report-directory=' + tmpdir.path;
const args = ['--report-on-fatalerror', dir, ...ARGS];
const child = spawnSync(process.execPath, args, { });
assert.notStrictEqual(child.status, 0, 'Process exited unexpectedly');
let reports = helper.findReports(child.pid, tmpdir.path);

const reports = helper.findReports(child.pid, tmpdir.path);
assert.strictEqual(reports.length, 1);

const report = reports[0];
helper.validate(report);
assert.strictEqual(require(report).header.threadId, null);
const lines = fs.readFileSync(report, 'utf8').split('\n').length - 1;
assert(lines > 10);
}

{
// Verify that --report-compact is respected when set.
tmpdir.refresh();
const args = ['--report-on-fatalerror', '--report-compact', ...ARGS];
const child = spawnSync(process.execPath, args, { cwd: tmpdir.path });
assert.notStrictEqual(child.status, 0, 'Process exited unexpectedly');

const reports = helper.findReports(child.pid, tmpdir.path);
assert.strictEqual(reports.length, 1);

const report = reports[0];
helper.validate(report);
// Verify that reports are not created on fatal error by default.
args = ['--max-old-space-size=20',
__filename,
'child'];
assert.strictEqual(require(report).header.threadId, null);
// Subtract 1 because "xx\n".split("\n") => [ 'xx', '' ].
const lines = fs.readFileSync(report, 'utf8').split('\n').length - 1;
assert.strictEqual(lines, 1);
}

child = spawnSync(process.execPath, args, { cwd: tmpdir.path });
{
// Verify that --report-compact is respected when set.
// Verify that --report-filename is respected when set.
tmpdir.refresh();
const args = [
'--report-on-fatalerror',
'--report-compact',
'--report-filename=stderr',
...ARGS
];
const child = spawnSync(process.execPath, args, { encoding: 'utf8' });
assert.notStrictEqual(child.status, 0, 'Process exited unexpectedly');
reports = helper.findReports(child.pid, tmpdir.path);

const reports = helper.findReports(child.pid, tmpdir.path);
assert.strictEqual(reports.length, 0);

const lines = child.stderr.split('\n');
// Skip over unavoidable free-form output from V8.
const report = lines[1];
const json = JSON.parse(report);

assert.strictEqual(json.header.threadId, null);
}

0 comments on commit 04237ec

Please sign in to comment.