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 — process: allow monitoring uncaughtException by Flarna · Pull Request #31257 · nodejs/node

Conversation

@Flarna

@Flarna Flarna commented Jan 8, 2020 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Installing an uncaughtException listener has a side effect that process won't exit. This is quite bad for monitoring/logging tools which tend to be interested in errors but don't want to cause side effects like swallow an exception or change the output on console.

There are some workarounds in the wild like monkey patching emit or rethrow in the exception if monitoring tool detects that it is the only listener but this is error prone and risky.

This PR allows to install a listener to monitor uncaughtException without the side effect to consider the exception has handled.

Refs: #30932

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 and v1rtl reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@nodejs-github-bot nodejs-github-bot added the process Issues and PRs related to the process subsystem. label Jan 8, 2020
@Flarna
Flarna force-pushed the uncaughtExceptionMonitor branch from 18704d4 to 85d940c Compare January 8, 2020 09:25
@Flarna
process: allow monitoring uncaughtException
9039128 
Installing an uncaughtException listener has a side effect that process
is not aborted. This is quite bad for monitoring/logging tools which
tend to be interested in errors but don't want to cause side effects
like swallow an exception or change the output on console.

There are some workarounds in the wild like monkey patching emit or
rethrow in the exception if monitoring tool detects that it is the only
listener but this is error prone and risky.

This PR allows to install a listener to monitor uncaughtException
without the side effect to consider the exception has handled. To avoid
conflicts with other events it exports a symbol on process which owns
this special meaning.
@Flarna
Flarna force-pushed the uncaughtExceptionMonitor branch from 85d940c to 9039128 Compare January 8, 2020 09:40
BridgeAR
BridgeAR reviewed Jan 8, 2020
View reviewed changes
Comment thread test/parallel/test-process-uncaught-exception-monitor.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread lib/internal/process/execution.js Resolved

Uh oh!

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

Flarna commented Jan 8, 2020

Copy link
Copy Markdown
Member Author

I tried also to add a test which verifies that process still exits if no uncaughtException listener is installed but failed. Is there anywhere a sample/test which verifies something like this?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Flarna
fixup! crlf => lf
df03daa
@BridgeAR

BridgeAR commented Jan 8, 2020

Copy link
Copy Markdown
Member

If you only want to check the stack, you could write a message test similar to e.g., test/message/events_unhandled_error_common_trace.js. Otherwise it's possible to maybe use the on('exit') hook or a child process.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Flarna
use arrow functions
b58dd7c
BridgeAR
BridgeAR approved these changes Jan 8, 2020
View reviewed changes
@Flarna
add test to verify default exit behavior is unchanged
6f53695
BridgeAR
BridgeAR reviewed Jan 8, 2020
View reviewed changes
Comment thread test/fixtures/uncaught-exceptions/uncaught-monitor.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread test/fixtures/uncaught-exceptions/uncaught-monitor.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@Flarna
fixup! improve fixture
acee3de
joyeecheung
joyeecheung reviewed Jan 8, 2020
View reviewed changes
Comment thread lib/internal/process/execution.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
patilharshal16
patilharshal16 approved these changes Jan 9, 2020
View reviewed changes
addaleax
addaleax reviewed Jan 9, 2020
View reviewed changes

@addaleax addaleax left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code LGTM, although it feels like this issue isn’t really specific to uncaught exceptions – maybe there should be a general way of attaching event listeners to emitters that doesn’t affect the return value of .emit()?

Also, it feels like monkey-patching process.emit to intercept uncaught exceptions would actually be just fine and should be considered supported by Node.js.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread doc/api/process.md Resolved

Uh oh!

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

Flarna commented Jan 9, 2020

Copy link
Copy Markdown
Member Author

The code LGTM, although it feels like this issue isn’t really specific to uncaught exceptions – maybe there should be a general way of attaching event listeners to emitters that doesn’t affect the return value of .emit()?

That sounds interessting. The guaranteed sequence of first calling monitoring listeners could be also implemented.
But besides the return value of emit() also APIs like emitter.listeners() and maybe even emitter.removeAllListeners() needs to handle such listeners in a special way.
In case of uncaughtException there is process.setUncaughtExceptionCaptureCallback() which avoids that uncaughtException is emitted at all.

