Debug.Assert(false) does not behave as expected compared to full CLR

[cesarblum]

Redirecting issue aspnet/dnx#2778

This is what we see in full CLR:

CoreCLR does not give the user the option to ignore the problem or to attach a debugger to the process.

[mellinoe]

Obviously it will be impossible to support this exactly on all platforms, but I wonder if we can let you somehow opt-in to this kind of behaviour, as it is quite a bit more useful than the current process termination.

[weshaggard]

Yes this one is unfortunate but we don't have a good way to pop up a UI at this low level in all the platforms we are running on. It is something we should try and figure out some better experience around however.

[noahfalk]

@Djuffin and @mikem8361 you guys want to think about this a bit? Offhand this seems like a reasonable place to callback to our host and let it set the policy.

[ellismg]

@noahfalk IIRC, that's what we ended up doing in Silverlight. I think that Debug.Assert would send a message to the host and they would decide how to handle the UI.

[noahfalk]

Unsurprisingly it sounds like Debugger.Break has similar platform dependencies. Probably whatever we do for assert will be closely related to our handling of Debugger.Break and we can handle them together. If they turn out to be quite different we can file a new issue.

[karelz]

We need end-to-end design proposal on this feature.

[mellinoe]

@danmosemsft You closed this from the other PR (dotnet/coreclr#10652). Was the issue here actually addressed, though (regarding getting an option to attach a debugger, etc.)?

[danmoseley]

My assumption (bad?) was that we never expect CoreCLR to be able to throw UI so this would never be possible.

Thinking about it though, I could imagine environment variables or somesuch "COMPlus_Attach_On_Nth_Assert" "COMPlus_Ignore_Nth_Assert". Probably there's a better idea.

@jkotas have you an idea in mind?

[danmoseley]

As long as there's a way (presumably the default setting) to avoid blocking test runs (presumably by FF) unlike in Desktop dotnet/corefx#17636

[noahfalk]

As long as there's a way (presumably the default setting) to avoid blocking test runs (presumably by FF) unlike in Desktop dotnet/corefx#17636

Triggering Environment.FailFast engages WER on Windows, and with default machine configurations that will display UI. There are various registry keys or native APIs that can be called to adjust the OS WER policy on a global/per-app/per-process basis. I'm assuming our test runs already do the right thing to suppress/log these pop-ups? The same issue would have been present before your change throwing an unhandled exception.

My assumption (bad?) was that we never expect CoreCLR to be able to throw UI so this would never be possible.

I'm not opposed in principle to the runtime (or maybe a host) displaying such a UI dialog, but the way it was done on Windows (debugger launch at least) was coupled with WER. So far from the threads it seems like only 2 or 3 people have been interested in this and the issue has been open almost two years? Designing an appropriate xplat mechanism with the right configuration knobs isn't something I'd want to embark on without more community interest and input.

I suggest we leave the issue open a bit longer in case more interest materializes after the 2.0 release, but otherwise don't do anything for now.

[danmoseley]

@noahfalk right, I was assuming that WER is suppressed on the runs -- in fact, I thought we end up with dumps via dumpling -- which is why FF would be okay.

[jkotas]

I'm not opposed in principle to the runtime (or maybe a host) displaying such a UI dialog

Agree - this should be done by registering a callback that can be used to customize the experience for given app model.

There is a prior art for it in CoreRT: https://github.com/dotnet/corert/tree/master/src/System.Private.DeveloperExperience.Console/src provides the assert dialog experience for console apps; and we have an alternative implementation of the same for .NET Native for UWP that displays the UI dialog because of it makes more sense for UWP apps.

As consequence, the UWP experience will differ between the debug (running on CoreCLR) and release (running on CoreRT). This issue is really about harmonizing the implementation between CoreCLR and CoreRT.

[jkotas]

This will be needed for .NET Core 3.0 WinForms/WPF app models.

[eerhardt]

To glom on to this scenario a little bit: when a Debug.Assert fails during a unit test, today the experience is quite poor. However, even once that vstest bug is fixed, the experience of failing fast during tests is still poor.

In my opinion, a good experience here would be:

• Fail the current test, logging the Debug.Assert message and callstack, just like if an exception is thrown.
• Continue running the other tests.
• Still getting a trx or xml test log file after the test completes is critical in order to debug the failures on CI machines.

Today, since the process is exited immediately, other tests aren't executed and I don't a trx file after the test is done.

Having a poor experience when asserts fail during tests led to ML.NET not using Debug.Assert at all. And instead, it invented its own Contracts.Assert API which has a handler just for tests. This is unfortunate IMO. We should provide the correct design in the framework to allow people to just use our APIs without having to invent their own.

[danmoseley]

Next step is design. Does anyone plan to develop a strawman? (@ericstj ?)

[noahfalk]

@tommcdon - making sure this is on your .Net Core 3.0 planning radar : )

