src: adds report_on_fatalerror option to per_process #29881

shobhitchittora · 2019-10-07T22:50:18Z

Description

moving the report_on_fatalerror flag to per_process as it allows to read it's value on fatal error where env itself is null.

Fixes: #29601

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

addaleax · 2019-10-07T22:52:58Z

src/node_options.h

@@ -231,6 +231,7 @@ class PerProcessOptions : public Options {

 #ifdef NODE_REPORT
  std::vector<std::string> cmdline;
+  bool report_on_fatalerror = false;


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

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

@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.)

Okay! Got it.

shobhitchittora · 2019-10-08T00:12:05Z

@addaleax Just a doubt here. Do the spawned child processes also share the same set of per process flags?
Also since --report-on-fatalerror is dependent on --experimental-report, should it also be moved to the per process level?

addaleax · 2019-10-08T00:24:16Z

@addaleax Just a doubt here. Do the spawned child processes also share the same set of per process flags?

Not necessarily – it depends on whether the child shares its parent’s NODE_OPTIONS environment variable and whether child_process.fork() was used. I don’t think that’s a concern here, though.

Also since --report-on-fatalerror is dependent on --experimental-report, should it also be moved to the per process level?

I think it’s fine to leave that where it is for now – ultimately, it doesn’t matter much, because the experimental flag is going to go away eventually anyway.

shobhitchittora · 2019-10-08T09:46:33Z

src/node_options.cc

@@ -63,6 +63,15 @@ void PerProcessOptions::CheckOptions(std::vector<std::string>* errors) {
                      "used, not both");
  }
 #endif
+
+// #ifdef NODE_REPORT


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

yes, I'd say so.

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.

shobhitchittora · 2019-10-08T09:47:36Z

test/report/test-report-fatal-error.js

+  const report = reports[0];
+  helper.validate(report);
+
+  args = ['--max-old-space-size=20',


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

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

gireeshpunathil · 2019-10-23T07:48:30Z

ping @shobhitchittora .

Also I think this needs to be reconciled / ratified between @addaleax and @boneskull ? i.e. : If --experimental-report is left under per-environment, we still get into a situation where we cannot check its presence on a fatalerror?

shobhitchittora · 2019-11-14T10:59:15Z

Hi all!
Sorry for late reply here. I was traveling for some time and just came back.
@addaleax / @boneskull would like your input on what @gireeshpunathil has mentioned, to move ahead.

boneskull · 2019-11-15T22:53:24Z

I think @addaleax should see my review comments above

addaleax · 2019-11-18T14:24:53Z

src/node_options.cc

+AddOption("--report-on-fatalerror",
+            "generate diagnostic report on fatal (internal) errors",
+            &PerProcessOptions::report_on_fatalerror,
+            kAllowedInEnvironment);            


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

addaleax · 2019-11-18T14:26:08Z

src/node_report_module.cc

@@ -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);


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).

addaleax · 2019-11-18T14:27:06Z

src/node_options.cc

@@ -63,6 +63,15 @@ void PerProcessOptions::CheckOptions(std::vector<std::string>* errors) {
                      "used, not both");
  }
 #endif
+
+// #ifdef NODE_REPORT


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.

BridgeAR · 2020-01-12T01:47:05Z

Ping @shobhitchittora. This needs a rebase and there are a few small comments left.

moving the flag to per_process allows to read it's value on fatal error where env is null Fixes: nodejs#29601

HarshithaKP · 2020-02-03T10:33:09Z

@shobhitchittora - with this change, I am getting a report if I run a program like this: node --max-old-space-size=5 --report-on-fatalerror foo.js - that is, no --experimental-report flag is ON. I believe this is not expected? Pls let me know.

gireeshpunathil · 2020-03-11T07:32:58Z

[ trying to see if we can accommodate this in the upcoming release ]

@shobhitchittora - according to #29881 (comment) , looks like this still has minor work to do. are you planning to progress on this? Also please let me know if you need any help with the missing piece. If you are not in a position to progress, no issues, pls let me know!

shobhitchittora · 2020-03-11T14:07:34Z

@gireeshpunathil I don't think I'd be able to pick this up, due to my busy schedule. Thanks for your patience. Feel free to pick up the commits for your usage.

gireeshpunathil · 2020-03-11T14:14:27Z

thanks @shobhitchittora for the quick response!

--report-on-fatalerror was not honored properly, as there was no way to check the value which was stored in the Environment pointer which can be inaccessible under certain fatal error situations. Move the flag out of Environment pointer so that this is doable. Co-authored-by: Shobhit Chittora schittora@paypal.com Fixes: nodejs#31576 Refs: nodejs#29881

