Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 3 commits into from

Conversation

PoojaDurgad
Copy link
Contributor

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

@nodejs-github-bot 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
@addaleax addaleax added the report Issues and PRs related to process.report. label Oct 10, 2020
@addaleax
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

@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

gireeshpunathil added a commit to gireeshpunathil/node that referenced this pull request Oct 15, 2020
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 <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
@gireeshpunathil
Copy link
Member

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

@gireeshpunathil gireeshpunathil added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 5, 2020
@github-actions 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
@github-actions
Copy link
Contributor

github-actions bot commented Nov 5, 2020

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 gpunathi@in.ibm.com
Reviewed-By: Rich Trott rtrott@gmail.com

[detached HEAD 1914f56fd1] doc: add new wordings to the API description
Author: Pooja D.P Pooja.D.P@ibm.com
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 gpunathi@in.ibm.com
Reviewed-By: Rich Trott rtrott@gmail.com

[detached HEAD 1037c4d887] doc: removed trailing whitespaces
Author: Pooja D.P Pooja.D.P@ibm.com
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 gpunathi@in.ibm.com
Reviewed-By: Rich Trott rtrott@gmail.com

[detached HEAD 8df3910379] fixup: address review comments
Author: Pooja D.P Pooja.D.P@ibm.com
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

gireeshpunathil pushed a commit that referenced this pull request Nov 5, 2020
PR-URL: #35588
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@gireeshpunathil
Copy link
Member

landed in 2b985a2

danielleadams pushed a commit that referenced this pull request Nov 9, 2020
PR-URL: #35588
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@danielleadams danielleadams mentioned this pull request Nov 9, 2020
MylesBorins pushed a commit that referenced this pull request Nov 24, 2020
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 <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
BethGriggs pushed a commit that referenced this pull request Dec 9, 2020
PR-URL: #35588
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
PR-URL: #35588
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Dec 10, 2020
BethGriggs pushed a commit that referenced this pull request Dec 15, 2020
PR-URL: #35588
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
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 <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
@targos targos removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Sep 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. process Issues and PRs related to the process subsystem. report Issues and PRs related to process.report.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants