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

Jump to bottom

[DNM] Remove Dispatcher unhandled exception handler since it's already handled in DynamoCore #3029

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

[DNM] Remove Dispatcher unhandled exception handler since it's already handled in DynamoCore #3029

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e2887c5
skip handling exception if already handled in DynamoCore
aparajit-pratap Jan 18, 2024
d803e2a
Merge branch 'master' of github.com:DynamoDS/DynamoRevit into dyn-6492
aparajit-pratap Jan 18, 2024
70507c9
remove dispatcher unhandled exception handler as it's been moved to D…
aparajit-pratap Jan 19, 2024
8f85329
Merge branch 'master' of github.com:DynamoDS/DynamoRevit into dyn-6492
aparajit-pratap Jan 23, 2024
0146ced
resolve merge conflicts
aparajit-pratap Feb 20, 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
49 changes: 0 additions & 49 deletions src/DynamoRevit/DynamoRevit.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -696,7 +696,6 @@ private static DynamoView InitializeCoreView(DynamoRevitCommandData commandData)

DynamoAppExternalEvent = ExternalEvent.Create(appEventHandler);

dynamoView.Dispatcher.UnhandledException += Dispatcher_UnhandledException;
dynamoView.Closed += OnDynamoViewClosed;
dynamoView.Loaded += (o, e) => DynamoAppExternalEvent.Raise();

Expand Down Expand Up @@ -1019,52 +1018,6 @@ public static string GetRevitContext(DynamoRevitCommandData commandData)

#endregion

#region Exception

/// <summary>
/// A method to deal with unhandled exceptions. Executes right before Revit crashes.
/// Dynamo is still valid at this time, but further work may cause corruption. Here,
/// we run the ExitCommand, allowing the user to save all of their work.
/// </summary>
/// <param name="sender"></param>
/// <param name="args">Info about the exception</param>
private static void Dispatcher_UnhandledException(
object sender, DispatcherUnhandledExceptionEventArgs args)
{
args.Handled = true;

// only handle a single crash per Dynamo sesh, this should be reset in the initial command
if (handledCrash)
return;

handledCrash = true;

string exceptionMessage = args.Exception.Message;

try
{
Dynamo.Logging.Analytics.TrackException(args.Exception, true);

RevitDynamoModel.Logger.LogError("Dynamo Unhandled Exception");
RevitDynamoModel.Logger.LogError(exceptionMessage);
}
catch { }

try
{
DynamoModel.IsCrashing = true;
RevitDynamoModel.OnRequestsCrashPrompt(new CrashErrorReportArgs(args.Exception));
RevitDynamoViewModel.Exit(false); // don't allow cancellation
}
catch { }
finally
{
args.Handled = true;
DynamoRevitApp.DynamoButtonEnabled = true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

what about the button ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The button is reenabled even without this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it somewhere in the view.CLose events ?
THere would be a chance that if an exception is thrown in the exception handler, then the button close will not be called at all

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, it is re-enabled in OnDynamoViewClosed here.

Copy link
Collaborator

@Mikhinja Mikhinja Jan 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

But why the API breaking change so late, in a minor build (3.0.2 vs 3.0.0), and why after Revit API freeze?

This would require more testing on Revit side, that various CER scenarios still work and we are not missing crash reports, possibly even more code changed.

Is this API change in 3.0.2 mandatory or can we revert it, or make other builds based on 3.0.0 with only the critical fixes? @QilongTang FYI

Copy link
Contributor Author

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

Choose a reason for hiding this comment

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

@Mikhinja this is not an API breaking change - the API change is in this PR: #3032

This PR is about removing the API call entirely in D4R and moving it to DynamoCore instead so other host integrations can benefit from it without needing to call it explicitly in their integration code.

Having said that, the Dynamo team is only experimenting with this so far and we are not ready to merge this for global launch, which is why I've marked this PR as DNM. We will continue refining this approach post 3.0.x.

Note that PR #3032 does need to be merged into D4R when it begins to consume 3.0.1 or 3.0.2 and higher.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, that change is what I was actually referring to. I merged it together with Dynamo 3.0.2.

}
}

#endregion

#region Shutdown

Expand All @@ -1077,7 +1030,6 @@ private static void OnDynamoViewClosed(object sender, EventArgs e)
{
var view = (DynamoView)sender;

view.Dispatcher.UnhandledException -= Dispatcher_UnhandledException;
view.Closed -= OnDynamoViewClosed;

DynamoRevitApp.DynamoButtonEnabled = true;
Expand All @@ -1095,7 +1047,6 @@ private static void OnSplashScreenClosed(object sender, EventArgs e)
{
var view = (Window)sender;

view.Dispatcher.UnhandledException -= Dispatcher_UnhandledException;
view.Closed -= OnSplashScreenClosed;
//if the user explicitly closed the splashscreen, then we should let them boot
//dynamo back up.
Expand Down