-
Notifications
You must be signed in to change notification settings - Fork 635
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.
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
crash handlers for Dynamo #14826
crash handlers for Dynamo #14826
Conversation
pinzart90
commented
Jan 10, 2024
•
edited
Loading
edited
- Introduced the following exception handlers (all in DynamoViewModel.cs):
- Dispatcher.UnhandledException - catches unhandled exceptions from the UI thread. Can mark exceptions as handled (and close Dynamo) so that host apps can continue running normally even though Dynamo crashed
- AppDomain.UnhandledException - catches unhandled exceptions that are fatal to the current process. These exceptions cannot be handled and process termination is guaranteed.
- TaskScheduler.UnobservedTaskException - catches unobserved Task exceptions from all threads. Does not crash DYnamo, we only log the exceptions and do not call CER or close Dynamo.
- DynamoModel.IsCrashing is reset (set to false) on every DynamoModel constructor call.
- SplashScreen::WebView_NavigationCompleted is wrapped in a try catch.
UI Smoke TestsTest: success. 2 passed, 0 failed. |
UI Smoke TestsTest: success. 2 passed, 0 failed. |
UI Smoke TestsTest: success. 2 passed, 0 failed. |
@pinzart90 it looks like none of the code from this line onwards will execute after these changes, i.e., if the unhandled exceptions are handled by your new handlers. Neither will this crash prompt dialog (sad-face) be displayed: While this might be a good change since now we're capturing these exceptions in CER, I wonder if any of the steps that we were doing earlier (other than showing the sad-face dialog), such as saving the recorded commands, etc. still be needed? |
@pinzart90 in the case of Sandbox, I do see that unhandled exceptions are eventually caught here and then handled to show the CER dialog before Dynamo is shutdown gracefully. So I'm not sure if having these new unhandled exception handlers that more or less do the same thing is even necessary. Can you clarify? |
@pinzart90 You have a conflict. |
if (!DynamoModel.IsCrashing && !IsClosing) | ||
{ | ||
CrashReportTool.ShowCrashWindow(viewModel, new CrashErrorReportArgs(ex)); | ||
Close();// Close the SpashScreen |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added Try Catch in the WebView_NavigationCompleted
body.
Looks like all exceptions thrown in WebView_NavigationCompleted are silenced by webview2.
SplashScreen.WebView_NavigationCompleted
is particularly sensitive because Dynamo views are initialzied inside this method and if an exceptions is thrown, SplashScreen will seem like it hangs.
@@ -162,6 +162,21 @@ private static string FindCERToolInInstallLocations() | |||
} | |||
} | |||
|
|||
internal static void ShowCrashWindow(object sender, CrashPromptArgs args) | |||
{ | |||
var viewModel = sender as DynamoViewModel; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add a null check here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually can be null. ShowCrashWindow could be called like ShowCrashWindow(null, new CrashPromptArgs());
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- what tasks in particular were throwing exceptions? Can we make them
Task<T>
? - Any thoughts on these handlers or similar ones needing to be added to DynamoModel for headless contexts? I guess we care less about those crashing.
|
@@ -690,14 +691,20 @@ public static DynamoViewModel Start(StartConfiguration startConfiguration = new | |||
|
|||
protected DynamoViewModel(StartConfiguration startConfiguration) | |||
{ | |||
// CurrentDomain_UnhandledException - catches unhandled exceptions that are fatal to the current process. These exceptions cannot be handled and process termination is guaranteed | |||
AppDomain.CurrentDomain.UnhandledException += CurrentDomain_UnhandledException; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought this was initially added to DynamoModel
. Can you explain why it has been added here instead of DynamoModel
and the same for TaskScheduler.UnobservedException
? Since DynamoModel
is instantiated before the view model, there could be code that's uncaught by any of these handlers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would require more refactor. CrashTool is in the DynamoCoreWPF project. It (along with other CrashPromptArgs) uses DynamoViewModel ..which I cannot access from DynamoCore and also may not exist yet depending on when the crash happens. Also if DynamoViewModel is created...then we would want all exception handlers to start using the DynamoViewModel. and stop the DynamoModel handler ?
I would leave that for another PR...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. Maybe we should decouple the CER crash prompt from DynamoViewModel. Currently, it needs the view model to show the dialog. Would be good to remove the dependency on the view model for this dialog if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am trying that now, but there are too many changes for this PR
I will create a followup