Don't dispose timers if we're in our UnhandledException handler. (#10…

…3937) * Don't dispose timers if we're in our UnhandledException handler. * Account for non-fatal exceptions. Safer 'disposing' of timer handle ref. * Volatile not necessary for single read of 32-bit int which is only updated with Interlocked.
dotnet · Jul 11, 2024 · ddc9d59 · ddc9d59
1 parent 39b6512
commit ddc9d59
Show file tree

Hide file tree

Showing 3 changed files with 24 additions and 2 deletions.
diff --git a/src/libraries/System.Runtime.Caching/src/System/Runtime/Caching/MemoryCache.cs b/src/libraries/System.Runtime.Caching/src/System/Runtime/Caching/MemoryCache.cs
@@ -40,6 +40,7 @@ public class MemoryCache : ObjectCache, IEnumerable, IDisposable
         private bool _throwOnDisposed;
         private EventHandler _onAppDomainUnload;
         private UnhandledExceptionEventHandler _onUnhandledException;
+        private int _inUnhandledExceptionHandler;
 #if NET
         [UnsupportedOSPlatformGuard("browser")]
         private static bool _countersSupported => !OperatingSystem.IsBrowser();
@@ -238,14 +239,19 @@ private void OnAppDomainUnload(object unusedObject, EventArgs unusedEventArgs)
             Dispose();
         }
 
+        internal bool InUnhandledExceptionHandler => _inUnhandledExceptionHandler > 0;
         private void OnUnhandledException(object sender, UnhandledExceptionEventArgs eventArgs)
         {
+            Interlocked.Increment(ref _inUnhandledExceptionHandler);
+
             // if the CLR is terminating, dispose the cache.
             // This will dispose the perf counters
             if (eventArgs.IsTerminating)
             {
                 Dispose();
             }
+
+            Interlocked.Decrement(ref _inUnhandledExceptionHandler);
         }
 
         private static void ValidatePolicy(CacheItemPolicy policy)

diff --git a/src/libraries/System.Runtime.Caching/src/System/Runtime/Caching/MemoryCacheStatistics.cs b/src/libraries/System.Runtime.Caching/src/System/Runtime/Caching/MemoryCacheStatistics.cs
@@ -340,8 +340,19 @@ public void Dispose()
                     GCHandleRef<Timer> timerHandleRef = _timerHandleRef;
                     if (timerHandleRef != null && Interlocked.CompareExchange(ref _timerHandleRef, null, timerHandleRef) == timerHandleRef)
                     {
-                        timerHandleRef.Dispose();
-                        Dbg.Trace("MemoryCacheStats", "Stopped CacheMemoryTimers");
+                        // If inside an unhandled exception handler, Timers may be succeptible to deadlocks. Use a safer approach.
+                        if (_memoryCache.InUnhandledExceptionHandler)
+                        {
+                            // This does not stop/dispose the timer. But the callback on the timer is protected by _disposed, which we have already
+                            // set above.
+                            timerHandleRef.FreeHandle();
+                            Dbg.Trace("MemoryCacheStats", "Freed CacheMemoryTimers");
+                        }
+                        else
+                        {
+                            timerHandleRef.Dispose();
+                            Dbg.Trace("MemoryCacheStats", "Stopped CacheMemoryTimers");
+                        }
                     }
                 }
                 while (_inCacheManagerThread != 0)

diff --git a/src/libraries/System.Runtime.Caching/src/System/Runtime/Caching/SRef.cs b/src/libraries/System.Runtime.Caching/src/System/Runtime/Caching/SRef.cs
@@ -59,6 +59,11 @@ public T Target
         public void Dispose()
         {
             Target.Dispose();
+            FreeHandle();
+        }
+
+        internal void FreeHandle()
+        {
             // Safe to call Dispose more than once but not thread-safe
             if (_handle.IsAllocated)
             {