Async Methods swallow exceptions #961
Conversation
Added a private field `_exception` and a constructor to `ResultCompletionEventArgs` to handle exceptions. Wrapped initialization and activation logic in `Screen.cs` with try-catch blocks to log errors and handle exceptions. Introduced new virtual methods `OnActivatedAsyncException`, `OnDeactivateAsyncException`, and `OnInitializedAsyncException` in `Screen.cs` to handle exceptions during asynchronous operations. Added a new method `CoroutineException` in `CaliburnApplication.cs`, `FormsApplication.cs`, `CaliburnApplicationDelegate.cs`, and `Bootstrapper.cs` to handle exceptions during coroutine execution. Added event handlers for `Coroutine.Completed` in these files to invoke `CoroutineException` when an error occurs. Made minor formatting changes and added missing namespaces in various files to ensure consistency and proper functionality. Removed unused `using` directives and added necessary ones in `BootstrapperBase.cs` and other files. Added logging for exceptions in `BootstrapperBase.cs` and ensured that coroutine exceptions are logged.
There was a problem hiding this comment.
Copilot reviewed 5 out of 12 changed files in this pull request and generated no comments.
Files not reviewed (7)
- src/Caliburn.Micro.Core/ResultCompletionEventArgs.cs: Evaluated as low risk
- src/Caliburn.Micro.Core/Screen.cs: Evaluated as low risk
- src/Caliburn.Micro.Platform/Platforms/Android/CaliburnApplication.cs: Evaluated as low risk
- src/Caliburn.Micro.Platform/Platforms/Maui/Android/CaliburnApplication.cs: Evaluated as low risk
- src/Caliburn.Micro.Platform/Platforms/Maui/FormsApplication.cs: Evaluated as low risk
- src/Caliburn.Micro.Platform/Platforms/Maui/Windows/CaliburnApplication.cs: Evaluated as low risk
- src/Caliburn.Micro.Platform/Platforms/Maui/iOS/CaliburnApplicationDelegate.cs: Evaluated as low risk
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
@darxis Hope you had a Happy New Year. Could you look at this when you get a chance? |
There was a problem hiding this comment.
Copilot reviewed 5 out of 12 changed files in this pull request and generated no comments.
Files not reviewed (7)
- src/Caliburn.Micro.Core/ResultCompletionEventArgs.cs: Evaluated as low risk
- src/Caliburn.Micro.Platform/Platforms/net46/Bootstrapper.cs: Evaluated as low risk
- src/Caliburn.Micro.Platform/Platforms/netcore-avalonia/BootstrapperBase.cs: Evaluated as low risk
- src/Caliburn.Micro.Platform/Platforms/uap/CaliburnApplication.cs: Evaluated as low risk
- src/Caliburn.Micro.Platform/Platforms/Android/CaliburnApplication.cs: Evaluated as low risk
- src/Caliburn.Micro.Platform/Platforms/iOS/CaliburnApplicationDelegate.cs: Evaluated as low risk
- src/Caliburn.Micro.Core/Screen.cs: Evaluated as low risk
vb2ae
changed the title
| /// </summary> | ||
| /// <param name="sender">The sender of the event.</param> | ||
| /// <param name="e">The event arguments containing exception details.</param> | ||
| protected virtual void CoroutineException(object sender, ResultCompletionEventArgs e) |
There was a problem hiding this comment.
Will this ever be called? Isn't it missing the following in Initialize method?
Coroutine.Completed += (s, e) =>
{
if (e.Error != null)
{
CoroutineException(s, e);
}
};If yes then please also check:
| /// <summary> | ||
| /// Called when exception called in OnActivatedAsync. | ||
| /// </summary> | ||
| protected virtual void OnActivatedAsyncException(Exception thrownException) |
There was a problem hiding this comment.
This gets called when OnActivateAsync or OnActivatedAsync throws an exception but the name OnActivatedAsyncException suggests it is related to the OnActivatedAsync method. Maybe we should split this into two methods? Like OnActivateAsyncException and OnActivatedAsyncException? Any other ideas?
PS: Same for OnInitializedAsyncException
There was a problem hiding this comment.
I think the rename from OnActivateAsync to OnActivatedAsync should be reverted, since that function is called when the framework wants to activate the screen and not when it is already activated.
| } | ||
| catch (Exception ex) | ||
| { | ||
| Log.Error(ex); |
There was a problem hiding this comment.
Missing call to OnInitializeAsyncException/OnInitializedAsyncException
There was a problem hiding this comment.
I agree, either all the methods should log the exception or there should be a OnException method that the user can hook into.
| @@ -174,5 +183,12 @@ protected virtual IEnumerable<object> GetAllInstances(Type service) | |||
| protected virtual void BuildUp(object instance) | |||
| { | |||
| } | |||
|
|
|||
| /// <summary> | |||
| /// Called by the bootstrapper's constructor at design time to start the framework. | |||
There was a problem hiding this comment.
| /// </summary> | ||
| /// <param name="sender">The sender.</param> | ||
| /// <param name="e">The event args.</param> | ||
| protected virtual void OnUnhandledException(object sender, DispatcherUnhandledExceptionEventArgs e) |
There was a problem hiding this comment.
Will it ever be called? I can't see a call to this within this PR. Also why is this only added for the netcore-avalonia platform?
There was a problem hiding this comment.
In WPF, this method subscribes to the the Application class DispatcherUnhandledException, is there something similar in Avalonia?
There was a problem hiding this comment.
removing avalonia ui does not have the DisplatcherUnhandledException
There was a problem hiding this comment.
In your comment you said that there was a private field added to this class but I only see the added constructor?
| IsInitialized = initialized = true; | ||
| await OnInitializedAsync(cancellationToken); | ||
| IsInitialized = initialized = true; | ||
| await OnInitializedAsync(cancellationToken); |
There was a problem hiding this comment.
This is not something that was done here in this PR, but why was the name changed from OnInitializeAsync to OnInitializedAsync? This method is called when initializing so the initializing happens in the OnInitializeAsync thus the name OnInitializeAsync makes more sense then OnInitializedAsync since OnInitializedAsync implies that the initialization is already completed.
| } | ||
| catch (Exception ex) | ||
| { | ||
| Log.Error(ex); |
There was a problem hiding this comment.
I agree, either all the methods should log the exception or there should be a OnException method that the user can hook into.
| /// <summary> | ||
| /// Called when exception called in OnActivatedAsync. | ||
| /// </summary> | ||
| protected virtual void OnActivatedAsyncException(Exception thrownException) |
There was a problem hiding this comment.
I think the rename from OnActivateAsync to OnActivatedAsync should be reverted, since that function is called when the framework wants to activate the screen and not when it is already activated.
| /// </summary> | ||
| /// <param name="sender">The sender.</param> | ||
| /// <param name="e">The event args.</param> | ||
| protected virtual void OnUnhandledException(object sender, DispatcherUnhandledExceptionEventArgs e) |
There was a problem hiding this comment.
In WPF, this method subscribes to the the Application class DispatcherUnhandledException, is there something similar in Avalonia?
| catch (Exception ex) | ||
| { | ||
| Log.Error(ex); | ||
| OnInitializedAsyncException(ex); | ||
| } |
Check notice
Code scanning / CodeQL
Generic catch clause Note
| catch (Exception ex) | ||
| { | ||
| Log.Error(ex); | ||
| OnActivatedAsyncException(ex); | ||
| } |
Check notice
Code scanning / CodeQL
Generic catch clause Note
| catch (Exception ex) | ||
| { | ||
| Log.Error(ex); | ||
| OnDeactivateAsyncException(ex); | ||
| } |
Check notice
Code scanning / CodeQL
Generic catch clause Note
This pull request introduces several changes to improve exception handling across various files in the
Caliburn.Microframework. The most significant changes include adding exception handling mechanisms in asynchronous methods and updating theResultCompletionEventArgsclass to include an exception property.Exception Handling Enhancements:
src/Caliburn.Micro.Core/Screen.cs: Added try-catch blocks inActivateAsync,DeactivateAsync, and other methods to log exceptions and call new virtual methods for handling exceptions. [1] [2] [3] [4]src/Caliburn.Micro.Core/ResultCompletionEventArgs.cs: Introduced a private_exceptionfield and a constructor to initialize it.Coroutine Exception Handling:
src/Caliburn.Micro.Platform/Platforms/Android/CaliburnApplication.cs: Added aCoroutineExceptionmethod to handle exceptions during coroutine execution.src/Caliburn.Micro.Platform/Platforms/Maui/Android/CaliburnApplication.cs: AddedCoroutineExceptionmethod and updatedInitializemethod to subscribe toCoroutine.Completedevent. [1] [2]src/Caliburn.Micro.Platform/Platforms/net46-netcore/Bootstrapper.cs: AddedCoroutineExceptionmethod and updatedInitializemethod to handle coroutine exceptions. [1] [2]Miscellaneous Changes:
src/Caliburn.Micro.Platform/Platforms/Maui/FormsApplication.cs: AddedCoroutineExceptionmethod and updatedInitializemethod to handle coroutine exceptions. [1] [2]src/Caliburn.Micro.Platform/Platforms/netcore-avalonia/BootstrapperBase.cs: AddedCoroutineExceptionmethod and updatedInitializemethod to handle coroutine exceptions. [1] [2]These changes enhance the robustness of the framework by ensuring that exceptions are properly logged and handled, preventing potential crashes and improving debugging capabilities.
Closing #863, #745