Skip to content
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

MemoryCache - Remove unhandled exception handler #105853

Conversation

StephenMolloy
Copy link
Member

Re-fixes #64115 in a different way from the previous fix.

The locks causing deadlock here are not known to MemoryCache at all. They exist in Timer and ThreadPool components. But the unhandled exception handler in MemoryCache doesn't do anything other than dispose the stats timer in .Net Core. (In NetFx, the deadlock risk wasn't the same, and the unhandled exception handler also had to clean up perf counters that could live beyond the process lifetime.) So this exception handler isn't really needed anymore.

@@ -39,8 +39,6 @@ public class MemoryCache : ObjectCache, IEnumerable, IDisposable
private bool _useMemoryCacheManager = true;
private bool _throwOnDisposed;
private EventHandler _onAppDomainUnload;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the AppDomainUnload event handler be deleted for the same reason?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AppDomainUnload doesn't run the same danger of being called unexpectedly on a stack that might be holding locks or be in some other inconsistent state. So I don't think it's a danger here. Since System.Runtime.Caching is simply a bridge package to allow folks to migrate from NetFx to .Net, I tend to approach with a servicing mentality and only make the changes that are necessary. (Also, the 1st party customer affected by this is still targeting 6.0 with some of their services, so this actually is servicing as we will want to backport whatever we do here.)

I can add another commit if you guys disagree with the conservative approach.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have constant trickle of shutdown bugs (hangs or crashes) caused by unnecessary code running during shutdown. I think it is just a matter of time until somebody hits a problem with the AppDomainUnload handler.

this actually is servicing as we will want to backport whatever we do here.

It is fine to do a minimal backport that just fixes the customer issue for servicing and do more complete fix in main. It is often how the fixes for the shutdown bugs look like. For example:

@StephenMolloy StephenMolloy requested a review from mconnew August 6, 2024 00:22
@StephenMolloy StephenMolloy marked this pull request as ready for review August 9, 2024 21:11
@StephenMolloy StephenMolloy merged commit bd29323 into dotnet:main Aug 9, 2024
84 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Sep 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UnhandledExceptionEventHandler in MemoryCache can cause a deadlock.
3 participants