Change UnobservedTaskException from being tagged as UnhandledException to UnobservedTaskException #548
Conversation
| { | ||
| if (!_settings.CatchUnhandledExceptions) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| Send(exception, UnhandledExceptionTags); | ||
| Send(exception, new List<string> { tag }); |
There was a problem hiding this comment.
Looks like the constant UnhandledExceptionTags is no longer used because of this change and so should be deleted too.
There was a problem hiding this comment.
Had that in mind, kept it in case we needed it but likely we do not anymore
| @@ -82,15 +82,15 @@ public WeakExceptionHandler(UnhandledExceptionHandler handler) | |||
| _reference = new WeakReference<UnhandledExceptionHandler>(handler); | |||
| } | |||
|
|
|||
| public void Invoke(Exception exception, bool isTerminating) | |||
| public void Invoke(Exception exception, bool isTerminating, string tag) | |||
There was a problem hiding this comment.
Perhaps this class is not intended to be used by customers, but this is technically a breaking change due to it being public. (Same as all the public methods above). Shall we instead make an overload for this method and change the other methods above to not have a tag passed in (if that argument is never used) and just pass the string literal from with their method bodies?
There was a problem hiding this comment.
Should be addressed now, let me know what you think
Edit: That class is private, so that contract doesn't matter. I added the compatibility to the UnhandledExceptionBridge class
There was a problem hiding this comment.
Ah yes, I didn't see this code is in a nested private class
| @@ -23,22 +24,22 @@ static UnhandledExceptionBridge() | |||
| RaiseUnhandledException(args.ExceptionObject as Exception, args.IsTerminating); | |||
| }; | |||
|
|
|||
| TaskScheduler.UnobservedTaskException += (sender, args) => { RaiseUnhandledException(args.Exception, false); }; | |||
| TaskScheduler.UnobservedTaskException += (sender, args) => { RaiseUnhandledException(args.Exception,false, "UnobservedTaskException"); }; | |||
There was a problem hiding this comment.
@ProRedCat I believe UnobservedTaskException may always (or nearly always) raise an AggregateException. If there is only one inner exception this should really be unboxed. This appears to be the current behavior of the MAUI Xamarin provider.
There was a problem hiding this comment.
This change looks good @ProRedCat. What about AggregateException's which are not unobserved task exceptions (handled + unhandled) - Will they be unwrapped elsewhere?
There was a problem hiding this comment.
In this case right now it's only the UnobservedTaskExceptions, will have a chat with some other around handled + unhandled
There was a problem hiding this comment.
Observability for others: We do have a feature for automatically unwrapping exceptions in our .NET Core provider.
This is done through _raygunClient.AddWrapperExceptions(typeof(AggregateException))
No description provided.