From 921d4e027395af5d7fbc557375e8f843bd6efd70 Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Wed, 25 Mar 2020 12:37:39 -0700 Subject: [PATCH] src: handle report options on fatalerror Follow on to https://github.com/nodejs/node/pull/32207, 3 other options are also not respected under situations that the isolate is not available. --- src/node_options.cc | 28 +++---- src/node_options.h | 6 +- src/node_report.cc | 48 +++++++---- src/node_report_module.cc | 19 +++-- test/report/test-report-fatal-error.js | 110 +++++++++++++++++++++---- 5 files changed, 154 insertions(+), 57 deletions(-) diff --git a/src/node_options.cc b/src/node_options.cc index c9276e12fe2e79..4148cfc04eb2ac 100644 --- a/src/node_options.cc +++ b/src/node_options.cc @@ -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, @@ -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); } @@ -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, diff --git a/src/node_options.h b/src/node_options.h index 8c120b0b0a6970..0bca70a424419f 100644 --- a/src/node_options.h +++ b/src/node_options.h @@ -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* errors) override; }; @@ -236,6 +233,9 @@ class PerProcessOptions : public Options { bool trace_sigint = false; std::vector 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* errors) override; diff --git a/src/node_report.cc b/src/node_report.cc index bb703594c9a237..ff2206dbb44ef0 100644 --- a/src/node_report.cc +++ b/src/node_report.cc @@ -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; @@ -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, @@ -70,8 +71,6 @@ 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, @@ -79,20 +78,25 @@ std::string TriggerNodeReport(Isolate* isolate, const std::string& name, Local stackstr) { std::string filename; - std::shared_ptr 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, @@ -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); @@ -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 ""; @@ -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); @@ -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; } diff --git a/src/node_report_module.cc b/src/node_report_module.cc index dfcae53be1313f..a0b54d5e6004f4 100644 --- a/src/node_report_module.cc +++ b/src/node_report_module.cc @@ -69,20 +69,22 @@ void GetReport(const FunctionCallbackInfo& info) { } static void GetCompact(const FunctionCallbackInfo& 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& 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& 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); @@ -90,15 +92,17 @@ static void GetDirectory(const FunctionCallbackInfo& info) { } static void SetDirectory(const FunctionCallbackInfo& 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()); - env->isolate_data()->options()->report_directory = *dir; + node::per_process::cli_options->report_directory = *dir; } static void GetFilename(const FunctionCallbackInfo& 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); @@ -106,10 +110,11 @@ static void GetFilename(const FunctionCallbackInfo& info) { } static void SetFilename(const FunctionCallbackInfo& 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()); - env->isolate_data()->options()->report_filename = *name; + node::per_process::cli_options->report_filename = *name; } static void GetSignal(const FunctionCallbackInfo& info) { diff --git a/test/report/test-report-fatal-error.js b/test/report/test-report-fatal-error.js index 3c93d9c77a755c..72576b40bd4384 100644 --- a/test/report/test-report-fatal-error.js +++ b/test/report/test-report-fatal-error.js @@ -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 = []; @@ -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); }