-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Debug.Assert(false) does not behave as expected compared to full CLR #15386
Comments
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. |
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. |
@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. |
@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. |
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. |
We need end-to-end design proposal on this feature. |
@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.)? |
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? |
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.
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. |
@noahfalk right, I was assuming that WER is suppressed on the runs -- in fact, I thought we end up with dumps via |
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. |
This will be needed for .NET Core 3.0 WinForms/WPF app models. |
To glom on to this scenario a little bit: when a In my opinion, a good experience here would be:
Today, since the process is exited immediately, other tests aren't executed and I don't a 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 |
Next step is design. Does anyone plan to develop a strawman? (@ericstj ?) |
@tommcdon - making sure this is on your .Net Core 3.0 planning radar : ) |
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:
Scenarios to validate:
|
@ericstj The API proposal does not seem to allow differentiating between these two cases. |
Previously 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 |
Would that be more compatible with the desktop? To use TraceListeners.
|
We also probably want to be consistent with |
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. |
Cool. Actually @maryamariyan will do it. |
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. Issue dotnet/corefx#3708 (this issue) will address Debug.Assert and dotnet/corefx#31003 will handle the Debug.Write issue. |
@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? |
@weshaggard yes that's correct. updated the comment accordingly. |
…avior. - New SetProvider returns previous DebugProvider while exchanging to new one - Will keep the s_ShowDialog and s_WriteCore delegates for tests until the API gets approved Related to: dotnet/corefx#3708, dotnet/corefx#31003
…of SetProvider. - Adding reference to System.Private.CoreLib to be able to access SetProvider, since it is not API Approved yet. - I can remove the s_ShowDialog and s_WriteCore delegates which are now used by Debug test project after API gets approved. We could use SetProvider API instead of the delegates to set up the test project. Related to: dotnet/corefx#3708, dotnet/corefx#31003
…of SetProvider. - Adding reference to System.Private.CoreLib to be able to access SetProvider, since it is not API Approved yet. - I can remove the s_ShowDialog and s_WriteCore delegates which are now used by Debug test project after API gets approved. We could use SetProvider API instead of the delegates to set up the test project. Related to: dotnet/corefx#3708, dotnet/corefx#31003
…of SetProvider. - Adding reference to System.Private.CoreLib to be able to access SetProvider, since it is not API Approved yet. - I can remove the s_ShowDialog and s_WriteCore delegates which are now used by Debug test project after API gets approved. We could use SetProvider API instead of the delegates to set up the test project. Related to: dotnet/corefx#3708, dotnet/corefx#31003
…avior. - New SetProvider returns previous DebugProvider while exchanging to new one - Will keep the s_ShowDialog and s_WriteCore delegates for tests until the API gets approved Related to: dotnet/corefx#3708, dotnet/corefx#31003
…avior. - New SetProvider returns previous DebugProvider while exchanging to new one - Will keep the s_ShowDialog and s_WriteCore delegates for tests until the API gets approved Related to: dotnet/corefx#3708, dotnet/corefx#31003
…20419) * Adding DebugProvider to help override Dialog Write and ShowDialog behavior. - New SetProvider returns previous DebugProvider while exchanging to new one - Will keep the s_WriteCore delegates for tests until the next PR to fix indentation is merged Related to: dotnet/corefx#3708, dotnet/corefx#31003 * Move ShowDialog and Write logic from Debug to DebugProvider * disabling DebugTests
…#20419) * Adding DebugProvider to help override Dialog Write and ShowDialog behavior. - New SetProvider returns previous DebugProvider while exchanging to new one - Will keep the s_WriteCore delegates for tests until the next PR to fix indentation is merged Related to: dotnet/corefx#3708, dotnet/corefx#31003 * Move ShowDialog and Write logic from Debug to DebugProvider * disabling DebugTests Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
…#20419) * Adding DebugProvider to help override Dialog Write and ShowDialog behavior. - New SetProvider returns previous DebugProvider while exchanging to new one - Will keep the s_WriteCore delegates for tests until the next PR to fix indentation is merged Related to: dotnet/corefx#3708, dotnet/corefx#31003 * Move ShowDialog and Write logic from Debug to DebugProvider * disabling DebugTests Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
- using SetProvider API to override default Write and ShowDialog behavior - Using return type of Debug.SetProvider in DebugTests - Adding comments to track limitations with corefx issue #32955 - Adding new tests showing existing bugs and discrepancies - Using DebugProvider to override ShowDialog and delegate for WriteCore - Removing s_ShowDialog delegate - Updates TraceInternal and DefaultTraceListener in corefx to make use of SetProvider. - Adding reference to System.Private.CoreLib to be able to access SetProvider, since it is not API Approved yet. Related to: dotnet/corefx#3708, dotnet/corefx#31003
- using SetProvider API to override default Write and ShowDialog behavior - Using return type of Debug.SetProvider in DebugTests - Adding comments to track limitations with corefx issue #32955 - Adding new tests showing existing bugs and discrepancies - Using DebugProvider to override ShowDialog and delegate for WriteCore - Removing s_ShowDialog delegate - Updates TraceInternal and DefaultTraceListener in corefx to make use of SetProvider. - Adding reference to System.Private.CoreLib to be able to access SetProvider, since it is not API Approved yet. Related to: dotnet/corefx#3708, dotnet/corefx#31003
…#20419) * Adding DebugProvider to help override Dialog Write and ShowDialog behavior. - New SetProvider returns previous DebugProvider while exchanging to new one - Will keep the s_WriteCore delegates for tests until the next PR to fix indentation is merged Related to: dotnet/corefx#3708, dotnet/corefx#31003 * Move ShowDialog and Write logic from Debug to DebugProvider * disabling DebugTests Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
…espectively (master) (#32963) * Update BuildTools, CoreClr to preview1-03322-02, preview1-27023-01, respectively * Wiring up Debug class with Trace Listeners. - using SetProvider API to override default Write and ShowDialog behavior - Using return type of Debug.SetProvider in DebugTests - Adding comments to track limitations with corefx issue #32955 - Adding new tests showing existing bugs and discrepancies - Using DebugProvider to override ShowDialog and delegate for WriteCore - Removing s_ShowDialog delegate - Updates TraceInternal and DefaultTraceListener in corefx to make use of SetProvider. - Adding reference to System.Private.CoreLib to be able to access SetProvider, since it is not API Approved yet. Related to: dotnet/corefx#3708, dotnet/corefx#31003 * DebugTests has an internal extensibility point for unit tests only in netcoreapp TargetGroup
…#20419) * Adding DebugProvider to help override Dialog Write and ShowDialog behavior. - New SetProvider returns previous DebugProvider while exchanging to new one - Will keep the s_WriteCore delegates for tests until the next PR to fix indentation is merged Related to: dotnet/corefx#3708, dotnet/corefx#31003 * Move ShowDialog and Write logic from Debug to DebugProvider * disabling DebugTests Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
Syncs IndentLevel and IndentSize for Debug and Trace Configure DebugTests to run with and without TraceListeners Fixes Debug indentation bugs, adds tests Related to: #3708, #31003
Syncs IndentLevel and IndentSize for Debug and Trace Configure DebugTests to run with and without TraceListeners Fixes Debug indentation bugs, adds tests Related to: #3708, #31003
Syncs IndentLevel and IndentSize for Debug and Trace Configure DebugTests to run with and without TraceListeners Fixes Debug indentation bugs, adds tests Related to: #3708, #31003
@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. |
…otnet#20419) * Adding DebugProvider to help override Dialog Write and ShowDialog behavior. - New SetProvider returns previous DebugProvider while exchanging to new one - Will keep the s_WriteCore delegates for tests until the next PR to fix indentation is merged Related to: dotnet/corefx#3708, dotnet/corefx#31003 * Move ShowDialog and Write logic from Debug to DebugProvider * disabling DebugTests
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.
The text was updated successfully, but these errors were encountered: