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

DYN-6492 add unhandled exception handler to viewmodel #14840

Merged
merged 4 commits into from
Jan 22, 2024
+33 −1
Merged

DYN-6492 add unhandled exception handler to viewmodel #14840

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5acbf76
add unhandled exception handler to viewmodel
aparajit-pratap Jan 12, 2024
13c30b4
cleanup
aparajit-pratap Jan 12, 2024
379782f
add backup crash dialog if CER is unavailable
aparajit-pratap Jan 18, 2024
9231c5c
catch any exceptions thrown from handler itself
aparajit-pratap Jan 19, 2024
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
34 changes: 33 additions & 1 deletion src/DynamoCoreWpf/ViewModels/Core/DynamoViewModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
using System.Windows.Media;
using System.Windows.Threading;
using Dynamo.Configuration;
using Dynamo.Core;
using Dynamo.Engine;
using Dynamo.Exceptions;
using Dynamo.Graph;
Expand All @@ -22,6 +23,7 @@
using Dynamo.Graph.Nodes;
using Dynamo.Graph.Workspaces;
using Dynamo.Interfaces;
using Dynamo.Logging;
using Dynamo.Models;
using Dynamo.PackageManager;
using Dynamo.PackageManager.UI;
Expand All @@ -30,7 +32,6 @@
using Dynamo.Services;
using Dynamo.UI;
using Dynamo.UI.Prompts;
using Dynamo.Updates;
using Dynamo.Utilities;
using Dynamo.Visualization;
using Dynamo.Wpf.Interfaces;
Expand Down Expand Up @@ -675,6 +676,8 @@ public struct StartConfiguration

protected DynamoViewModel(StartConfiguration startConfiguration)
{
Dispatcher.CurrentDispatcher.UnhandledException += CurrentDispatcher_UnhandledException;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pinzart90 does it make sense to also subscribe to the appdomain exception event here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure where we should put this handler. Theoretically it should be added as soon as possible in the application lifecycle so that more exceptions can be handled.
Also this handler will not catch exceptions coming from other threads. THat is where AppDomain.UNhandledException would come in handy. However it seems a bit risky since it could be called in non UI threads (at least that's what I;ve noticed in my testing)


this.ShowLogin = startConfiguration.ShowLogin;

// initialize core data structures
Expand Down Expand Up @@ -754,6 +757,33 @@ protected DynamoViewModel(StartConfiguration startConfiguration)
FileTrustViewModel = new FileTrustWarningViewModel();
}

private void CurrentDispatcher_UnhandledException(object sender, DispatcherUnhandledExceptionEventArgs e)
{
if (e.Handled)
{
return;
}

e.Handled = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Marking it as handled will mean other handlers will no longer be called anymore. Ex D4R will exception Handler will no longer be called

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, as discussed, this needs some design so host-specific shutdown/cleanup steps can be taken into account in this handler. WIP.

Copy link
Contributor Author

@aparajit-pratap aparajit-pratap Jan 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I checked, and marking it as handled does not mean that other handlers will not be called. The Revit unhandled exception handler will still be called, the only thing is that the value of e.Handled will be true before it's called, which means we can check its value to decide if we want to skip any redundant steps that have already completed here such as logging to analytics, displaying the crash dialogs, etc.

Correction: I was wrong about the previous comment. I saw the handler in D4R being called earlier since another exception was being thrown while the exception was being handled in DynamoViewModel while shutting down Dynamo and this call wasn't within the try-catch. I have now moved this within the try-catch in the handler as well. According to the docs, Event handlers for this event must be written with care to avoid creating secondary exceptions and to catch any that occur.

I have removed the same handler from D4R, see my PR in DynamoRevit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, looks like there is another handler that can mark exceptions as “handled” https://learn.microsoft.com/en-us/dotnet/api/system.windows.threading.dispatcher.unhandledexceptionfilter?view=windowsdesktop-8.0

not sure what we could do with it(or how to use it). Maybe mark dynamo VM exceptions as handled so the app just continues and we only log(somehow). Probably for another time..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

VM exceptions are usually non-recoverable. They are signs of the VM being corrupt most of the time so it's wise to shutdown and restart Dynamo in those cases. Still worth taking a look at this. Thanks.

CrashGracefully(e.Exception);
pinzart90 marked this conversation as resolved.
Show resolved Hide resolved
}

private void CrashGracefully(Exception ex)
{
try
{
Model?.Logger?.LogError($"Unhandled exception {ex.Message}");

DynamoModel.IsCrashing = true;
Analytics.TrackException(ex, true);
Model?.OnRequestsCrashPrompt(new CrashErrorReportArgs(ex));

Exit(false); // don't allow cancellation
}
catch
{ }
}

/// <summary>
/// Sets up the provided <see cref="DefaultWatch3DViewModel"/> object and
/// adds it to the Watch3DViewModels collection.
Expand Down Expand Up @@ -3412,6 +3442,8 @@ public ShutdownParams(
///
public bool PerformShutdownSequence(ShutdownParams shutdownParams)
{
Dispatcher.CurrentDispatcher.UnhandledException -= CurrentDispatcher_UnhandledException;
aparajit-pratap marked this conversation as resolved.
Show resolved Hide resolved

if (shutdownSequenceInitiated)
{
// There was a prior call to shutdown. This could happen for example
Expand Down
Loading