diff --git a/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.cs b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.cs index 840f05dc00f856..f1ce1193e78973 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.cs @@ -1289,7 +1289,7 @@ private static void AddWrapperToReferenceTrackerHandleCache(NativeObjectWrapper private sealed class RcwCache { - private readonly Lock _lock = new Lock(useTrivialWaits: true); + private readonly ReaderWriterLockSlim _lock = new ReaderWriterLockSlim(); private readonly Dictionary _cache = []; /// @@ -1301,7 +1301,8 @@ private sealed class RcwCache /// The proxy object currently in the cache for or the proxy object owned by if no entry exists and the corresponding native wrapper. public (NativeObjectWrapper actualWrapper, object actualProxy) GetOrAddProxyForComInstance(IntPtr comPointer, NativeObjectWrapper wrapper, object comProxy) { - lock (_lock) + _lock.EnterWriteLock(); + try { Debug.Assert(wrapper.ProxyHandle.Target == comProxy); ref GCHandle rcwEntry = ref CollectionsMarshal.GetValueRefOrAddDefault(_cache, comPointer, out bool exists); @@ -1336,32 +1337,63 @@ private sealed class RcwCache // Return our target object. return (wrapper, comProxy); } + finally + { + _lock.ExitWriteLock(); + } } public object? FindProxyForComInstance(IntPtr comPointer) { - lock (_lock) + _lock.EnterReadLock(); + try { - if (_cache.TryGetValue(comPointer, out GCHandle existingHandle)) + if (!_cache.TryGetValue(comPointer, out GCHandle existingHandle)) { - if (existingHandle.Target is NativeObjectWrapper { ProxyHandle.Target: object cachedProxy }) - { - // The target exists and is still alive. Return it. - return cachedProxy; - } + // No entry in the cache. + return null; + } + if (existingHandle.Target is NativeObjectWrapper { ProxyHandle.Target: object cachedProxy }) + { + // The target exists and is still alive. Return it. + return cachedProxy; + } + // The target was collected, so we need to remove the entry from the cache. + // We'll do this in a write lock after we exit the read lock. + // We don't use an upgradeable lock here as only one thread can hold an upgradeable lock at a time, + // effectively eliminating the benefit of using a reader-writer lock. + } + finally + { + _lock.ExitReadLock(); + } - // The target was collected, so we need to remove the entry from the cache. + _lock.EnterWriteLock(); + try + { + // Someone else could have removed the entry or added a new one in the time + // between us releasing the read lock and acquiring the write lock. + if (_cache.TryGetValue(comPointer, out GCHandle existingHandle) + && existingHandle.Target is null) + { + // There's still a dead entry in the cache, + // remove it. _cache.Remove(comPointer); existingHandle.Free(); } - - return null; } + finally + { + _lock.ExitWriteLock(); + } + + return null; } public void Remove(IntPtr comPointer, NativeObjectWrapper wrapper) { - lock (_lock) + _lock.EnterWriteLock(); + try { // TryGetOrCreateObjectForComInstanceInternal may have put a new entry into the cache // in the time between the GC cleared the contents of the GC handle but before the @@ -1376,6 +1408,10 @@ public void Remove(IntPtr comPointer, NativeObjectWrapper wrapper) cachedRef.Free(); } } + finally + { + _lock.ExitWriteLock(); + } } }