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

[cesarblum]

Redirecting issue https://github.com/aspnet/dnx/issues/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 (https://github.com/dotnet/coreclr/issues/10652). Was the issue here actually addressed, though (regarding getting an option to attach a debugger, etc.)?

[danmosemsft]

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?

[danmosemsft]

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.

[danmosemsft]

@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.

[danmosemsft]

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.

```C#
public static partial class Debug
{
public static event EventHandler 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.

[danmosemsft]

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.

[danmosemsft]

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.

[jkotas]

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

There are 3 different cases:

1. handler added by a library
2. handler provided by an application
3. handler added by a library and handler provided by an application

The check for null cannot differentiate between cases 1. and 2. I think there would need to be an extra property to be able to differentiate between the two.

Alternatively, we can make the handler just a simple delegate. The last one set wins. No builtin support for chaining the handlers.

[ericstj]

I don’t see 1&2 as different. Application is just another library. If an app model wants to differentiate it needs to expose api or behavior to make it so. That’s what I was getting at with set error mode. I'd also be ok with a simple delegate property with last-in-wins.

[noahfalk]

I've been poking around a bit and there are plentiful resources that show .NET developers expect Debug.Assert behavior is customized via TraceListeners (books, magazine articles, blogs, web forum posts, sample code projects, MSDN docs ...). While I enjoy the simplicity of @ericstj's approach if we were building anew, it feels very odd to me to preserve TraceListeners as part of the framework but eliminate their role as the Debug.Assert customization mechanism. Are there compelling reasons not to continue using TraceListeners that I am overlooking?

If not I'd suggest adding the following design goals to better match the expectations from existing .NET code, docs, and developers:
1) In retail code, Trace.Assert and Debug.Assert have identical behavior
2) A library can add a TraceListener to the Trace.Listeners collection and its Fail() method will be invoked on all failed asserts
3) Removing the DefaultTraceListener from the Trace.Listeners collection or setting DefaultTraceListener.AssertUIEnabled = false are effective mechanisms to suppress any default GUI assert pop-up

I understand adding Debug.Listeners collection would be hard given the new layering so perhaps we continue to omit it? It is awkward for anyone wanting very high source/binary compat, but if we honor the investments people already made in creating custom TraceListeners then developers can at least relate to this as a subset of desktop functionality rather than as a redesign. Guidance to find/replace "Debug.Listeners" with "Trace.Listeners" for porting code doesn't feel too onerous and we could continue to improve upon it in the future given feedback.

Thoughts?

[ericstj]

Are there compelling reasons not to continue using TraceListeners that I am overlooking?

IIRC (channeling the folks who previously had opinions about this stuff) TraceListeners were just one of many tracing technologies that came/went in the desktop framework. The problem with bringing them back as the debug assert customization is that it opts folks into that trace technology when they otherwise might want to use something else (or in fact, the rest of their app already uses something else). I've also got some old docs that I'll email you about this.

I do see value in bringing back Debug listeners, but I wouldn't do so by merging them into Trace.Listeners. Something about bringing back a technology for compatibility but changing it in this way sits wrong with me. Instead I would bring it back and just create a new static property for listeners. Trace.DebugListeners, or DebugTrace.Listeners, etc. At least that way folks have a simple port rather than to figure out how to differentiate debug and trace messages.

[noahfalk]

Thanks @ericst !

The problem with bringing them back as the debug assert customization is that it opts folks into that trace technology when they otherwise might want to use something else

My primary concern was to support TraceListeners by default for those who wanted to use it, but I'm open to additional options for supporting customers that want a different scheme if we think its worth the cost. I don't think supporting the former case necessarily interferes with doing better in the latter case. Assuming however that we did make TraceListener the only framework extensibility point, if I was a customer who wanted to use my own way of customizing assert behavior I would write something like this to abstract myself away from it:

Trace.Listeners.Clear();
Trace.Listeners.Add(new NoAssertDefaultTraceListener());
Trace.Listeners.Add(new MyTraceListenerAdapter());

public class NoAssertDefaultTraceListener : DefaultTraceListener
    {
        public override void Fail(string message, string detailMessage)
        {
        }
    }

class MyTraceListenerAdapter : TraceListener
{
        public override void Write(string message) { }
        public override void WriteLine(string message) { }
        public override void Fail(string message, string detailMessage)
        {
            MyCustomAssertLogic.AssertionFailed(message, detailMessage);
        }
}

Does that seem like a fair representation or am I missing some additional entanglements? Actual online examples are typically simpler, either ((DefaultTraceListener)Debug.Listeners[0]).AssertUiEnabled=falseor Trace.Listeners.Clear(). This way is probably the most complex, eliminating all default assert behavior while leaving the remainder of DefaultTraceListener logging intact.

If that code was only thing I needed to write I might think "hmm, not that elegant, but I'm glad the random sample code I copy/pasted from the 10 year old desktop example worked and now I am moving on." I think I'd feel more aggravated if I found that code and used it in a .Net Core app today. Everything written there compiles fine, but then at runtime it has different behavior than desktop .NET running the same code. Searching the web to try and find the .Net Core way of doing it just turns up all the desktop examples which don't work (hopefully if we did create new extensions in the future we'd document it well enough that search engines would give better results, but its hard to predict).

I do see value in bringing back Debug listeners, but I wouldn't do so by merging them into Trace.Listeners.

My understanding was that on desktop they are already identical, is that inaccurate? I believe the implementation of Debug.Listeners and Trace.Listeners is:

return TraceInternal.Listeners

Something about bringing back a technology for compatibility but changing it in this way sits wrong with me.

I agree, it was not my intent to be proposing a change. If I inadvertently did then I'd revise my recommendation after I get the correct understanding of the desktop behavior.

[ericstj]

My understanding was that on desktop they are already identical

Indeed. Surprising we did that but you are correct. For some reason I thought we maintained exclusive collections but I was wrong. Debug is pretty much a copy of Trace except with Debug condition attributes sprinkled around. I like the suggestion of using Trace 👍

Were you thinking you'd have Debug access Trace through reflection, to avoid bringing down the type closure into CoreLib? (I did a quick experiment and found that the current assembly closure for Trace adds about 2.0 MB worth of IL compared to an app just using Debug).

If reflection, what should the behavior be for folks who don't have Trace assembly, and what should the behavior be for the folks who didn't customize the trace.listeners collection?

What are your thoughts for the other members on Debug that worked before on Desktop, should those defer to Trace as well?

These are all solvable problems but things we'll need to think about.

[noahfalk]

I hadn't gone too deep into the details, but this would be my strawman for it...

Were you thinking you'd have Debug access Trace through reflection, to avoid bringing down the type closure into CoreLib? (I did a quick experiment and found that the current assembly closure for Trace adds about 2.0 MB worth of IL compared to an app just using Debug).

Yeah - We'd use refection either to invoke methods directly, or to construct delegates that we call through on future invocations for better performance.

If reflection, what should the behavior be for folks who don't have Trace assembly,

If the scenario is marketed to users as minified but functionally equivalent (such as assembly level .NET Core tree shaking) then Debug/Trace default behavior should not depend upon the presence of an optional component. A few options that I could imagine to achieve that:
1) Trace assembly is classified as non-optional if Debug API is in use
2) Minimal closure to implement DefaultTraceListener is moved to a non-optional assembly (S.P.C?)
3) Some type that implements the key behaviors of DefaultTraceListener is implemented in a non-optional assembly (S.P.C?) with with a very limited API. DefaultTraceListener in Trace assembly becomes a wrapper for it that preserves the pre-existing API. Debug APIs know how to call the new type directly if Trace assembly is missing.

On the other hand if the scenario is marketed to users as an alternate variation of a managed framework that doesn't implement .NET Standard 2 then I think we have a more free hand to implement new default behavior and extensibility mechanisms.

and what should the behavior be for the folks who didn't customize the trace.listeners collection?

I'd match the desktop default, which is one instance of DefaultTraceListener is placed in the collection when it is initialized.

What are your thoughts for the other members on Debug that worked before on Desktop, should those defer to Trace as well?

Yeah

[ericstj]

I actually think you need to go into more detail to vet any of these suggestions since past issues from folks like Wes and Pallavi found that the hard problems were in the details. Granted some things have changed since those original assessments, but we need to flesh out a proposal to see if that's the case.

WRT to minified vs alternate: it isn't one or the other, it could be both. I think minified is the idea behind bringing back trace listener linkage. If we do this I'd expect someone that was explicitly using it gets that behavior. Alternate is also possible to still provide a way to achieve similar goals without relying on TraceListener infra.

I'd match the desktop default, which is one instance of DefaultTraceListener is placed in the collection when it is initialized.

Would you choose for that DefaultTraceListener to exhibit the old desktop behavior or existing .NETCore behavior?

What are your thoughts for the other members on Debug that worked before on Desktop, should those defer to Trace as well?

Yeah

I like the consistency of forwarding everything from Debug, though it may highlight some more challenges like the previous comment (desktop vs .netcore).

[noahfalk]

I actually think you need to go into more detail to vet any of these suggestions since past issues from folks like Wes and Pallavi found that the hard problems were in the details.

If its helpful I did a C# example here: https://github.com/noahfalk/corefx/commit/dd861d631a4d6e8188d74c5c17905868bbefa41a

That said there is always the next level of detail and plenty of things I did in my example were somewhat arbitrary. I probably won't be able to spend too much more time at this, so if you guys determine there are still too many details unaccounted for or its papering over hard problems I'm happy to defer to your judgement. I think my main opinion is just that it seemed odd to put Trace into .Net Standard but then not give it the same behavior that Desktop gave it so I was trying to improve that situation. If we could do better on that it would be great, if not, fair enough : )

Would you choose for that DefaultTraceListener to exhibit the old desktop behavior or existing .NETCore behavior?

For everything other than assert, the behavior is already virtually identical. Debug.WriteLine() has a bug in it where it doesn't respect indentation on first invocation but that could be easily fixed.

For assert I think this moves back to the original question of the thread, although not a part I was attempting to weigh in on with my suggestion. Both my example and the earlier AssertFailed event never actually defined what the default UI behavior would be and I think of that default as its own separate problem to be solved. I don't think parity with desktop is critical here because it is a user-facing rather than programtic interface. As best I can tell people liked the existing behavior so that is a good point in its favor. There are also implementation considerations that maybe its hard to replicate desktop behavior and we need accept something easier to do xplat. Then there were some assumptions that maybe people would want different things for new AppModels, hence making it extensible. I imagine whatever we pick will be at least somewhat contentious, but without having any user feedback or data on it my initial position would be "make the default as close to existing desktop behavior as we can and then let people who don't like it change it."

[jkotas]

make the default as close to existing desktop behavior as we can and then let people who don't like it change it

The default for .NET Core console apps has to stay no UI. Changing it back to pop up winforms assert dialog by default would be a major step backward.

As best I can tell people liked the existing behavior so that is a good point in its favor.

The default desktop behavior was optimized for classic winforms apps. For everything but WinForms, I think it has been an unwritten best practice to suppress default behavior and replace it with something else. I have seen a lot of instance like https://github.com/mono/monodevelop/blob/55fef4c2f249f1fa043543521a3adaa8082d5b22/main/src/core/MonoDevelop.Core/MonoDevelop.Core/LoggingService.cs#L111 (or equivalent config file setting).

[ericstj]

I like the API suggested and added a few comments to your commit.

I agree with @jkotas that we shouldn't make the default show UI on .NETCore. We can make winforms and WPF opt into that. We can, however, preserve TraceListener linkage by default if present.

[noahfalk]

I value the user data more than my whims ; ) No UI dialog by default is fine by me.

[Anipik]

@noahfalk can i grab this one ?

[noahfalk]

@Anipik - I've always considered @ericstj and @danmosemsft being in charge on this one, not sure if they already had a plan in mind or if they want to hand it off?

[Anipik]

@danmosemsft told me to ask you if i can continue with your provided example code

[danmosemsft]

@noahfalk, @Anipik is just checking you aren't planning on taking this further

[noahfalk]

Ah I didn't realize you had already talked : ) Nope, I wasn't planning on taking that code any further so I'm very appreciative to have you step in!

[danmosemsft]

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.

[danmosemsft]

@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.