GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Add new wordings to the API docs in process.md by PoojaDurgad · Pull Request #35588 · nodejs/node

Conversation

@PoojaDurgad

@PoojaDurgad PoojaDurgad commented Oct 10, 2020

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@PoojaDurgad
doc: add new wordings to the API description
2e45fe4
@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to Node.js documentation. process Issues and PRs related to the process subsystem. labels Oct 10, 2020
@PoojaDurgad
doc: removed trailing whitespaces
878eb7f
@addaleax addaleax added the report Issues and PRs related to Node.js diagnostic reports. label Oct 10, 2020
@addaleax

addaleax commented Oct 10, 2020

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@gireeshpunathil

gireeshpunathil commented Oct 11, 2020

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@addaleax

addaleax commented Oct 11, 2020

Copy link
Copy Markdown
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 and mhdawson reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@gireeshpunathil

gireeshpunathil commented Oct 11, 2020

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

PoojaDurgad reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@gireeshpunathil gireeshpunathil mentioned this pull request Oct 11, 2020
report: honor setUncaughtExceptionCaptureCallback #35595
Merged
2 of 4 tasks
gireeshpunathil added a commit to gireeshpunathil/node that referenced this pull request Oct 15, 2020
@gireeshpunathil
lib: honor setUncaughtExceptionCaptureCallback
ddff2b2
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

gireeshpunathil commented Oct 27, 2020

Copy link
Copy Markdown
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?

PoojaDurgad reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@PoojaDurgad
fixup: address review comments
93e76bb
gireeshpunathil
gireeshpunathil approved these changes Oct 31, 2020
View reviewed changes
Trott
Trott approved these changes Nov 1, 2020
View reviewed changes
@gireeshpunathil gireeshpunathil added the commit-queue PRs queued for automated landing through the Commit Queue. label Nov 5, 2020
@github-actions github-actions Bot added commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. and removed commit-queue PRs queued for automated landing through the Commit Queue. labels Nov 5, 2020
@github-actions

github-actions Bot commented Nov 5, 2020

Copy link
Copy Markdown
Contributor
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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gireeshpunathil pushed a commit that referenced this pull request Nov 5, 2020
@PoojaDurgad @gireeshpunathil
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

gireeshpunathil commented Nov 5, 2020

Copy link
Copy Markdown
Member

landed in 2b985a2

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@gireeshpunathil gireeshpunathil closed this Nov 5, 2020
danielleadams pushed a commit that referenced this pull request Nov 9, 2020
@PoojaDurgad @danielleadams
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 danielleadams mentioned this pull request Nov 9, 2020
v15.2.0 proposal #36055
Merged
MylesBorins pushed a commit that referenced this pull request Nov 24, 2020
@gireeshpunathil @MylesBorins
lib: honor setUncaughtExceptionCaptureCallback
d58a466
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]>
BethGriggs pushed a commit that referenced this pull request Dec 9, 2020
@PoojaDurgad @BethGriggs
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
@PoojaDurgad @BethGriggs
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 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
@PoojaDurgad @BethGriggs
doc: add new wordings to the API description
c90571f
PR-URL: #35588
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
@gireeshpunathil @joesepi
lib: honor setUncaughtExceptionCaptureCallback
8716650
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]>
@targos targos removed the commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. label Sep 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Trott Trott Trott approved these changes
@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

doc Issues and PRs related to Node.js documentation. process Issues and PRs related to the process subsystem. report Issues and PRs related to Node.js diagnostic reports.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@PoojaDurgad @addaleax @gireeshpunathil @Trott @targos @nodejs-github-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.