Also, it feels like monkey-patching process.emit to intercept uncaught exceptions would actually be just fine and should be considered supported by Node.js.

In general I would avoid monkey patching if easily possible. There might be serveral users patching the same api and some try to unpatch which ends up in a mess if done in the wrong order.
Patching process.emit means also that inheritance tree is changed and users may call the wrong original function. I know these are quite corner cases but I got used to corner cases...
Another option is monkey patching process._fatalException but I assume this will not increase the happiness looking into the comments around this function.

Another idea would be to have some more generic fatal error event which signals also other types of errors like OOM to allow sync reporting. But I fear it may be not possible to call JS functions in a save way at this time.
BridgeAR 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.
bnoordhuis
bnoordhuis reviewed Jan 9, 2020
View reviewed changes

@bnoordhuis bnoordhuis left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a regression test that ensures an uncaughtExceptionMonitor listener that itself throws an exception terminates the process? (I.e., doesn't result in an infinite loop or anything like that.)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@Flarna

Flarna commented Jan 10, 2020 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

@bnoordhuis sure.

But thinking once more about this I'm not sure if we should add a try/catch around this specific emit(uncaughtExceptionMonitor) and swallow any exceptions from these listeners.
In general throwing in a listener is a bad idea and most of the time it ends up in an uncaught exception. This is different here as there was already an uncaught exception. Throwing in a monitoring listener will result in ending the process no calling regular uncaughtException handlers nor an UncaughtExceptionCaptureCallback.

Update: Added a test. I have not added a try catch as an exception within the fatal error handling should result in exit code 7 and this is the case now.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Flarna added 2 commits January 10, 2020 09:09
@Flarna
replace symbol uncaughtExceptionMonitor by a string
5daab4d
@Flarna
add test to verify that throwing in monitoring listener results in
c3b5f2d 
exit code 7
bnoordhuis
bnoordhuis approved these changes Jan 10, 2020
View reviewed changes

@bnoordhuis bnoordhuis left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a suggestion.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/fixtures/uncaught-exceptions/uncaught-monitor2.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
addaleax
addaleax approved these changes Jan 10, 2020
View reviewed changes
@nodejs-github-bot

nodejs-github-bot commented Jan 10, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/28352/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@BridgeAR BridgeAR added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Jan 10, 2020
cjihrig
cjihrig approved these changes Jan 10, 2020
View reviewed changes
Comment thread doc/api/process.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread doc/api/process.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread doc/api/process.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
Flarna and others added 3 commits January 10, 2020 19:38
@Flarna @cjihrig
Update doc/api/process.md
9db2da6 
Co-Authored-By: Colin Ihrig <[email protected]>
@Flarna @cjihrig
Update doc/api/process.md
4206027 
Co-Authored-By: Colin Ihrig <[email protected]>
@Flarna @cjihrig
Update doc/api/process.md
efedbb5 
Co-Authored-By: Colin Ihrig <[email protected]>
Trott
Trott approved these changes Jan 10, 2020
View reviewed changes
@Trott

Trott commented Jan 10, 2020 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

The initial commit message still mentions the symbol, but that has been removed, right? Whoever lands this should update the commit message (unless @Flarna does it before that time and force-pushes).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott Trott added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 11, 2020
@Trott

Trott commented Jan 11, 2020

Copy link
Copy Markdown
Member

Landed in f4797ff
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott Trott closed this Jan 11, 2020
Trott pushed a commit that referenced this pull request Jan 11, 2020
@Flarna @Trott
process: allow monitoring uncaughtException
f4797ff 
Installing an uncaughtException listener has a side effect that process
is not aborted. This is quite bad for monitoring/logging tools which
tend to be interested in errors but don't want to cause side effects
like swallow an exception or change the output on console.

There are some workarounds in the wild like monkey patching emit or
rethrow in the exception if monitoring tool detects that it is the only
listener but this is error prone and risky.

This PR allows to install a listener to monitor uncaughtException
without the side effect to consider the exception has handled.

PR-URL: #31257
Refs: #30932
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@Trott Trott added the notable-change PRs with changes that should be highlighted in changelogs. label Jan 11, 2020
@Flarna
Flarna deleted the uncaughtExceptionMonitor branch January 11, 2020 07:17
MylesBorins pushed a commit that referenced this pull request Jan 16, 2020
@Flarna @MylesBorins
process: allow monitoring uncaughtException
79eba6a 
Installing an uncaughtException listener has a side effect that process
is not aborted. This is quite bad for monitoring/logging tools which
tend to be interested in errors but don't want to cause side effects
like swallow an exception or change the output on console.

There are some workarounds in the wild like monkey patching emit or
rethrow in the exception if monitoring tool detects that it is the only
listener but this is error prone and risky.

This PR allows to install a listener to monitor uncaughtException
without the side effect to consider the exception has handled.

PR-URL: #31257
Refs: #30932
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
codebytere added a commit that referenced this pull request Jan 16, 2020
@codebytere
2020-01-21, Version 13.7.0 (Current)
20b7365 
Notable changes:

* **deps**:
  * upgrade npm to 6.13.6 (Ruy Adorno) [#31304](#31304)
* **doc**:
  * add GeoffreyBooth to collaborators (Geoffrey Booth) [#31306](#31306)
* **process**:
  * allow monitoring uncaughtException (Gerhard Stoebich) [#31257](#31257)
@codebytere codebytere mentioned this pull request Jan 16, 2020
v13.7.0 proposal #31382
Merged
codebytere added a commit that referenced this pull request Jan 17, 2020
@codebytere
2020-01-21, Version 13.7.0 (Current)
a283d37 
Notable changes:

* **deps**:
  * upgrade npm to 6.13.6 (Ruy Adorno) [#31304](#31304)
* **doc**:
  * add GeoffreyBooth to collaborators (Geoffrey Booth) [#31306](#31306)
* **process**:
  * allow monitoring uncaughtException (Gerhard Stoebich) [#31257](#31257)
codebytere added a commit to codebytere/node that referenced this pull request Jan 19, 2020
@codebytere
2020-01-21, Version 13.7.0 (Current)
d116035 
Notable changes:

* **deps**:
  * upgrade npm to 6.13.6 (Ruy Adorno) [nodejs#31304](nodejs#31304)
* **doc**:
  * add GeoffreyBooth to collaborators (Geoffrey Booth) [nodejs#31306](nodejs#31306)
* **process**:
  * allow monitoring uncaughtException (Gerhard Stoebich) [nodejs#31257](nodejs#31257)

PR-URL: nodejs#31382
codebytere added a commit that referenced this pull request Jan 20, 2020
@codebytere
2020-01-21, Version 13.7.0 (Current)
d1e8e6f 
Notable changes:

* deps:
  * upgrade to libuv 1.34.1 (cjihrig) #31332
  * upgrade npm to 6.13.6 (Ruy Adorno) #31304
* doc:
  * add GeoffreyBooth to collaborators (Geoffrey Booth) #31306
* module
  * add API for interacting with source maps (bcoe) #31132
  * loader getSource, getFormat, transform hooks (Geoffrey Booth) #30986
  * logical conditional exports ordering (Guy Bedford) #31008
  * unflag conditional exports (Guy Bedford) #31001
* process:
  * allow monitoring uncaughtException (Gerhard Stoebich) #31257

PR-URL: #31382
@mmarchini

mmarchini commented Jan 20, 2020

Copy link
Copy Markdown
Contributor

@Flarna thank you for working on this! It seems like the right direction to monitor uncaught exceptions on third-party monitoring tools.

Just a minor nit on the PR description: Installing an uncaughtException listener has a side effect that process is not aborted, when I first read it, I thought this PR was referring to --abort-on-uncaught-exception. Do you mind changing the description to Installing an uncaughtException listener has a side effect that process won't exit to avoid confusion when others read it?
All reactions

Sorry, something went wrong.

Uh oh!

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

Flarna commented Jan 20, 2020

Copy link
Copy Markdown
Member Author

@mmarchini Done, I removed also the final sentence that a symbol has been added as this was removed during the review phase.
mmarchini reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
codebytere added a commit that referenced this pull request Jan 21, 2020
@codebytere
2020-01-21, Version 13.7.0 (Current)
bbab963 
Notable changes:

* deps:
  * upgrade to libuv 1.34.1 (cjihrig) #31332
  * upgrade npm to 6.13.6 (Ruy Adorno) #31304
* module
  * add API for interacting with source maps (bcoe) #31132
  * loader getSource, getFormat, transform hooks (Geoffrey Booth) #30986
  * logical conditional exports ordering (Guy Bedford) #31008
  * unflag conditional exports (Guy Bedford) #31001
* process:
  * allow monitoring uncaughtException (Gerhard Stoebich) #31257
* Added new collaborators:
  * [GeoffreyBooth](https://github.com/GeoffreyBooth) - Geoffrey Booth. #31306

PR-URL: #31382
codebytere added a commit that referenced this pull request Jan 21, 2020
@codebytere
2020-01-21, Version 13.7.0 (Current)
a484f1a 
Notable changes:

* deps:
  * upgrade to libuv 1.34.1 (cjihrig) #31332
  * upgrade npm to 6.13.6 (Ruy Adorno) #31304
* module
  * add API for interacting with source maps (bcoe) #31132
  * loader getSource, getFormat, transform hooks (Geoffrey Booth) #30986
  * logical conditional exports ordering (Guy Bedford) #31008
  * unflag conditional exports (Guy Bedford) #31001
* process:
  * allow monitoring uncaughtException (Gerhard Stoebich) #31257
* Added new collaborators:
  * [GeoffreyBooth](https://github.com/GeoffreyBooth) - Geoffrey Booth. #31306

PR-URL: #31382
@targos targos removed the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Apr 25, 2020
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
@Flarna @targos
process: allow monitoring uncaughtException
689a74c 
Installing an uncaughtException listener has a side effect that process
is not aborted. This is quite bad for monitoring/logging tools which
tend to be interested in errors but don't want to cause side effects
like swallow an exception or change the output on console.

There are some workarounds in the wild like monkey patching emit or
rethrow in the exception if monitoring tool detects that it is the only
listener but this is error prone and risky.

This PR allows to install a listener to monitor uncaughtException
without the side effect to consider the exception has handled.

PR-URL: nodejs#31257
Refs: nodejs#30932
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit that referenced this pull request Apr 28, 2020
@Flarna @targos
process: allow monitoring uncaughtException
de3603f 
Installing an uncaughtException listener has a side effect that process
is not aborted. This is quite bad for monitoring/logging tools which
tend to be interested in errors but don't want to cause side effects
like swallow an exception or change the output on console.

There are some workarounds in the wild like monkey patching emit or
rethrow in the exception if monitoring tool detects that it is the only
listener but this is error prone and risky.

This PR allows to install a listener to monitor uncaughtException
without the side effect to consider the exception has handled.

PR-URL: #31257
Refs: #30932
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@targos targos mentioned this pull request May 2, 2020
v12.17.0 release proposal #33197
Merged
@Linkgoron Linkgoron mentioned this pull request Apr 19, 2021
if uncaughtExceptionMonitor handler throws, the process dies without chance for recovery by uncaughtException or uncaughtRejection #38280
Closed
@coderabbitai coderabbitai Bot mentioned this pull request Jul 24, 2026
Add runtime-aware error reporting SDKs jaeyeol-yee/dolshoe#2
Merged
This was referenced Aug 1, 2026
feat: record telemetry exception events chrisdoc/hevy-mcp#839
Merged
chore(repo): 유지보수 하드닝 — 크래시 가드·로그 retention·즉시 stats 푸시·next 범프·회귀 스펙 ldhbenecia/cairn#364
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joyeecheung joyeecheung joyeecheung left review comments
@BridgeAR BridgeAR BridgeAR approved these changes
@bnoordhuis bnoordhuis bnoordhuis approved these changes
@Trott Trott Trott approved these changes
@addaleax addaleax addaleax approved these changes
@cjihrig cjihrig cjihrig approved these changes
+1 more reviewer
@patilharshal16 patilharshal16 patilharshal16 approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

No one assigned

Labels

notable-change PRs with changes that should be highlighted in changelogs. process Issues and PRs related to the process subsystem. semver-minor PRs that contain new features and should be released in the next minor version.

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.

11 participants

@Flarna @BridgeAR @nodejs-github-bot @Trott @mmarchini @bnoordhuis @addaleax @cjihrig @joyeecheung @patilharshal16 @targos
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.