-
-
Notifications
You must be signed in to change notification settings - Fork 205
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
Fix unhandled exception not captured when hub disabled #2103
Conversation
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.
Does that mean that if the SDK got initialized it'll always capture Unhandled Exceptions
even if it has been closed on purpose?
Yes. An unhandled exception leads to a crash, so we want to always catch those. |
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!
I noticed that the
Sentry.Samples.Console.Basic
example wasn't working when run on my Mac with thenet48
target, but was working fine with thenet6.0
target, or on Windows with either target.After some investigation, it turns out that the Mono desktop runtime will call any
Dispose
from ausing
statement (or anyfinally
block) before it invokesAppDomain.UnhandledException
or similar events. That's probably a bug in Mono, as the behavior on .NET or .NET Framework is to fire those events immediately and not dispose. This doesn't look like it will be fixed in Mono any time soon, so we should accept the current behavior.The problem is that if
Hub.Dispose
is called before the unhandled exception integration captures the exception, the hub will have been disabled and the exception won't be captured. The solution thus is to allow the unhandled exception integration (and similar) to capture even if the hub has been disabled.While resolving this, I also noticed that we were checking
IsEnabled
only on extension methods likeCaptureException
,CaptureMessage
, etc. So it was always possible to still send events post-dispose by callingCaptureEvent
directly. That is a separate bug, but also resolved in this PR. The hub now checks if it is disabled before capturing anything, rather than the extension methods. An internalCaptureEventInternal
and some related code will allow the integrations to bypass those checks and still capture on disabled hubs.This change means that no events will not be captured post-dispose unless they come from an internal integration. That was always the intention, but only behaved that way with extension methods like
CaptureException
, and not with the basicCaptureEvent
method.