Reset uncaught exception handling after startup execution #5342
Conversation
mhart
changed the title
|
|
||
| // Reset this back to its default after startup execution | ||
| // Leaving it on comes at the expense of collecting stack traces for all thrown exceptions | ||
| if (script->isolate->impl->inspector == kj::none) { |
There was a problem hiding this comment.
Should we set it to false regardless of inspector? Since we use inspector for profiling as well?
There was a problem hiding this comment.
EDIT: Ignore comment, I misread the condition.
In the Script constructor, we do:
if (isolate->impl->inspector != kj::none || errorReporter != kj::none) {
lock.v8Isolate->SetCaptureStackTraceForUncaughtExceptions(true);
}So if the inspector is off, but errorReporter is used (which is always the case in workerd, and in dynamic isolates), then the code as currently written will leave SetCaptureStackTraceForUncaughtExceptions() on.
I think that's not what we want? Seems like we should just turn it off unconditionally here.
There was a problem hiding this comment.
Oops, I misread the conditional.
OK, I could go either way on this.
There was a problem hiding this comment.
I'll check what the behaviour's actually like in the inspector after this change (we'd still want some way of getting unhandled exceptions with the inspector on if the user wants to – just not sure if there's a way to manually turn it on via Protocol Monitor or similar)
| }); | ||
|
|
||
| // Reset this back to its default after startup execution | ||
| // Leaving it on comes at the expense of collecting stack traces for all thrown exceptions |
There was a problem hiding this comment.
Would you mind adding a link to the github issue, so we don't lose the context?
3 participants
Fixes #5332
I've confirmed this keeps the stack for any uncaught errors during startup:
Turning this off any earlier (eg at the end of the Script constructor) is too early:
I've also confirmed this improves performance of running scripts with
throwpatterns as per #5332