[ericstj]

This is a rough strawman as I know folks will have contention with a static event handler but I think it makes since here given its backing a static API.

    public static partial class Debug
    {
        public static event EventHandler<AssertionFailedEventArgs> AssertionFailed;
    }

    public class AssertionFailedEventArgs : EventArgs
    {
        public AssertionFailedEventArgs(string message, string detailMessage)
        {
            Message = message;
            DetailMessage = detailMessage;
        }
        public string Message { get; }
        public string DetailMessage { get; }
    }

Design considerations:

• There should be no return value from assert. If an assert handler wishes to impact the process it must do so from within the handler (eg: throw exception, log, kill process, etc)
• There is not outer context passed to the assert. If an assert handler wishes to map back to context it must do so itself (EG: locate a logger with the test context)
• The stack trace is not provided by default since it is potentially a costly thing to create and not always needed.
• A default handler should be provided that does an appropriate action so as not to require a handler. This default handler should be replaced when the handler is provided by the application.
• A handler added by a library should not remove / replace one provided by the application, but would be called in addition to one provided by the application.
• GUI framework like WinForms and WPF should provide a default handler in app startup code. This handler should define a way to suppress the UI, for example SetErrorMode or some dedicated .NET API.

Scenarios to validate:

1. Assert for console can kill process with appropriate stack trace.
2. Assert for GUI can pop a message box to match the desktop behavior.
3. GUI pop up can be suppressed.
4. A test can respond to an assert by throwing an exception to fail the test.
5. A test can respond to an assert by logging the assert in the context of the test case that was running (eg: ThreadLocal or AsyncLocal to map back logger with state for running test case).

[jkotas]

A handler added by a library should not remove / replace one provided by the application
This default handler should be replaced when the handler is provided by the application.

@ericstj The API proposal does not seem to allow differentiating between these two cases.

[danmoseley]

Previously Debug.Assert like Debug.Write* and Debug.Fail would go to all the TraceListeners (and DefaultTraceListener would typically be hooked up to handle OutputDebugString, MessageBox, etc). We broke that connection because of layering, which means they all have fixed implementations. This issue being about making Assert more customizable, dotnet/corefx#31003 about Write.

This implementation seems to continue the assumption that TraceListeners are not involved, although there is value (useful and compatible) in Debug.XXX going through the TraceListeners. Should we instead bring back that reverse dependency somehow (reflection) and use the trace listeners mechanism to handle swapping out assert behaviors. We do after all still have this mechanism, it's just not wired to the Debug class.

[mikem8361]

Would that be more compatible with the desktop? To use TraceListeners.

[danmoseley]

We also probably want to be consistent with Trace. In .NET Core, Trace does go to TraceListeners, however the DefaultTraceListener does not have a complete implementation for Assert. It has a bool for AssertUiEnabled but it doesn't use it.
https://github.com/dotnet/corefx/blob/master/src/System.Diagnostics.TraceSource/src/System/Diagnostics/DefaultTraceListener.cs#L96

[ericstj]

@ericstj The API proposal does not seem to allow differentiating between these two cases.

Sure it does if the differentiation is happening inside debug itself. It could check for a null event handler and use default behavior.

Feel free to take or leave my suggestions, I was merely suggesting a compromise.

You could add back TraceListeners but that'd require different API if you wanted it to work with reflection, since Debug.Listeners couldn't expose a TraceListenerCollection. There are various techniques we could use to try and make that work (exposing a different base type, for instance) that could be explored if that's the direction folks want to go.

[danmoseley]

Cool. Actually @maryamariyan will do it.

[maryamariyan]

I worked locally on the prototype example provided by @noahfalk and with the difference of using base class instead of interfaces. The only consideration with that is choosing a way to share the base class between TraceListeners in corefx and Debug in coreclr.
Based on an offline talk I had with @jkotas, another idea to think about is to use delegates to make the connection between TraceListeners and Debug.Write instead.

Issue dotnet/corefx#3708 (this issue) will address Debug.Assert and dotnet/corefx#31003 will handle the Debug.Write issue.

[weshaggard]

Issue dotnet/corefx#31003 will address what we need to do to make Debug.Assert show dialogs some of the time. As already agreed earlier in this issue thread, what we already have will remain the default console behavior.

@maryamariyan I assume you meant issue https://github.com/dotnet/corefx/issues/3708 (this issue) will address Debug.Assert and dotnet/corefx#31003 will handle the Debug.Write issue, correct?

[maryamariyan]

@weshaggard yes that's correct. updated the comment accordingly.

[danmoseley]

@maryamariyan what work remains here? Is it only for Winforms/WPF to hook it up to a dialog? If so I think we can close this, and ensure they have issues in their repos to do that.