Skip to content
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.

Sign up for GitHub

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

Async Methods swallow exceptions #961

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Async Methods swallow exceptions #961

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
71c6fbc
Enhance exception handling and logging across the app
vb2ae Jan 1, 2025
a96c669
fix the build
vb2ae Jan 1, 2025
d1e6c0f
Fix some of the errors
vb2ae Jan 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/Caliburn.Micro.Core/ResultCompletionEventArgs.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,5 +18,12 @@ public class ResultCompletionEventArgs : EventArgs
/// </summary>
/// <value><c>true</c> if cancelled; otherwise, <c>false</c>.</value>
public bool WasCancelled;

/// <summary>
/// Initializes a new instance of the <see cref="ResultCompletionEventArgs"/> class.
/// </summary>
public ResultCompletionEventArgs()
{
}
}
}
71 changes: 61 additions & 10 deletions src/Caliburn.Micro.Core/Screen.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,20 +103,35 @@

if (!IsInitialized)
{
try
{
Log.Info("Initializing {0}.", this);
#pragma warning disable CS0618 // Type or member is obsolete
await OnInitializeAsync(cancellationToken);
await OnInitializeAsync(cancellationToken);
Dismissed Show dismissed Hide dismissed
#pragma warning restore CS0618 // Type or member is obsolete
IsInitialized = initialized = true;
await OnInitializedAsync(cancellationToken);
IsInitialized = initialized = true;
await OnInitializedAsync(cancellationToken);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing call to OnInitializeAsyncException/OnInitializedAsyncException

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, either all the methods should log the exception or there should be a OnException method that the user can hook into.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

OnInitializedAsyncException(ex);
}
Comment on lines +115 to +119

Check notice

Code scanning / CodeQL

Generic catch clause Note

Generic catch clause.
}

Log.Info("Activating {0}.", this);
try
{
Log.Info("Activating {0}.", this);
#pragma warning disable CS0618 // Type or member is obsolete
await OnActivateAsync(cancellationToken);
await OnActivateAsync(cancellationToken);
Dismissed Show dismissed Hide dismissed
#pragma warning restore CS0618 // Type or member is obsolete
IsActive = true;
await OnActivatedAsync(cancellationToken);

IsActive = true;
await OnActivatedAsync(cancellationToken);
}
catch (Exception ex)
{
Log.Error(ex);
OnActivatedAsyncException(ex);
}
Comment on lines +130 to +134

Check notice

Code scanning / CodeQL

Generic catch clause Note

Generic catch clause.
await (Activated?.InvokeAllAsync(this, new ActivationEventArgs
{
WasInitialized = initialized
Expand All @@ -134,7 +149,16 @@
});

Log.Info("Deactivating {0}.", this);
await OnDeactivateAsync(close, cancellationToken);
try
{
await OnDeactivateAsync(close, cancellationToken);
}
catch (Exception ex)
{
Log.Error(ex);
OnDeactivateAsyncException(ex);
}
Comment on lines +156 to +160

Check notice

Code scanning / CodeQL

Generic catch clause Note

Generic catch clause.

IsActive = false;

await (Deactivated?.InvokeAllAsync(this, new DeactivationEventArgs
Expand Down Expand Up @@ -206,6 +230,15 @@
}


/// <summary>
/// Called when exception called in OnActivatedAsync.
/// </summary>
protected virtual void OnActivatedAsyncException(Exception thrownException)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
Log.Info("Activated Async Exception");
Log.Error(thrownException);
}

/// <summary>
/// Called when view has been activated.
/// </summary>
Expand All @@ -226,5 +259,23 @@
Log.Info("Task deactivate");
return Task.FromResult(true);
}

/// <summary>
/// Called when exception called in OnDectivateAsync.
/// </summary>
protected virtual void OnDeactivateAsyncException(Exception thrownException)
{
Log.Info("Deactivate Async Exception");
Log.Error(thrownException);
}

/// <summary>
/// Called when exception called in OnInitializedAsync.
/// </summary>
protected virtual void OnInitializedAsyncException(Exception thrownException)
{
Log.Info("Initialized Async Exception");
Log.Error(thrownException);
}
}
}
13 changes: 11 additions & 2 deletions src/Caliburn.Micro.Platform/Platforms/Android/CaliburnApplication.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ public class CaliburnApplication : Application
public CaliburnApplication(IntPtr javaReference, JniHandleOwnership transfer)
: base(javaReference, transfer)
{

}

/// <summary>
Expand All @@ -41,7 +41,7 @@ protected virtual void StartDesignTime()

/// <summary>
/// Called by the bootstrapper's constructor at runtime to start the framework.
/// </summary>B
/// </summary>
protected virtual void StartRuntime()
{
AssemblySourceCache.Install();
Expand Down Expand Up @@ -131,5 +131,14 @@ protected virtual IEnumerable<object> GetAllInstances(Type service)
protected virtual void BuildUp(object instance)
{
}

/// <summary>
/// Handles exceptions that occur during coroutine execution.
/// </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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

{
}
}
}
24 changes: 18 additions & 6 deletions src/Caliburn.Micro.Platform/Platforms/Maui/Android/CaliburnApplication.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,7 @@
using System;
using System.Collections.Generic;
using System.Reflection;
using Android.App;
using Android.Runtime;
using Microsoft.Maui;
using Microsoft.Maui.Hosting;

