-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Don't dispose timers if we're in our UnhandledException handler. #103937
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
Don't dispose timers if we're in our UnhandledException handler. #103937
Conversation
src/libraries/System.Runtime.Caching/src/System/Runtime/Caching/MemoryCacheStatistics.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Runtime.Caching/src/System/Runtime/Caching/MemoryCache.cs
Outdated
Show resolved
Hide resolved
…dated with Interlocked.
/backport to release/8.0 |
Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/9912799164 |
/backport to release/6.0 |
Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/9912802393 |
@StephenMolloy backporting to release/8.0 failed, the patch most likely resulted in conflicts: $ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Don't dispose timers if we're in our UnhandledException handler.
Using index info to reconstruct a base tree...
M src/libraries/System.Runtime.Caching/src/System/Runtime/Caching/MemoryCache.cs
M src/libraries/System.Runtime.Caching/src/System/Runtime/Caching/MemoryCacheStatistics.cs
Falling back to patching base and 3-way merge...
Auto-merging src/libraries/System.Runtime.Caching/src/System/Runtime/Caching/MemoryCacheStatistics.cs
Auto-merging src/libraries/System.Runtime.Caching/src/System/Runtime/Caching/MemoryCache.cs
CONFLICT (content): Merge conflict in src/libraries/System.Runtime.Caching/src/System/Runtime/Caching/MemoryCache.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 Don't dispose timers if we're in our UnhandledException handler.
Error: The process '/usr/bin/git' failed with exit code 128 Please backport manually! |
@StephenMolloy an error occurred while backporting to release/8.0, please check the run log for details! Error: git am failed, most likely due to a merge conflict. |
@StephenMolloy backporting to release/6.0 failed, the patch most likely resulted in conflicts: $ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Don't dispose timers if we're in our UnhandledException handler.
Using index info to reconstruct a base tree...
M src/libraries/System.Runtime.Caching/src/System/Runtime/Caching/MemoryCache.cs
M src/libraries/System.Runtime.Caching/src/System/Runtime/Caching/MemoryCacheStatistics.cs
Falling back to patching base and 3-way merge...
Auto-merging src/libraries/System.Runtime.Caching/src/System/Runtime/Caching/MemoryCacheStatistics.cs
Auto-merging src/libraries/System.Runtime.Caching/src/System/Runtime/Caching/MemoryCache.cs
CONFLICT (content): Merge conflict in src/libraries/System.Runtime.Caching/src/System/Runtime/Caching/MemoryCache.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 Don't dispose timers if we're in our UnhandledException handler.
Error: The process '/usr/bin/git' failed with exit code 128 Please backport manually! |
@StephenMolloy an error occurred while backporting to release/6.0, please check the run log for details! Error: git am failed, most likely due to a merge conflict. |
…er. (dotnet#103937)" This reverts commit ddc9d59.
* Revert "Don't dispose timers if we're in our UnhandledException handler. (#103937)" * MemoryCache - Remove UnhandledException handler. * MemoryCache - Remove AppDomain.DomainUnload handler as well.
Fixes #64115 (and #102666 that I want to leave open so this can be triaged from the Timer side of things as well.)
The locks causing deadlock here are not known to MemoryCache at all. They exist in Timer and ThreadPool components. But to summarize, the .Net Timer class made changes in 5.0 that reduced contention and made Timer more performant. But this also introduced multiple locks into the mechanics of Timer, which can be taken in the wrong order if an unhandled exception throws during timer processing and an
UnhandledException
handler does something with Timers.I believe the mantra has always been to
Dispose()
things that areIDisposable
. MemoryCache itself is one of these things. If somebody calls Dispose() on MemoryCache purposefully, then it's reasonable to clean up our stuff - especially any Timers we've got firing periodically. So this PR aims to maintain that behavior; even if our Dispose() actions aren't as critical to perform as they were in NetFx when they also had to clean up perf counters that are shared machine-wide.However, if we are in the process of cleaning up after an unhandled exception where we don't know if it's safe to Dispose() our Timers... I think it's safe to not do anything with the Timer, since the process will be going away shortly.
Unfortunately, we can only know if we are in our own UnhandledException handler. This won't solve any issue that arises from somebody else calling MemoryCache.Dispose() - Timer.Dispose() for that matter - in their UnhandledException handler. In those cases, I think we'd have to recommend the same approach we have had to take here with
Timer
. Don't disposeMemoryCache
in an unhandled exception handler. We still have our handler hooked up and will clean ourselves up anyway - minus the Timer of course.