From 55231a1fb93e1ba522bd958cc7ecd7a1a76e557c Mon Sep 17 00:00:00 2001 From: Steve Molloy Date: Mon, 24 Jun 2024 16:22:21 -0700 Subject: [PATCH 1/3] Don't dispose timers if we're in our UnhandledException handler. --- .../src/System/Runtime/Caching/MemoryCache.cs | 4 ++++ .../System/Runtime/Caching/MemoryCacheStatistics.cs | 10 ++++++++-- 2 files changed, 12 insertions(+), 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 index 75053fdbbe52df..33edea8d2b7972 100644 --- 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,8 +239,11 @@ private void OnAppDomainUnload(object unusedObject, EventArgs unusedEventArgs) Dispose(); } + internal bool InUnhandledExceptionHandler => _inUnhandledExceptionHandler == 1; private void OnUnhandledException(object sender, UnhandledExceptionEventArgs eventArgs) { + Interlocked.Exchange(ref _inUnhandledExceptionHandler, 1); + // if the CLR is terminating, dispose the cache. // This will dispose the perf counters if (eventArgs.IsTerminating) 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 index dba747c4acdfe5..7a10159397474d 100644 --- 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,14 @@ public void Dispose() GCHandleRef timerHandleRef = _timerHandleRef; if (timerHandleRef != null && Interlocked.CompareExchange(ref _timerHandleRef, null, timerHandleRef) == timerHandleRef) { - timerHandleRef.Dispose(); - Dbg.Trace("MemoryCacheStats", "Stopped CacheMemoryTimers"); + // #64115 and #102666 - It's not safe to dispose timers in our unhandled exception handler. But if we're in the unhandled + // exception handler, then the process is going away. We don't really need to clean up timers. (Interactions of + // processes/AppDomains/Timers/PerfCounters/etc were different in NetFx from which this package is derived.) + if (!_memoryCache.InUnhandledExceptionHandler) + { + timerHandleRef.Dispose(); + Dbg.Trace("MemoryCacheStats", "Stopped CacheMemoryTimers"); + } } } while (_inCacheManagerThread != 0) From c35bc940c0e994be268154f0cef6f9bbe5ec6599 Mon Sep 17 00:00:00 2001 From: Steve Molloy Date: Thu, 27 Jun 2024 15:21:07 -0700 Subject: [PATCH 2/3] Account for non-fatal exceptions. Safer 'disposing' of timer handle ref. --- .../src/System/Runtime/Caching/MemoryCache.cs | 6 ++++-- .../System/Runtime/Caching/MemoryCacheStatistics.cs | 13 +++++++++---- .../src/System/Runtime/Caching/SRef.cs | 5 +++++ 3 files changed, 18 insertions(+), 6 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 index 33edea8d2b7972..16500c9d613d84 100644 --- a/src/libraries/System.Runtime.Caching/src/System/Runtime/Caching/MemoryCache.cs +++ b/src/libraries/System.Runtime.Caching/src/System/Runtime/Caching/MemoryCache.cs @@ -239,10 +239,10 @@ private void OnAppDomainUnload(object unusedObject, EventArgs unusedEventArgs) Dispose(); } - internal bool InUnhandledExceptionHandler => _inUnhandledExceptionHandler == 1; + internal bool InUnhandledExceptionHandler => Volatile.Read(ref _inUnhandledExceptionHandler) > 0; private void OnUnhandledException(object sender, UnhandledExceptionEventArgs eventArgs) { - Interlocked.Exchange(ref _inUnhandledExceptionHandler, 1); + Interlocked.Increment(ref _inUnhandledExceptionHandler); // if the CLR is terminating, dispose the cache. // This will dispose the perf counters @@ -250,6 +250,8 @@ private void OnUnhandledException(object sender, UnhandledExceptionEventArgs eve { 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 index 7a10159397474d..2e2ba577b7a248 100644 --- 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,10 +340,15 @@ public void Dispose() GCHandleRef timerHandleRef = _timerHandleRef; if (timerHandleRef != null && Interlocked.CompareExchange(ref _timerHandleRef, null, timerHandleRef) == timerHandleRef) { - // #64115 and #102666 - It's not safe to dispose timers in our unhandled exception handler. But if we're in the unhandled - // exception handler, then the process is going away. We don't really need to clean up timers. (Interactions of - // processes/AppDomains/Timers/PerfCounters/etc were different in NetFx from which this package is derived.) - if (!_memoryCache.InUnhandledExceptionHandler) + // 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"); 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 index 0b6f90c7005575..ac017ceaa8e719 100644 --- 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) { From 3387f26d56fa5ad3385a9d812921ed461c25e11e Mon Sep 17 00:00:00 2001 From: Steve Molloy Date: Fri, 28 Jun 2024 20:54:41 -0700 Subject: [PATCH 3/3] Volatile not necessary for single read of 32-bit int which is only updated with Interlocked. --- .../src/System/Runtime/Caching/MemoryCache.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 index 16500c9d613d84..2c871f3f545326 100644 --- a/src/libraries/System.Runtime.Caching/src/System/Runtime/Caching/MemoryCache.cs +++ b/src/libraries/System.Runtime.Caching/src/System/Runtime/Caching/MemoryCache.cs @@ -239,7 +239,7 @@ private void OnAppDomainUnload(object unusedObject, EventArgs unusedEventArgs) Dispose(); } - internal bool InUnhandledExceptionHandler => Volatile.Read(ref _inUnhandledExceptionHandler) > 0; + internal bool InUnhandledExceptionHandler => _inUnhandledExceptionHandler > 0; private void OnUnhandledException(object sender, UnhandledExceptionEventArgs eventArgs) { Interlocked.Increment(ref _inUnhandledExceptionHandler);