src: adds report_on_fatalerror option to per_process by shobhitchittora · Pull Request #29881 · nodejs/node · GitHub

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

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.

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

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.

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

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?

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.

I think @addaleax should see my review comments above

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

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

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.

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

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

[ 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!

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

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>

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

src: adds report_on_fatalerror option to per_process by shobhitchittora · Pull Request #29881 · nodejs/node · GitHub

Uh oh!

Uh oh!

src: adds report_on_fatalerror option to per_process - #29881

src: adds report_on_fatalerror option to per_process#29881shobhitchittora wants to merge 2 commits intonodejs:masternodejs/node:masterfrom shobhitchittora:fatal-error-diagnostic-reportshobhitchittora/node:fatal-error-diagnostic-reportCopy head branch name to clipboard

Conversation

shobhitchittora commented Oct 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

addaleax Oct 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shobhitchittora Oct 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shobhitchittora commented Oct 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Oct 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gireeshpunathil commented Oct 23, 2019

Uh oh!

shobhitchittora commented Nov 14, 2019

Uh oh!

boneskull commented Nov 15, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Jan 12, 2020

Uh oh!

HarshithaKP commented Feb 3, 2020

Uh oh!

gireeshpunathil commented Mar 11, 2020

Uh oh!

shobhitchittora commented Mar 11, 2020

Uh oh!

gireeshpunathil commented Mar 11, 2020

Uh oh!

cjihrig commented Mar 26, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

src: adds report_on_fatalerror option to per_process#29881
shobhitchittora wants to merge 2 commits into
nodejs:masterfrom
shobhitchittora:fatal-error-diagnostic-report

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