Open RussKie opened 4 years ago
I had a quick look and I think the original code lived in NDP\fx\src\CompMod\System\Diagnostics\assertwrapper.cs
Just for the record, the Desktop assert handler is terribly broken because showing modal UI on the faulting thread is performing message pumping, thus introducing reentrancy at a point where code doesn't expect it.
This has been causing endless pain during debugging sessions because the application keeps running while waiting in the dialog, allowing other code to run before you have a chance to break into the debugger. Worse: that other code runs before the asserting method finishes execution, so everything is often in inconsistent state when other event handlers are run, possibly triggering a cascade of asserts. In particular if you trigger an assert in code called from a paint handler you don't even get to read the dialog because the message pump will run through another paint immediately, recursively triggering the assert dialog until crashing.
Considering the behavior in Desktop wasn't a great experience I'm not excited that this is treated as a regression. The default Core experience at least breaks into debugger (if present and configured correctly) and avoids running arbitrary code in unknown context. While its not that great either, I personally prefer the Core default over the Desktop default. Even though the Desktop default is more "beginner friendly" and ok for simple forms, it is terrible for more advanced forms with timers or controls with paint handlers.
I'm not completely opposed to bringing back interactive error handlers but this needs to be redesigned and not just ported. Ideally error handling should always be out of process like the WER dialog, but at the very least it should immediately break into the debugger if one is attached, so the dev can observe the faulting code at the state where it triggered the assert.
@weltkante I agree that old assert dialog is terrible. But with this is mind many ppl used it in only those places where it's ok to have such a dialog on assert.
The default Core experience at least breaks into debugger (if present and configured correctly) and avoids running arbitrary code in unknown context.
You mean if it attached? I mean that if debugger not attached that I prefer old horrible dialog than app crash with record in event log as for now...
I'm not completely opposed to bringing back interactive error handlers but this needs to be redesigned and not just ported. Ideally error handling should always be out of process like the WER dialog, but at the very least it should immediately break into the debugger if one is attached, so the dev can observe the faulting code at the state where it triggered the assert.
Totally agree, and if debugger not attached suggest to use opened one or new instance...
You mean if it attached?
yes, only if I'm already debugging, not if I'm just having a debugger installed
But with this is mind many ppl used it in only those places where it's ok to have such a dialog on assert.
There is no such thing as a place "where it is ok" that is consciously chosen by developers. I doubt anyone who is writing assertions is doing this under the assumption that his (or maybe someone elses) UI timer/paint code will run inline in the failure case, that is always a painful surpise the first time it happens to someone and in hindsight obvious, but I've not seen anybody go out of their way and write their asserts differently after having experienced this. What people do instead is ignore the problem and hope they don't have to debug it again, or replace the default assert handler with their own.
I mean that if debugger not attached that I prefer old horrible dialog than app crash with record in event log
Maybe you misunderstood me, I don't oppose adding UI feedback for assertions, I just meant that as a developer I prefer breaking into the debugger which is provided by the current .NET Core default experience. For end users having UI feedback is clearly preferable over no feedback and logging into EventLog.
However if that UI feedback starts popping multiple cascading dialogs then I hope you agree that its not ideal anymore either. If this is provided by the UI framework I'm saying it should be improved instead of being a plain port of Desktop because it has a terrible experience (for both developers and end users) when the application uses timers or has custom drawing code. At the very minimum break if a debugger is already attached (fixes developer experience), but that is not the limit of what can be improved, something to prevent assert cascades caused by reentrancy would be desireable for non-developers as well.
I'm proposing the following changes over Desktop:
This assumes the effort to design an out-of-process solution is prohibitive, so I've constrained the proposal to what can be achieved in-process incrementally over Desktop behavior.
@weltkante I agree with you in most cases.
I just meant that as a developer I prefer breaking into the debugger which is provided by the current .NET Core default experience.
As a developer I prefer this too. But current .NET Core default experience didn't provide this when debugger is not attached. I perfectly satisfied with current behavior with attached debugger. But totally dissatisfied when debugger is not attached. I am not understand why you expect that developer == attached debugger and end user == detached. My thoughts on this: developer == debug, end user = release.
All above is about System.Diagnostics.Debug.Assert.
- [must have] if a debugger is attached break into the debugger so the developer can examine his program in the state of failure
- [should have] I'd suggest not showing the dialog if the developer resumes the debugger, because if he wanted to terminate he could already have done this from the debugger. Open for discussion.
- [should have] If reentrancy triggers cascading errors either hang (so user can read the error and kill the program through task manager) or crash. Do not show endlessly cascading popups by continuing the modal message loop, that is a terrible user experience for both end users and developers and sometimes can even render the machine unusable when you can't gain enough control to open task manager and kill the program. Exact behavior open for discussion.
+1.
Debug.Assert
and Trace.Assert
are both using the same handler, I don't think it can differentiate. Since different assemblies may be differently compiled it may not be possible to figure out if the assert was triggered by an assembly compiled as debug. If its possible to figure out I agree with your suggestion and launching the debugger is nice to have for debug builds. Maybe a compromise would be check if the entry point assembly (usually the exe) was compiled as debug.
.NET Core Version: .NET Core 3.1 onwards
Have you experienced this same bug with .NET Framework?: No
Problem description:
System.Diagnostics.Trace.Assert
andSystem.Diagnostics.Debug.Assert
no longer trigger a dialog prompts in desktop apps. Instead apps silently silently crash unless have a debugger attached.As discussed in https://github.com/dotnet/runtime/issues/40512, the assert behavior was changed in dotnet/coreclr#20764, and no longer raises a dialog. https://github.com/dotnet/corefx/blob/master/src/System.Diagnostics.Debug/tests/DebugTestsUsingListeners.cs#L248-L303 illustrates how to implement a custom trace listener, which may act as a basis for a fix.
Expected behavior:
No crash, and assertion dialog must appear after assert.
Minimal repro:
System.Diagnostics.Debug.Assert(false, "test");
anywhere in the code, for example: