-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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: add capture function for uncaught exceptions #17159
Changes from all commits
f1293d5
4c85f6e
df76a62
f78f2ac
d169b26
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -9,6 +9,7 @@ | |||||||||
|
||||||||||
(function(process) { | ||||||||||
let internalBinding; | ||||||||||
const exceptionHandlerState = { captureFn: null }; | ||||||||||
|
||||||||||
function startup() { | ||||||||||
const EventEmitter = NativeModule.require('events'); | ||||||||||
|
@@ -35,6 +36,7 @@ | |||||||||
const _process = NativeModule.require('internal/process'); | ||||||||||
_process.setupConfig(NativeModule._source); | ||||||||||
_process.setupSignalHandlers(); | ||||||||||
_process.setupUncaughtExceptionCapture(exceptionHandlerState); | ||||||||||
NativeModule.require('internal/process/warning').setup(); | ||||||||||
NativeModule.require('internal/process/next_tick').setup(); | ||||||||||
NativeModule.require('internal/process/stdio').setup(); | ||||||||||
|
@@ -377,8 +379,10 @@ | |||||||||
// that threw and was never cleared. So clear it now. | ||||||||||
async_id_fields[kInitTriggerAsyncId] = 0; | ||||||||||
|
||||||||||
if (process.domain && process.domain._errorHandler) | ||||||||||
caught = process.domain._errorHandler(er); | ||||||||||
if (exceptionHandlerState.captureFn !== null) { | ||||||||||
exceptionHandlerState.captureFn(er); | ||||||||||
caught = true; | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this conflicts with Lines 2479 to 2482 in bb44626
We probably need better tests for that behaviour (I noticed it wasn't really tested while working on it myself)... unless I'm missing something with the above. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @apapirovski Since you brought it up in the other PR: I’m writing a PR that removes There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @addaleax Sounds good! I guess it would either have to land before this or this would still need to be updated, right? Out of curiosity, what direction are you going with it? (Because technically mine removed it too but I'm guessing you have a different solution in mind.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @apapirovski Here’s the PR: #17333 :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, and regarding updating this PR: The situation you’re worried about is a domain setting a capture function, that would proceed to capture the exception generated by one of these? I think you are right, #17333 should probably land first (or at least this should not land without it)… |
||||||||||
} | ||||||||||
|
||||||||||
if (!caught) | ||||||||||
caught = process.emit('uncaughtException', er); | ||||||||||
|
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.
Nit: template string here?
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.
But does that really make it more readable?