report: handle on-fatalerror better #32207

HarshithaKP · 2020-03-11T17:37:44Z

--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: #31576
Refs: #29881

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

src/node_report_module.cc

HarshithaKP · 2020-03-12T08:42:12Z

src/node_options.cc

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


This is one extra change I am doing than #29881, to fix the issue mentioned in #29881 (comment)

src/node_options.cc

gireeshpunathil · 2020-03-13T06:15:21Z

This PR complements #32242 (though there will be many conflicts to resolve) , to make it stable with the last known bug being addressed. Request reviews, so that it can land along side before v14 d-cut!

nodejs-github-bot · 2020-03-15T02:25:27Z

CI: https://ci.nodejs.org/job/node-test-pull-request/29841/

cjihrig · 2020-03-15T14:22:07Z

src/node_options.cc

@@ -86,6 +86,11 @@ void PerIsolateOptions::CheckOptions(std::vector<std::string>* errors) {
    return;
  }

+  if (per_process::cli_options->report_on_fatalerror) {


I think we should omit this, as #32242 makes it unnecessary, as previously noted.

@cjihrig, removed this block. PTAL.

cjihrig · 2020-03-15T14:23:27Z

src/node_options.cc

@@ -706,6 +701,13 @@ PerProcessOptionsParser::PerProcessOptionsParser(
            "print V8 command line options",
            &PerProcessOptions::print_v8_help);

+#ifdef NODE_REPORT


Again, #32242 will make the #ifdef NODE_REPORT unnecessary.

Removed #ifdef NODE_REPORT.

cjihrig · 2020-03-15T14:24:53Z

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

-    helper.validate(report);
-  }));
+  const spawnSync = require('child_process').spawnSync;
+  let args = ['--experimental-report',


--experimental-report can be dropped due to #32242.

Removed --experimental-report from args list.

cjihrig · 2020-03-15T14:27:11Z

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

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


Can you add a comment here. Something along the lines of // Verify that reports are not created on fatal error by default.

Added suggested comment in test case. PTAL.

HarshithaKP · 2020-03-16T06:37:24Z

In test/report/test-report-fatal-error.js, linter error saying common is assigned but not used, I couldn't find proper place to use it in test case.

cjihrig · 2020-03-16T13:02:12Z

In test/report/test-report-fatal-error.js, linter error saying common is assigned but not used, I couldn't find proper place to use it in test case.

Instead of const common = require('../common');, you can just do require('../common'); since you aren't using it. You have to require() it though because it has some side effects, and our linting setup checks for its presence.

HarshithaKP · 2020-03-16T13:23:25Z

@cjihrig, thanks. Fixed the error with your suggestion.

sam-github

Looks great to me.

nodejs-github-bot · 2020-03-16T19:22:05Z

CI: https://ci.nodejs.org/job/node-test-pull-request/29873/

sam-github · 2020-03-23T20:40:57Z

@HarshithaKP This looks pretty much ready to land, but it needs a trivial rebase before a final CI. Ping me when you've done it and I'll kick that off.

EDIT: Alternatively, give me permissions and I'll do it: https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/allowing-changes-to-a-pull-request-branch-created-from-a-fork#enabling-repository-maintainer-permissions-on-existing-pull-requests

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

HarshithaKP · 2020-03-24T06:50:30Z

@sam-github, thanks. Rebased it. PTAL.

Overlooked in 2fa74e3. Refs: nodejs#32207

Follow on to nodejs#32207, 3 other options are also not respected under situations that the isolate is not available.

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>

Overlooked in 2fa74e3. Refs: #32207 PR-URL: #32535 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.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: nodejs#32207 Fixes: nodejs#31576 Refs: nodejs#29881 Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

Follow on to nodejs#32207, 3 other options are also not respected under situations that the isolate is not available. PR-URL: nodejs#32497 Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Overlooked in 2fa74e3. Refs: nodejs#32207 PR-URL: nodejs#32535 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.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>

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>

Overlooked in 2fa74e3. Refs: #32207 PR-URL: #32535 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

The property `process.report.reportOnFatalError` was deemed experimental, as it was not honored under certain scenarios (for example out of memory conditions). The report configuration were previously stored on the `environment` structure which was not available on these types of fatal error cases. The referenced PR has addressed this case (sometime back), and the property is working as intended. Refs: nodejs#32207 PR-URL: nodejs#35654 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Beth Griggs <bgriggs@redhat.com>

The property `process.report.reportOnFatalError` was deemed experimental, as it was not honored under certain scenarios (for example out of memory conditions). The report configuration were previously stored on the `environment` structure which was not available on these types of fatal error cases. The referenced PR has addressed this case (sometime back), and the property is working as intended. Refs: #32207 PR-URL: #35654 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Beth Griggs <bgriggs@redhat.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Mar 11, 2020

HarshithaKP changed the title ~~report: handle on-fatalerror better~~ report: handle on-fatalerror better[WIP] Mar 11, 2020

addaleax reviewed Mar 11, 2020

View reviewed changes

src/node_report_module.cc Outdated Show resolved Hide resolved

HarshithaKP force-pushed the diagnostic_report_fatal_error branch from ae976b0 to a0df730 Compare March 12, 2020 08:39

HarshithaKP commented Mar 12, 2020

View reviewed changes

HarshithaKP changed the title ~~report: handle on-fatalerror better[WIP]~~ report: handle on-fatalerror better Mar 12, 2020

HarshithaKP mentioned this pull request Mar 12, 2020

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

Closed

gireeshpunathil mentioned this pull request Mar 13, 2020

report: move diagnostic reports to stable #32242

Merged

4 tasks

gireeshpunathil reviewed Mar 13, 2020

View reviewed changes

src/node_options.cc Outdated Show resolved Hide resolved

gireeshpunathil requested review from addaleax and cjihrig March 15, 2020 05:35

cjihrig reviewed Mar 15, 2020

View reviewed changes

HarshithaKP force-pushed the diagnostic_report_fatal_error branch 2 times, most recently from cc0c963 to 8d71449 Compare March 16, 2020 06:28

gireeshpunathil approved these changes Mar 16, 2020

View reviewed changes

sam-github approved these changes Mar 16, 2020

View reviewed changes

HarshithaKP added 4 commits March 24, 2020 02:39

fixup: address review comment

7915e7e

fixup: fix linter error

3bf8c60

fixup: fix linter error

7dbc5de

HarshithaKP force-pushed the diagnostic_report_fatal_error branch from 5e9fdd9 to 7dbc5de Compare March 24, 2020 06:45

sam-github mentioned this pull request Mar 26, 2020

src: handle report options on fatalerror #32497

Closed

3 tasks

addaleax added a commit to addaleax/node that referenced this pull request Mar 28, 2020

report: add missing locks for report_on_fatalerror accessors

9336772

Overlooked in 2fa74e3. Refs: nodejs#32207

addaleax mentioned this pull request Mar 28, 2020

report: add missing locks for report_on_fatalerror accessors #32535

Closed

2 tasks

sam-github added a commit to sam-github/node that referenced this pull request Mar 30, 2020

src: handle report options on fatalerror

921d4e0

Follow on to nodejs#32207, 3 other options are also not respected under situations that the isolate is not available.

targos added backport-blocked-v12.x and removed backport-blocked-v12.x labels Apr 22, 2020

targos mentioned this pull request May 2, 2020

v12.17.0 release proposal #33197

Merged

gireeshpunathil mentioned this pull request Oct 15, 2020

doc: upgrade stability status of report API #35654

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

report: handle on-fatalerror better #32207

report: handle on-fatalerror better #32207

HarshithaKP commented Mar 11, 2020

HarshithaKP Mar 12, 2020

gireeshpunathil commented Mar 13, 2020

nodejs-github-bot commented Mar 15, 2020

cjihrig Mar 15, 2020

HarshithaKP Mar 16, 2020

cjihrig Mar 15, 2020

HarshithaKP Mar 16, 2020

cjihrig Mar 15, 2020

HarshithaKP Mar 16, 2020

cjihrig Mar 15, 2020

HarshithaKP Mar 16, 2020

HarshithaKP commented Mar 16, 2020

cjihrig commented Mar 16, 2020

HarshithaKP commented Mar 16, 2020

sam-github left a comment

nodejs-github-bot commented Mar 16, 2020

sam-github commented Mar 23, 2020 •

edited

HarshithaKP commented Mar 24, 2020

report: handle on-fatalerror better #32207

report: handle on-fatalerror better #32207

Conversation

HarshithaKP commented Mar 11, 2020

Checklist

Choose a reason for hiding this comment

gireeshpunathil commented Mar 13, 2020

nodejs-github-bot commented Mar 15, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HarshithaKP commented Mar 16, 2020

cjihrig commented Mar 16, 2020

HarshithaKP commented Mar 16, 2020

sam-github left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Mar 16, 2020

sam-github commented Mar 23, 2020 • edited

HarshithaKP commented Mar 24, 2020

sam-github commented Mar 23, 2020 •

edited