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 [email protected]

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

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

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

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

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

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 [email protected]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 <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

Overlooked in 2fa74e3. Refs: #32207 PR-URL: #32535 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>

--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 [email protected] PR-URL: nodejs#32207Fixes: nodejs#31576 Refs: nodejs#29881 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Sam Roberts <[email protected]>

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 <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

Overlooked in 2fa74e3. Refs: nodejs#32207 PR-URL: nodejs#32535 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>

--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 [email protected] PR-URL: #32207Fixes: #31576 Refs: #29881 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Sam Roberts <[email protected]>

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 <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

Overlooked in 2fa74e3. Refs: #32207 PR-URL: #32535 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>

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 <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Beth Griggs <[email protected]>

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 <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Beth Griggs <[email protected]>

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

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

HarshithaKP commented Mar 12, 2020
View reviewed changes

src/node_options.cc Outdated
Copy link
MemberAuthor
HarshithaKPMar 12, 2020
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is one extra change I am doing than #29881, to fix the issue mentioned in #29881 (comment)

HarshithaKP changed the title ~~report: handle on-fatalerror better[WIP]~~report: handle on-fatalerror betterMar 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 resolvedHide 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 8d71449Compare 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 7dbc5deCompare 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

Uh oh!

report: handle on-fatalerror better#32207

report: handle on-fatalerror better #32207

Uh oh!

Conversation

HarshithaKP commented Mar 11, 2020

Checklist

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gireeshpunathil commented Mar 13, 2020

Uh oh!

nodejs-github-bot commented Mar 15, 2020

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!

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!

HarshithaKP commented Mar 16, 2020

Uh oh!

cjihrig commented Mar 16, 2020

Uh oh!

HarshithaKP commented Mar 16, 2020

Uh oh!

sam-github left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Mar 16, 2020

Uh oh!

sam-github commented Mar 23, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HarshithaKP commented Mar 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

sam-github commented Mar 23, 2020•
edited
Loading