-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Use [DebuggerDisableUserUnhandledExceptions] #2254
base: dotnet-vnext
Are you sure you want to change the base?
Use [DebuggerDisableUserUnhandledExceptions] #2254
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dotnet-vnext #2254 +/- ##
================================================
- Coverage 85.29% 85.28% -0.02%
================================================
Files 312 312
Lines 7426 7426
Branches 1118 1119 +1
================================================
- Hits 6334 6333 -1
Misses 746 746
- Partials 346 347 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
When I try this locally, in the sync case, I am seeing the exception caught in the various lambdas such as this one:
|
I am seeing something similar in async where it is the lambda's in ResiliencePipeline.Async.cs that is actually calling the user's delegate and catching the exception, so that is where the |
Thanks for hunting those down. I didn't do a proper dig through the code (clearly) to find the closest place to the user's original code. Tomorrow I'll look at moving the attributes around. |
Using a search for Are there any other heuristics I can use to find likely useful places for this? Otherwise I'll need to rely on you both to see if it's behaving how you'd expect 😄 |
Searching for Once you have found a method like that, then the follow up question is if there is a way for the exception being caught to be one which (1) is from user code -and- (2) should usually be ignored as Polly is very likely to retry the operation. If you want to be able to test this work, Visual Studio 2022 version 17.12 preview 1 will support You can also somewhat test things today thusly --
Sorry if this is obvious, but the basic way to test is:
If you are using 17.11 (async) / 17.11 or earlier (sync) you should see the debugger break "User-Unhandled". You want to make sure you have added If you are using 17.12, you want to see that the debugger no longer breaks user unhandled. I hope this helps and thanks so much for working on this! |
@gregg-miskelly Thanks for the information. I already have previews installed, so I think I'll just wait until I get updated to 17.12 and then run out test suites inside the IDE in Release mode and see where the debugger breaks (or not). |
FYI 17.12 preview 1 was released today |
75bafbf
to
483a4e5
Compare
@gregg-miskelly I was having a play around with this branch in 17.11 and 17.12 and I didn't really notice any difference with and without the attribute. I'm probably doing something stupid, but I followed the instructions you posted the other week. Do you have a simple GitHub repo that I could clone that shows the desired behavioural difference so I can check I've got things configured properly before I re-test with Polly? |
@martincostello I am sorry. Unfortunately, the preview 1 version of this work has a rather large bug that we are working to fix for preview 2. If you want to work around it rather than waiting for preview 2 --
|
- Build with the .NET 9 (preview 6) SDK. - Add tests TFM for `net9.0`. - Remove tests for `net6.0` TFM. - Run workflows on PRs to `dotnet-vnext` branch. - Only audit direct NuGet dependencies. - Fix IDE0022 warning. - Temporarily disable some tests that are failing in GitHub Actions.
Disable TFM parallelism for `dotnet test` in the Polly.Core project on Windows to fix two failing tests. See https://learn.microsoft.com/dotnet/core/whats-new/dotnet-9/sdk#run-tests-in-parallel.
Update .NET SDK to version 9.0.100-preview.7.24407.12. --- updated-dependencies: - dependency-name: Microsoft.NET.Sdk dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: polly-updater-bot[bot] <138034000+polly-updater-bot[bot]@users.noreply.github.com> Co-authored-by: polly-updater-bot[bot] <138034000+polly-updater-bot[bot]@users.noreply.github.com>
Update NuGet packages for .NET 9 preview 7.
Update Microsoft.Extensions.TimeProvider.Testing to .NET 9 preview 7.
3436c6b
to
51e0bf2
Compare
- Build with the .NET 9 (preview 6) SDK. - Add tests TFM for `net9.0`. - Remove tests for `net6.0` TFM. - Run workflows on PRs to `dotnet-vnext` branch. - Only audit direct NuGet dependencies. - Fix IDE0022 warning. - Temporarily disable some tests that are failing in GitHub Actions.
- Add usage of `[DebuggerDisableUserUnhandledExceptions]` in `StrategyHelper` and `RetryEngine` to avoid newer versions of the Visual Studio debugger for breaking for exceptions we are intentionally handling. - Add copy of `[DebuggerDisableUserUnhandledExceptions]` for use in downlevel versions of .NET that do not contain the attribute. - Add `net9.0` TFM to potentially include to use the built-in definition of the attribute. - Add `net8.0` TFM to Polly to resolve dependency resolution issues in the Snippets project.
Remove redundant usage of `[DebuggerNonUserCode]`.
Add `[DebuggerDisableUserUnhandledExceptions]` to all the methods containing `catch (Exception...)` that appear to be executing the user's callback.
Not required due to lack of catch.
Remove redundant attributes.
Do not include DebuggerDisableUserUnhandledExceptionsAttribute for .NET 9 as it is built-in as of preview 7.
Suppress S3236 warnings.
Suppress S3236 warning.
a374368
to
14b80f0
Compare
Re-enable tests.
Just come back to this, and I'm still failing to see a difference in behaviour with My understanding was that with Polly 8.4.0, Visual Studio would break here, but with the changes from this branch it wouldn't. It practice, I'm seeing the same behaviour with both - Visual Studio doesn't break and I just see the exceptions logged to the console. I'm consuming Polly via a local NuGet package source using the I feel like I must be missing something here. Could you share a simple GitHub project that's set up in a way that should illustrate the difference? Then I can pull this branch's code into it and verify if I see the same. |
Seems to work with `net9.0` now.
Accidentally removed.
Your understanding of the expected behavior is correct. What version of the .NET Runtime is the process you are debugging using? For async-user unhandled, you need to modify the client to use .NET 9. Also, if you are on .NET 9, do you have Just My Code enabled? |
Ah, that's probably what I missed - I didn't update the sample to .NET 9 and was just using the CI build for net8.0 with the copy of the attribute in it. I'll update the host project tomorrow and try again. |
Test changes from App-vNext/Polly#2254.
Yep, that's what it was. Using App-vNext/Polly-Samples#107, the debugger breaks here when an exception is thrown now when the app is targeting That suggests to me that I'm missing one or more attributes as it should be being ignored and not breaking as it's being retried? |
Agreed. If show external code, and inspect the frames on the call stack, you can hopefully find the frame with the |
Test changes from App-vNext/Polly#2254.
Add `[DebuggerDisableUserUnhandledExceptions]` to `StrategyHelper`.
@gregg-miskelly I thought I'd found one I'd missed, but ingesting that change still doesn't seem to have changed the behaviour - would you mind cloning App-vNext/Polly-Samples#107 locally and taking a look to see what I'm missing? |
@martincostello Thanks for continuing to work on this! The attribute usage in Polly is correct. The issue is that there is a non-user async method on the stack above Polly's catch handler (Microsoft.EntityFrameworkCore.dll!Microsoft.EntityFrameworkCore.ExecutionStrategyExtensions.ExecuteAsync.AnonymousMethod__4_0). Any async method will have a compiler-generated catch handler. In hindsight this should have been obvious to us, but we didn't think about those compiler-generated catch blocks when we added Thanks again and sorry for all the problems. |
No worries, that's the point of testing 😄 Let me know when a fix makes it into a public preview of 17.12 and then I'll try again. |
One more question: do we need to use the attribute in methods with Running the other demos in the same project, there's another one that is breaking in the debugger that has this method on the stack - do we need to mark those methods with the attribute too, or is this similar to the EFCore case? |
No, you only need to worry about
It looks like the same issue as the EFCore case. |
e548cc6
to
7b6aa5a
Compare
FYI: The fix to better handle intermediate async methods made it in time for VS 17.12 Preview 3. |
[DebuggerDisableUserUnhandledExceptions]
inStrategyHelper
andRetryEngine
to avoid newer versions of the Visual Studio debugger for breaking for exceptions we are intentionally handling.[DebuggerDisableUserUnhandledExceptions]
for use in downlevel versions of .NET that do not contain the attribute.net9.0
TFM to potentially include to use the built-in definition of the attribute.net8.0
TFM to Polly to resolve dependency resolution issues in the Snippets project.Break for Async User-Unhandled exceptions in the Visual Studio Debugger
/cc @halter73 @gregg-miskelly