Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: adds report_on_fatalerror option to per_process #29881

Closed
wants to merge 2 commits into from
Closed

src: adds report_on_fatalerror option to per_process #29881

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4cfbc7b
src: adds report_on_fatalerror option to per_process
Oct 7, 2019
decb848
src: removes fatal_error check from per-isolate + adds tests
Oct 8, 2019
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
2 changes: 1 addition & 1 deletion src/node_errors.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -456,7 +456,7 @@ void OnFatalError(const char* location, const char* message) {
#ifdef NODE_REPORT
Isolate* isolate = Isolate::GetCurrent();
Environment* env = Environment::GetCurrent(isolate);
if (env == nullptr || env->isolate_data()->options()->report_on_fatalerror) {
if (per_process::cli_options->report_on_fatalerror) {
report::TriggerNodeReport(
isolate, env, message, "FatalError", "", Local<String>());
}
Expand Down
26 changes: 16 additions & 10 deletions src/node_options.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,15 @@ void PerProcessOptions::CheckOptions(std::vector<std::string>* errors) {
use_largepages != "silent") {
errors->push_back("invalid value for --use-largepages");
}

// #ifdef NODE_REPORT
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do I need to move this check in per-process option checks? Do we still need a dependency on --experimental-report flag being set.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, I'd say so.

Copy link
Member

Choose a reason for hiding this comment

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

Do we still need a dependency on --experimental-report flag being set.

I think that gets a bit tricky here, so I’m fine with omitting it.

// if (report_on_fatalerror) {
// errors->push_back(
// "--report-on-fatalerror option is valid only when "
// "--experimental-report is set");
// }
// #endif // NODE_REPORT

per_isolate->CheckOptions(errors);
}

Expand Down Expand Up @@ -100,12 +109,6 @@ void PerIsolateOptions::CheckOptions(std::vector<std::string>* errors) {
"--experimental-report is set");
}

if (report_on_fatalerror) {
errors->push_back(
"--report-on-fatalerror option is valid only when "
"--experimental-report is set");
}

if (report_on_signal) {
errors->push_back("--report-on-signal option is valid only when "
"--experimental-report is set");
Expand Down Expand Up @@ -622,10 +625,6 @@ PerIsolateOptionsParser::PerIsolateOptionsParser(
"generate diagnostic report upon receiving signals",
&PerIsolateOptions::report_on_signal,
kAllowedInEnvironment);
AddOption("--report-on-fatalerror",
"generate diagnostic report on fatal (internal) errors",
&PerIsolateOptions::report_on_fatalerror,
kAllowedInEnvironment);
AddOption("--report-signal",
"causes diagnostic report to be produced on provided signal,"
" unsupported in Windows. (default: SIGUSR2)",
Expand Down Expand Up @@ -698,6 +697,13 @@ PerProcessOptionsParser::PerProcessOptionsParser(
"print V8 command line options",
&PerProcessOptions::print_v8_help);

#ifdef NODE_REPORT
AddOption("--report-on-fatalerror",
"generate diagnostic report on fatal (internal) errors",
&PerProcessOptions::report_on_fatalerror,
kAllowedInEnvironment);
Copy link
Member

Choose a reason for hiding this comment

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

This is mis-indented (it’s inside a function so it should not be right at the start of the line)

#endif // NODE_REPORT

#ifdef NODE_HAVE_I18N_SUPPORT
AddOption("--icu-data-dir",
"set ICU data load path to dir (overrides NODE_ICU_DATA)"
Expand Down
2 changes: 1 addition & 1 deletion src/node_options.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,6 @@ class PerIsolateOptions : public Options {
#ifdef NODE_REPORT
bool report_uncaught_exception = false;
bool report_on_signal = false;
bool report_on_fatalerror = false;
std::string report_signal;
std::string report_filename;
std::string report_directory;
Expand Down Expand Up @@ -240,6 +239,7 @@ class PerProcessOptions : public Options {

#ifdef NODE_REPORT
std::vector<std::string> cmdline;
bool report_on_fatalerror = false;
Copy link
Member

@addaleax addaleax Oct 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Can you also remove the report_on_fatalerror option from the per-Isolate option list?

Copy link
Contributor Author

@shobhitchittora shobhitchittora Oct 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Is there any other case where we might need it? I left it as I was not clear on that.
@addaleax / @gireeshpunathil

Copy link
Member

Choose a reason for hiding this comment

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

@shobhitchittora I’m not sure what you mean by that – you’ve moved the option to a wider scope, so the original field is now useless and can be removed.

(You do need to update the references in node_report_module.cc as well in order for that to work.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay! Got it.

#endif // NODE_REPORT

inline PerIsolateOptions* get_per_isolate_options();
Expand Down
4 changes: 2 additions & 2 deletions src/node_report_module.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,13 +119,13 @@ static void SetSignal(const FunctionCallbackInfo<Value>& info) {
static void ShouldReportOnFatalError(const FunctionCallbackInfo<Value>& info) {
Environment* env = Environment::GetCurrent(info);
info.GetReturnValue().Set(
env->isolate_data()->options()->report_on_fatalerror);
node::per_process::cli_options->report_on_fatalerror);
Copy link
Member

Choose a reason for hiding this comment

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

I think you should acquire node::per_process::cli_options_mutex in this case, to avoid race conditions when accessing this from multiple threads.

Also, it’s not important, but the previous indentation was the standard one we use for statement continuations (4 spaces).

}

static void SetReportOnFatalError(const FunctionCallbackInfo<Value>& info) {
Environment* env = Environment::GetCurrent(info);
CHECK(info[0]->IsBoolean());
env->isolate_data()->options()->report_on_fatalerror = info[0]->IsTrue();
node::per_process::cli_options->report_on_fatalerror = info[0]->IsTrue();
}

static void ShouldReportOnSignal(const FunctionCallbackInfo<Value>& info) {
Expand Down
37 changes: 23 additions & 14 deletions test/report/test-report-fatal-error.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,18 +21,27 @@ if (process.argv[2] === 'child') {
const helper = require('../common/report.js');
const tmpdir = require('../common/tmpdir');
tmpdir.refresh();
const spawn = require('child_process').spawn;
const args = ['--experimental-report',
'--report-on-fatalerror',
'--max-old-space-size=20',
__filename,
'child'];
const child = spawn(process.execPath, args, { cwd: tmpdir.path });
child.on('exit', common.mustCall((code) => {
assert.notStrictEqual(code, 0, 'Process exited unexpectedly');
const reports = helper.findReports(child.pid, tmpdir.path);
assert.strictEqual(reports.length, 1);
const report = reports[0];
helper.validate(report);
}));
const spawnSync = require('child_process').spawnSync;
let args = ['--experimental-report',
'--report-on-fatalerror',
'--max-old-space-size=20',
__filename,
'child'];

let child = spawnSync(process.execPath, args, { cwd: tmpdir.path });

assert.notStrictEqual(child.status, 0, 'Process exited unexpectedly');
let reports = helper.findReports(child.pid, tmpdir.path);
assert.strictEqual(reports.length, 1);
const report = reports[0];
helper.validate(report);

args = ['--max-old-space-size=20',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI, just using --report-on-fatalerror flag here would generate a report on fatal error without relying on --experimental-report flag.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't expect a report on fatal if the --experimental-report flag was not present

__filename,
'child'];

child = spawnSync(process.execPath, args, { cwd: tmpdir.path });
assert.notStrictEqual(child.status, 0, 'Process exited unexpectedly');
reports = helper.findReports(child.pid, tmpdir.path);
assert.strictEqual(reports.length, 0);
}