Add new wordings to the API docs in process.md #35588

PoojaDurgad · 2020-10-10T18:26:22Z

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 · 2020-10-10T23:06:26Z

I was very surprised to see that this is true… This is a bug in the report feature, imo, so I’d be careful about documenting it as if it was intentional behavior. /cc @nodejs/diagnostics

gireeshpunathil · 2020-10-11T11:50:20Z

IMO, the API definition relates to user callbacks on uncaught exceptions, not to any internal mechanisms or other functions that depend on it. IMO, common use case of this API would be to implement custom / additional behavior on exceptional scenarios, not necessarily to avoid / suppress existing behaviors such as report generation.

So I would not call it (the current behavior) a bug.

addaleax · 2020-10-11T12:02:38Z

IMO, common use case of this API would be to implement custom / additional behavior on exceptional scenarios, not necessarily to avoid / suppress existing behaviors such as report generation.

Avoiding and suppressing existing behaviors is exactly what this API was made for, namely, in the context of domains. It is really very surprising that errors caught in domains still generate reports, even though they do not cause uncaught exceptions. I also think that the general expectation that people have here would be that --report-uncaught-exception would correspond to the cases in which the process.on('uncaughtException') event is emitted.

gireeshpunathil · 2020-10-11T12:12:12Z

@addaleax - ok, I see your point. I will investigate how to suppress report generation, if the callback was overriden.

@PoojaDurgad - in which case, the text in this PR would not be necessary, but I suggest to keep it open till that is fixed, and which case this can be modified as something like: actions configured to take place on exceptions such as report generations will be affected too

This api does not alter the behavior of diagnostic report configured on uncaught exceptions. This is deemed as a bug. Honor this API. Refs: nodejs#35588 PR-URL: nodejs#35595 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Richard Lau <[email protected]>

gireeshpunathil · 2020-10-27T05:23:42Z

@PoojaDurgad - the linked PR is landed. So can you pls verify #35588 (comment) ? And then, modify the change text in this PR to reflect the current behavior?

github-actions · 2020-11-05T12:12:35Z

Commit Queue failed