--report-on-fatalerror was not honored properly, as there was no way to check the value which was stored in the Environment pointer which can be inaccessible under certain fatal error situations. Move the flag out of Environment pointer so that this is doable. Co-authored-by: Shobhit Chittora schittora@paypal.com PR-URL: #32207 Fixes: #31576 Refs: #29881 Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

cjihrig · 2020-03-26T02:52:43Z

It seems like this can be closed, given that #32207 landed. Please reopen if that is incorrect. Thanks for the PR.

--report-on-fatalerror was not honored properly, as there was no way to check the value which was stored in the Environment pointer which can be inaccessible under certain fatal error situations. Move the flag out of Environment pointer so that this is doable. Co-authored-by: Shobhit Chittora schittora@paypal.com PR-URL: nodejs#32207 Fixes: nodejs#31576 Refs: nodejs#29881 Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

--report-on-fatalerror was not honored properly, as there was no way to check the value which was stored in the Environment pointer which can be inaccessible under certain fatal error situations. Move the flag out of Environment pointer so that this is doable. Co-authored-by: Shobhit Chittora schittora@paypal.com PR-URL: #32207 Fixes: #31576 Refs: #29881 Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Oct 7, 2019

addaleax reviewed Oct 7, 2019

View reviewed changes

shobhitchittora commented Oct 8, 2019

View reviewed changes

addaleax approved these changes Nov 18, 2019

View reviewed changes

Shobhit Chittora added 2 commits January 12, 2020 10:58

src: adds report_on_fatalerror option to per_process

4cfbc7b

moving the flag to per_process allows to read it's value on fatal error where env is null Fixes: nodejs#29601

src: removes fatal_error check from per-isolate + adds tests

decb848

shobhitchittora force-pushed the fatal-error-diagnostic-report branch from 8845fa4 to decb848 Compare January 12, 2020 05:30

richardlau added the report Issues and PRs related to process.report. label Jan 30, 2020

richardlau mentioned this pull request Jan 30, 2020

Diagnostic report on fatal error does not respect settings #31576

Closed

legendecas mentioned this pull request Mar 11, 2020

Discussion: Criteria for upgrading diagnostic reporting from experimental to stable #26293

Closed

HarshithaKP mentioned this pull request Mar 11, 2020

report: handle on-fatalerror better #32207

Closed

4 tasks

cjihrig closed this Mar 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: adds report_on_fatalerror option to per_process #29881

src: adds report_on_fatalerror option to per_process #29881

shobhitchittora commented Oct 7, 2019 •

edited

Loading

addaleax Oct 7, 2019 •

edited

Loading

shobhitchittora Oct 7, 2019 •

edited

Loading

addaleax Oct 7, 2019

shobhitchittora Oct 7, 2019

shobhitchittora commented Oct 8, 2019 •

edited

Loading

addaleax commented Oct 8, 2019

shobhitchittora Oct 8, 2019

boneskull Oct 8, 2019

addaleax Nov 18, 2019

shobhitchittora Oct 8, 2019

boneskull Oct 8, 2019

gireeshpunathil commented Oct 23, 2019

shobhitchittora commented Nov 14, 2019

boneskull commented Nov 15, 2019

addaleax Nov 18, 2019

addaleax Nov 18, 2019

addaleax Nov 18, 2019

BridgeAR commented Jan 12, 2020

HarshithaKP commented Feb 3, 2020

gireeshpunathil commented Mar 11, 2020

shobhitchittora commented Mar 11, 2020

gireeshpunathil commented Mar 11, 2020

cjihrig commented Mar 26, 2020

src: adds report_on_fatalerror option to per_process #29881

src: adds report_on_fatalerror option to per_process #29881

Conversation

shobhitchittora commented Oct 7, 2019 • edited Loading

Description

Checklist

addaleax Oct 7, 2019 • edited Loading

Choose a reason for hiding this comment

shobhitchittora Oct 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shobhitchittora commented Oct 8, 2019 • edited Loading

addaleax commented Oct 8, 2019

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

gireeshpunathil commented Oct 23, 2019

shobhitchittora commented Nov 14, 2019

boneskull commented Nov 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Jan 12, 2020

HarshithaKP commented Feb 3, 2020

gireeshpunathil commented Mar 11, 2020

shobhitchittora commented Mar 11, 2020

gireeshpunathil commented Mar 11, 2020

cjihrig commented Mar 26, 2020

shobhitchittora commented Oct 7, 2019 •

edited

Loading

addaleax Oct 7, 2019 •

edited

Loading

shobhitchittora Oct 7, 2019 •

edited

Loading

shobhitchittora commented Oct 8, 2019 •

edited

Loading