namespace Caliburn.Micro.Maui
{
Expand All @@ -23,7 +20,7 @@ public abstract class CaliburnApplication : Microsoft.Maui.MauiApplication
public CaliburnApplication(IntPtr javaReference, JniHandleOwnership transfer)
: base(javaReference, transfer)
{

}

/// <summary>
Expand All @@ -39,7 +36,7 @@ protected virtual void StartDesignTime()

/// <summary>
/// Called by the bootstrapper's constructor at runtime to start the framework.
/// </summary>B
/// </summary>
protected virtual void StartRuntime()
{
AssemblySourceCache.Install();
Expand Down Expand Up @@ -79,6 +76,14 @@ protected void Initialize()
{
StartRuntime();
}

Coroutine.Completed += (s, e) =>
{
if (e.Error != null)
{
CoroutineException(s, e);
}
};
}

/// <summary>
Expand All @@ -97,6 +102,13 @@ protected virtual IEnumerable<Assembly> SelectAssemblies()
return new[] { GetType().GetTypeInfo().Assembly };
}


/// <summary>
/// Handles exceptions that occur during coroutine execution.
/// </summary>
/// <param name="sender">The sender of the event.</param>
/// <param name="e">The event arguments containing the exception details.</param>
protected virtual void CoroutineException(object sender, ResultCompletionEventArgs e)
{
}
}
}
26 changes: 21 additions & 5 deletions src/Caliburn.Micro.Platform/Platforms/Maui/FormsApplication.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
using System.Collections.Generic;
using System.Linq;
using System.Reflection;
using System.Runtime.CompilerServices;
using System.Threading.Tasks;
using Microsoft.Maui.Controls;

Expand All @@ -19,8 +18,10 @@ public abstract class MauiApplication : Application
/// <summary>
/// Start the framework.
/// </summary>
protected void Initialize() {
if (isInitialized) {
protected void Initialize()
{
if (isInitialized)
{
return;
}

Expand All @@ -30,10 +31,11 @@ protected void Initialize() {

var baseExtractTypes = AssemblySourceCache.ExtractTypes;

AssemblySourceCache.ExtractTypes = assembly => {
AssemblySourceCache.ExtractTypes = assembly =>
{
var baseTypes = baseExtractTypes(assembly);
var elementTypes = assembly.ExportedTypes
.Where(t => typeof (Element).GetTypeInfo().IsAssignableFrom(t.GetTypeInfo()));
.Where(t => typeof(Element).GetTypeInfo().IsAssignableFrom(t.GetTypeInfo()));

return baseTypes.Union(elementTypes);
};
Expand All @@ -44,6 +46,13 @@ protected void Initialize() {
IoC.BuildUp = BuildUp;

AssemblySource.Instance.Refresh();
Coroutine.Completed += (s, e) =>
{
if (e.Error != null)
{
CoroutineException(s, e);
}
};
}

/// <summary>
Expand Down Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: Fix comment within <summary>. Same for:

/// </summary>
protected virtual void CoroutineException(object sender, ResultCompletionEventArgs e)
{
}
}
}
7 changes: 7 additions & 0 deletions src/Caliburn.Micro.Platform/Platforms/Maui/Windows/CaliburnApplication.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,5 +141,12 @@ protected virtual void OnUnhandledException(object sender, Microsoft.UI.Xaml.Unh
{
}

/// <summary>
/// Called by the bootstrapper's constructor at design time to start the framework.
/// </summary>
protected virtual void CoroutineException(object sender, ResultCompletionEventArgs e)
{
}

}
}
19 changes: 15 additions & 4 deletions src/Caliburn.Micro.Platform/Platforms/Maui/iOS/CaliburnApplicationDelegate.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,5 @@
using System;
using System.Collections.Generic;
using System.Reflection;
using Foundation;
using UIKit;
using Microsoft.Maui;

namespace Caliburn.Micro.Maui
Expand Down Expand Up @@ -86,6 +83,13 @@ protected void Initialize()
}
else
StartRuntime();
Coroutine.Completed += (s, e) =>
{
if (e.Error != null)
{
CoroutineException(s, e);
}
};
}

/// <summary>
Expand All @@ -101,7 +105,14 @@ protected virtual void Configure()
/// <returns>A list of assemblies to inspect.</returns>
protected virtual IEnumerable<Assembly> SelectAssemblies()
{
return new[] {GetType().GetTypeInfo().Assembly};
return new[] { GetType().GetTypeInfo().Assembly };
}

/// <summary>
/// Called by the bootstrapper's constructor at design time to start the framework.
/// </summary>
protected virtual void CoroutineException(object sender, ResultCompletionEventArgs e)
{
}

}
Expand Down
13 changes: 11 additions & 2 deletions src/Caliburn.Micro.Platform/Platforms/iOS/CaliburnApplicationDelegate.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ protected virtual void Configure()
/// <returns>A list of assemblies to inspect.</returns>
protected virtual IEnumerable<Assembly> SelectAssemblies()
{
return new[] {GetType().GetTypeInfo().Assembly};
return new[] { GetType().GetTypeInfo().Assembly };
}

/// <summary>
Expand All @@ -127,7 +127,7 @@ protected virtual object GetInstance(Type service, string key)
/// <returns>The located services.</returns>
protected virtual IEnumerable<object> GetAllInstances(Type service)
{
return new[] {Activator.CreateInstance(service)};
return new[] { Activator.CreateInstance(service) };
}

/// <summary>
Expand All @@ -137,5 +137,14 @@ protected virtual IEnumerable<object> GetAllInstances(Type service)
protected virtual void BuildUp(object instance)
{
}

/// <summary>
/// Handles exceptions that occur during coroutine execution.
/// </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)
{
}
}
}
Loading
Loading