- Loading data for nodejs/node/pull/35588 ✔ Done loading data for nodejs/node/pull/35588 ----------------------------------- PR info ------------------------------------ Title Add new wordings to the API docs in process.md (#35588) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch PoojaDurgad:process-setuncaught -> nodejs:master Labels doc, process, report Commits 3 - doc: add new wordings to the API description - doc: removed trailing whitespaces - fixup: address review comments Committers 1 - Pooja D.P PR-URL: https://github.com/nodejs/node/pull/35588 Reviewed-By: Gireesh Punathil Reviewed-By: Rich Trott ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/35588 Reviewed-By: Gireesh Punathil Reviewed-By: Rich Trott -------------------------------------------------------------------------------- ✔ Last GitHub Actions successful ℹ Doc-only changes ℹ This PR was created on Sat, 10 Oct 2020 18:26:22 GMT ✔ Approvals: 2 ✔ - Gireesh Punathil (@gireeshpunathil) (TSC): https://github.com/nodejs/node/pull/35588#pullrequestreview-521132470 ✔ - Rich Trott (@Trott) (TSC): https://github.com/nodejs/node/pull/35588#pullrequestreview-521182943 -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/master up to date... From https://github.com/nodejs/node * branch master -> FETCH_HEAD ✔ origin/master is now up-to-date - Downloading patch for 35588 From https://github.com/nodejs/node * branch refs/pull/35588/merge -> FETCH_HEAD ✔ Fetched commits as eb24573987b2..93e76bbffc44 -------------------------------------------------------------------------------- Auto-merging doc/api/process.md [master cb6ff981e5] doc: add new wordings to the API description Author: Pooja D.P Date: Sat Oct 10 22:13:10 2020 +0400 1 file changed, 2 insertions(+), 1 deletion(-) Auto-merging doc/api/process.md [master 8f149806ec] doc: removed trailing whitespaces Author: Pooja D.P Date: Sun Oct 11 00:01:11 2020 +0400 1 file changed, 1 insertion(+), 1 deletion(-) Auto-merging doc/api/process.md [master 174e4feefe] fixup: address review comments Author: Pooja D.P Date: Wed Oct 28 08:54:56 2020 +0400 1 file changed, 2 insertions(+), 2 deletions(-) ✔ Patches applied There are 3 commits in the PR. Attempting autorebase. Rebasing (2/6)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
doc: add new wordings to the API description

PR-URL: #35588
Reviewed-By: Gireesh Punathil [email protected]
Reviewed-By: Rich Trott [email protected]

[detached HEAD 1914f56fd1] doc: add new wordings to the API description
Author: Pooja D.P [email protected]
Date: Sat Oct 10 22:13:10 2020 +0400
1 file changed, 2 insertions(+), 1 deletion(-)
Rebasing (3/6)
Rebasing (4/6)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
doc: removed trailing whitespaces

PR-URL: #35588
Reviewed-By: Gireesh Punathil [email protected]
Reviewed-By: Rich Trott [email protected]

[detached HEAD 1037c4d887] doc: removed trailing whitespaces
Author: Pooja D.P [email protected]
Date: Sun Oct 11 00:01:11 2020 +0400
1 file changed, 1 insertion(+), 1 deletion(-)
Rebasing (5/6)
Rebasing (6/6)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
fixup: address review comments

PR-URL: #35588
Reviewed-By: Gireesh Punathil [email protected]
Reviewed-By: Rich Trott [email protected]

[detached HEAD 8df3910379] fixup: address review comments
Author: Pooja D.P [email protected]
Date: Wed Oct 28 08:54:56 2020 +0400
1 file changed, 2 insertions(+), 2 deletions(-)

Successfully rebased and updated refs/heads/master.
✔ 1914f56fd1eea7333de9ab02694cc058650b9859
✔ 0:0 skipping fixes-url fixes-url
✔ 0:0 blank line after title line-after-title
✔ 0:0 line-lengths are valid line-length
✔ 0:0 metadata is at end of message metadata-end
✔ 1:8 PR-URL is valid. pr-url
✔ 0:0 reviewers are valid reviewers
✔ 0:0 valid subsystems subsystem
✔ 0:0 Title is formatted correctly. title-format
✔ 0:0 Title is <= 50 columns. title-length
✔ 1037c4d8877e36978e0d23c7acb9e823616e94d4
✔ 0:0 skipping fixes-url fixes-url
✔ 0:0 blank line after title line-after-title
✔ 0:0 line-lengths are valid line-length
✔ 0:0 metadata is at end of message metadata-end
✔ 1:8 PR-URL is valid. pr-url
✔ 0:0 reviewers are valid reviewers
✔ 0:0 valid subsystems subsystem
✔ 0:0 Title is formatted correctly. title-format
✔ 0:0 Title is <= 50 columns. title-length
✖ 8df3910379d94b244da62a91f53b3ab64cffa8ba
✔ 0:0 skipping fixes-url fixes-url
✔ 0:0 blank line after title line-after-title
✔ 0:0 line-lengths are valid line-length
✔ 0:0 metadata is at end of message metadata-end
✔ 1:8 PR-URL is valid. pr-url
✔ 0:0 reviewers are valid reviewers
✖ 0:0 Invalid subsystem: "fixup" subsystem
✔ 0:0 Title is formatted correctly. title-format
✔ 0:0 Title is <= 50 columns. title-length

ℹ Please fix the commit message and try again.

Commit Queue action: https://github.com/nodejs/node/actions/runs/347440105

PR-URL: #35588 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Rich Trott <[email protected]>

gireeshpunathil · 2020-11-05T12:37:48Z

landed in 2b985a2

PR-URL: #35588 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Rich Trott <[email protected]>

This api does not alter the behavior of diagnostic report configured on uncaught exceptions. This is deemed as a bug. Honor this API. Refs: #35588 PR-URL: #35595 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Richard Lau <[email protected]>

PR-URL: #35588 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Rich Trott <[email protected]>

This api does not alter the behavior of diagnostic report configured on uncaught exceptions. This is deemed as a bug. Honor this API. Refs: nodejs#35588 PR-URL: nodejs#35595 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Richard Lau <[email protected]>

doc: add new wordings to the API description

2e45fe4

nodejs-github-bot added doc Issues and PRs related to the documentations. process Issues and PRs related to the process subsystem. labels Oct 10, 2020

doc: removed trailing whitespaces

878eb7f

addaleax added the report Issues and PRs related to process.report. label Oct 10, 2020

gireeshpunathil mentioned this pull request Oct 11, 2020

report: honor setUncaughtExceptionCaptureCallback #35595

Merged

4 tasks

fixup: address review comments

93e76bb

gireeshpunathil approved these changes Oct 31, 2020

View reviewed changes

Trott approved these changes Nov 1, 2020

View reviewed changes

gireeshpunathil added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 5, 2020

github-actions bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Nov 5, 2020

gireeshpunathil pushed a commit that referenced this pull request Nov 5, 2020

doc: add new wordings to the API description

2b985a2

PR-URL: #35588 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Rich Trott <[email protected]>

gireeshpunathil closed this Nov 5, 2020

danielleadams pushed a commit that referenced this pull request Nov 9, 2020

doc: add new wordings to the API description

8bd364a

PR-URL: #35588 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Rich Trott <[email protected]>

danielleadams mentioned this pull request Nov 9, 2020

v15.2.0 proposal #36055

Merged

BethGriggs pushed a commit that referenced this pull request Dec 9, 2020

doc: add new wordings to the API description

3ccf64c

PR-URL: #35588 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Rich Trott <[email protected]>

BethGriggs pushed a commit that referenced this pull request Dec 10, 2020

doc: add new wordings to the API description

7de8ea7

PR-URL: #35588 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Rich Trott <[email protected]>

BethGriggs mentioned this pull request Dec 10, 2020

v14.15.2 proposal #36476

Merged

BethGriggs pushed a commit that referenced this pull request Dec 15, 2020

doc: add new wordings to the API description

c90571f

PR-URL: #35588 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Rich Trott <[email protected]>

targos removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Sep 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new wordings to the API docs in process.md #35588

Add new wordings to the API docs in process.md #35588

PoojaDurgad commented Oct 10, 2020

addaleax commented Oct 10, 2020

gireeshpunathil commented Oct 11, 2020

addaleax commented Oct 11, 2020

gireeshpunathil commented Oct 11, 2020

gireeshpunathil commented Oct 27, 2020

github-actions bot commented Nov 5, 2020

PR-URL: #35588
Reviewed-By: Gireesh Punathil [email protected]
Reviewed-By: Rich Trott [email protected]

PR-URL: #35588
Reviewed-By: Gireesh Punathil [email protected]
Reviewed-By: Rich Trott [email protected]

PR-URL: #35588
Reviewed-By: Gireesh Punathil [email protected]
Reviewed-By: Rich Trott [email protected]

gireeshpunathil commented Nov 5, 2020

Add new wordings to the API docs in process.md #35588

Add new wordings to the API docs in process.md #35588

Conversation

PoojaDurgad commented Oct 10, 2020

Checklist

addaleax commented Oct 10, 2020

gireeshpunathil commented Oct 11, 2020

addaleax commented Oct 11, 2020

gireeshpunathil commented Oct 11, 2020

gireeshpunathil commented Oct 27, 2020

github-actions bot commented Nov 5, 2020

PR-URL: #35588 Reviewed-By: Gireesh Punathil [email protected] Reviewed-By: Rich Trott [email protected]

PR-URL: #35588 Reviewed-By: Gireesh Punathil [email protected] Reviewed-By: Rich Trott [email protected]

PR-URL: #35588 Reviewed-By: Gireesh Punathil [email protected] Reviewed-By: Rich Trott [email protected]

gireeshpunathil commented Nov 5, 2020

PR-URL: #35588
Reviewed-By: Gireesh Punathil [email protected]
Reviewed-By: Rich Trott [email protected]

PR-URL: #35588
Reviewed-By: Gireesh Punathil [email protected]
Reviewed-By: Rich Trott [email protected]

PR-URL: #35588
Reviewed-By: Gireesh Punathil [email protected]
Reviewed-By: Rich Trott [email protected]