DYN-6492 add unhandled exception handler to viewmodel #14840
Conversation
aparajit-pratap
changed the title
UI Smoke TestsTest: success. 2 passed, 0 failed. |
UI Smoke TestsTest: success. 2 passed, 0 failed. |
| @@ -675,6 +675,8 @@ public static DynamoViewModel Start(StartConfiguration startConfiguration = new | |||
|
|
|||
| protected DynamoViewModel(StartConfiguration startConfiguration) | |||
| { | |||
| Dispatcher.CurrentDispatcher.UnhandledException += CurrentDispatcher_UnhandledException; | |||
There was a problem hiding this comment.
@pinzart90 does it make sense to also subscribe to the appdomain exception event here?
There was a problem hiding this comment.
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)
| return; | ||
| } | ||
|
|
||
| e.Handled = true; |
There was a problem hiding this comment.
Marking it as handled will mean other handlers will no longer be called anymore. Ex D4R will exception Handler will no longer be called
There was a problem hiding this comment.
Yes, as discussed, this needs some design so host-specific shutdown/cleanup steps can be taken into account in this handler. WIP.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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..
There was a problem hiding this comment.
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.
UI Smoke TestsTest: success. 2 passed, 0 failed. |
|
@aparajit-pratap maybe we should also check what civil is doing for handling exceptions and make sure this change is compatible |
@pinzart90 I checked, and it doesn't look like civil is doing any exception handling like this or like the one being done in D4R. |
UI Smoke TestsTest: success. 2 passed, 0 failed. |
aparajit-pratap
changed the title
Purpose
Subscribe to
Application.DispatcherUnhandledExceptionevent to handle unhandled exceptions coming from external code like extensions. This PR basically takes the work done in #14826 by @pinzart90 and moves it to theViewModelso it isn't specific to sandbox.WIP for some initial feedback.
Declarations
Check these if you believe they are true
*.resxfilesRelease Notes
(FILL ME IN) Brief description of the fix / enhancement. Mandatory section
FYIs
(FILL ME IN, Optional) Names of anyone else you wish to be notified of