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

process: allow monitoring uncaughtException #31257

Closed

Conversation

@Flarna
Copy link
Member

Flarna commented Jan 8, 2020

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
@Flarna Flarna force-pushed the dynatrace-oss-contrib:uncaughtExceptionMonitor branch from 18704d4 to 85d940c Jan 8, 2020
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 dynatrace-oss-contrib:uncaughtExceptionMonitor branch from 85d940c to 9039128 Jan 8, 2020
@Flarna

This comment has been minimized.

Copy link
Member Author

Flarna commented Jan 8, 2020

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?

@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Jan 8, 2020

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.

Copy link
Member

addaleax left a comment

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.

doc/api/process.md Outdated Show resolved Hide resolved
@Flarna

This comment has been minimized.

Copy link
Member Author

Flarna commented Jan 9, 2020

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.

Copy link
Member

bnoordhuis left a comment

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

@Flarna

This comment has been minimized.

Copy link
Member Author

Flarna commented Jan 10, 2020

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

Copy link
Member

bnoordhuis left a comment

LGTM with a suggestion.

@nodejs-github-bot

This comment has been minimized.

doc/api/process.md Outdated Show resolved Hide resolved
doc/api/process.md Outdated Show resolved Hide resolved
doc/api/process.md Outdated Show resolved Hide resolved
Flarna and others added 3 commits Jan 10, 2020
Co-Authored-By: Colin Ihrig <cjihrig@gmail.com>
Co-Authored-By: Colin Ihrig <cjihrig@gmail.com>
Co-Authored-By: Colin Ihrig <cjihrig@gmail.com>
@Trott
Trott approved these changes Jan 10, 2020
@nodejs-github-bot

This comment has been minimized.

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Jan 10, 2020

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

@Trott Trott added the semver-minor label Jan 11, 2020
@Trott

This comment has been minimized.

Copy link
Member

Trott commented Jan 11, 2020

Landed in f4797ff

@Trott Trott closed this Jan 11, 2020
Trott added a commit that referenced this pull request Jan 11, 2020
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 <ruben@bridgewater.de>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@Flarna Flarna deleted the dynatrace-oss-contrib:uncaughtExceptionMonitor branch Jan 11, 2020
MylesBorins added a commit that referenced this pull request Jan 16, 2020
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 <ruben@bridgewater.de>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere added a commit that referenced this pull request Jan 16, 2020
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
codebytere added a commit that referenced this pull request Jan 17, 2020
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
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
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

This comment has been minimized.

Copy link
Member

mmarchini commented Jan 20, 2020

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

@Flarna

This comment has been minimized.

Copy link
Member Author

Flarna commented Jan 20, 2020

@mmarchini Done, I removed also the final sentence that a symbol has been added as this was removed during the review phase.

codebytere added a commit that referenced this pull request Jan 21, 2020
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
codebytere added a commit that referenced this pull request Jan 21, 2020
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
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
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

10 participants
You can’t perform that action at this time.