Make Condition to not use ConditionalWeakTable

src/coreclr/debug/daccess/dacdbiimpl.cpp

@@ -29,7 +29,6 @@
 #endif // FEATURE_COMINTEROP
 
 #include "request_common.h"
-#include "conditionalweaktable.h"
 
 #ifndef USE_DAC_TABLE_RVA
 extern "C" bool TryGetSymbol(ICorDebugDataTarget* dataTarget, uint64_t baseAddress, const char* symbolName, uint64_t* symbolAddress);
@@ -6100,15 +6099,26 @@ HRESULT STDMETHODCALLTYPE DacDbiInterfaceImpl::EnumerateMonitorEventWaitList(VMP
         if(psb == NULL)
             return hr;
 
-        FieldDesc* pConditionTableField = (&g_CoreLib)->GetField(FIELD__MONITOR__CONDITION_TABLE);
-        CONDITIONAL_WEAK_TABLE_REF conditionTable = *(DPTR(CONDITIONAL_WEAK_TABLE_REF))PTR_TO_TADDR(pConditionTableField->GetStaticAddressHandle(pConditionTableField->GetBase()));
+        // The managed Lock (if any) for this object is referenced from the sync block.
+        // If there is no Lock, there can be no Condition installed on it and therefore no waiters.
+        OBJECTHANDLE lockHandle = psb->GetLockIfExists();
+        if (lockHandle == (OBJECTHANDLE)NULL)
+            return hr;
 
+        OBJECTREF lockObj = ObjectFromHandle(lockHandle);
+        if (lockObj == NULL)
+            return hr;
 
-        OBJECTREF condition = NULL;
-        if (!conditionTable->TryGetValue(OBJECTREF(pObj), &condition))
-        {
+        // Lock._waitEventOrCondition holds either an AutoResetEvent or a Condition. Only when a
+        // Condition has been installed does the Lock have a Monitor wait list to enumerate.
+        FieldDesc* pWaitEventOrConditionField = (&g_CoreLib)->GetField(FIELD__LOCK__WAIT_EVENT_OR_CONDITION);
+        OBJECTREF condition = pWaitEventOrConditionField->GetRefValue(lockObj);
+        if (condition == NULL)
+            return hr;
+
+        PTR_MethodTable pConditionMT = CoreLibBinder::GetExistingClass(CLASS__CONDITION);
+        if (condition->GetMethodTable() != pConditionMT)
             return hr;
-        }
 
         MapSHash<TADDR, Thread*> waiterToThreadMap;
         FieldDesc* pConditionWaiterField = (&g_CoreLib)->GetField(FIELD__CONDITION__WAITERS_HEAD);

src/coreclr/vm/corelib.h

@@ -626,14 +626,14 @@ DEFINE_CLASS(OLE_AUT_BINDER,        System,                 OleAutBinder)
 END_ILLINK_FEATURE_SWITCH()
 
 DEFINE_CLASS(MONITOR,               Threading,              Monitor)
-DEFINE_FIELD(MONITOR,               CONDITION_TABLE,        s_conditionTable)
 DEFINE_METHOD(MONITOR,              SYNCHRONIZED_METHOD_ENTER, SynchronizedMethodEnter, SM_Obj_RefBool_RetVoid)
 DEFINE_METHOD(MONITOR,              SYNCHRONIZED_METHOD_EXIT,  SynchronizedMethodExit,  SM_Obj_RefBool_RetVoid)
 
 DEFINE_CLASS(LOCK,                  Threading,              Lock)
 DEFINE_FIELD(LOCK,                  OWNING_THREAD_ID,       _owningThreadId)
 DEFINE_FIELD(LOCK,                  STATE,                  _state)
 DEFINE_FIELD(LOCK,                  RECURSION_COUNT,        _recursionCount)
+DEFINE_FIELD(LOCK,                  WAIT_EVENT_OR_CONDITION,_waitEventOrCondition)
 DEFINE_METHOD(LOCK,                 CTOR,                   .ctor,                IM_RetVoid)
 DEFINE_METHOD(LOCK,                 INITIALIZE_FOR_MONITOR, InitializeForMonitor, SM_PtrLock_Int_UInt_PtrException_RetVoid)
 

src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems

@@ -1272,7 +1272,7 @@
     <Compile Include="$(MSBuildThisFileDirectory)System\Threading\CancellationTokenRegistration.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)System\Threading\CancellationTokenSource.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)System\Threading\CompressedStack.cs" />
-    <Compile Include="$(MSBuildThisFileDirectory)System\Threading\Condition.cs" Condition="'$(FeatureMono)' != 'true'" />
+    <Compile Include="$(MSBuildThisFileDirectory)System\Threading\Condition.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)System\Threading\StackCrawlMark.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)System\Threading\EventResetMode.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)System\Threading\EventWaitHandle.cs" />
@@ -2998,4 +2998,4 @@
     <Compile Include="$(MSBuildThisFileDirectory)System\Threading\Wasi\WasiPollWorld.wit.imports.wasi.io.v0_2_0.IPoll.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)System\Threading\Wasi\WasiPollWorld.wit.imports.wasi.io.v0_2_0.PollInterop.cs" />
   </ItemGroup>
-</Project>
+</Project>

src/libraries/System.Private.CoreLib/src/System/Threading/Condition.cs

@@ -1,8 +1,6 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
-#pragma warning disable 0420 //passing volatile fields by ref
-
 using System.Diagnostics;
 using System.Diagnostics.Tracing;
 
@@ -14,8 +12,7 @@ internal sealed class Waiter
         {
             public Waiter? next;
             public Waiter? prev;
-            public AutoResetEvent ev = new AutoResetEvent(false);
-            public bool signalled;
+            public readonly AutoResetEvent ev = new AutoResetEvent(false);
         }
 
         [ThreadStatic]
@@ -36,7 +33,7 @@ private static Waiter GetWaiterForCurrentThread()
                 waiter = new Waiter();
             }
 
-            waiter.signalled = false;
+            Debug.Assert(waiter.next is null && waiter.prev is null);
             return waiter;
         }
 
@@ -47,12 +44,18 @@ private static void ReleaseWaiterForCurrentThread(Waiter waiter)
         }
 
         private readonly Lock _lock;
+
+        // When condition is installed in a Lock it takes the same field as waitEvent would.
+        // If waitEvent is also needed, it is available through here.
+        internal AutoResetEvent? _waitEvent;
+
         private Waiter? _waitersHead;
         private Waiter? _waitersTail;
 
         internal Lock AssociatedLock => _lock;
 
-        private unsafe void AssertIsInList(Waiter waiter)
+        [Conditional("DEBUG")]
+        private void AssertIsInList(Waiter waiter)
         {
             Debug.Assert(_waitersHead != null && _waitersTail != null);
             Debug.Assert((_waitersHead == waiter) == (waiter.prev == null));
@@ -64,7 +67,8 @@ private unsafe void AssertIsInList(Waiter waiter)
             Debug.Fail("Waiter is not in the waiter list");
         }
 
-        private unsafe void AssertIsNotInList(Waiter waiter)
+        [Conditional("DEBUG")]
+        private void AssertIsNotInList(Waiter waiter)
         {
             Debug.Assert(waiter.next == null && waiter.prev == null);
             Debug.Assert((_waitersHead == null) == (_waitersTail == null));
@@ -74,20 +78,32 @@ private unsafe void AssertIsNotInList(Waiter waiter)
                     Debug.Fail("Waiter is in the waiter list, but should not be");
         }
 
-        private unsafe void AddWaiter(Waiter waiter)
+        // Returns true if the waiter cannot be possibly in the list.
+        // (i.e. not reachable via _waitersHead)
+        internal bool NotInList(Waiter waiter)
+        {
+            return _waitersHead != waiter && waiter.prev == null;
+        }
+
+        private void AddWaiter(Waiter waiter)
         {
             Debug.Assert(_lock.IsHeldByCurrentThread);
             AssertIsNotInList(waiter);
 
-            waiter.prev = _waitersTail;
-            waiter.prev?.next = waiter;
-
+            Waiter? tail = _waitersTail;
+            waiter.prev = tail;
+            if (tail is null)
+            {
+                _waitersHead = waiter;
+            }
+            else
+            {
+                tail.next = waiter;
+            }
             _waitersTail = waiter;
-
-            _waitersHead ??= waiter;
         }
 
-        private unsafe void RemoveWaiter(Waiter waiter)
+        private void RemoveWaiter(Waiter waiter)
         {
             Debug.Assert(_lock.IsHeldByCurrentThread);
             AssertIsInList(waiter);
@@ -114,7 +130,7 @@ public Condition(Lock @lock)
             _lock = @lock;
         }
 
-        public unsafe bool Wait(int millisecondsTimeout, object associatedObjectForMonitorWait)
+        public bool Wait(int millisecondsTimeout, object associatedObjectForMonitorWait)
         {
             ArgumentOutOfRangeException.ThrowIfLessThan(millisecondsTimeout, -1);
 
@@ -128,6 +144,7 @@ public unsafe bool Wait(int millisecondsTimeout, object associatedObjectForMonit
 
             uint recursionCount = _lock.ExitAll();
             bool success = false;
+            bool wasSignaled;
             try
             {
                 success =
@@ -142,14 +159,16 @@ public unsafe bool Wait(int millisecondsTimeout, object associatedObjectForMonit
                 _lock.Reenter(recursionCount);
                 Debug.Assert(_lock.IsHeldByCurrentThread);
 
-                if (!waiter.signalled)
+                // If the waiter is still in the list, it was not signaled.
+                wasSignaled = NotInList(waiter);
+                if (!wasSignaled)
                 {
                     RemoveWaiter(waiter);
                 }
                 else if (!success)
                 {
                     //
-                    // The wait timed out, but we were signalled before we could reacquire the lock.
+                    // The wait timed out, but we were signaled before we could reacquire the lock.
                     // Since WaitOne timed out, it didn't trigger the auto-reset of the AutoResetEvent.
                     // So, we need to manually reset the event.
                     //
@@ -160,19 +179,39 @@ public unsafe bool Wait(int millisecondsTimeout, object associatedObjectForMonit
                 ReleaseWaiterForCurrentThread(waiter);
             }
 
-            return waiter.signalled;
+            return wasSignaled;
         }
 
-        public unsafe void SignalAll()
+        public void SignalAll()
         {
             if (!_lock.IsHeldByCurrentThread)
                 throw new SynchronizationLockException();
 
-            while (_waitersHead != null)
-                SignalOne();
+            Waiter? waiter = _waitersHead;
+            if (waiter is null)
+            {
+                return;
+            }
+
+            // Detach the entire waiter list in one operation, then walk it and signal each waiter.
+            // Per-waiter prev/next must be cleared BEFORE calling ev.Set() so that the woken thread
+            // observes the waiter as not in the list (see NotInList) and the cached Waiter is clean.
+            // Woken threads cannot make progress until the caller releases _lock, so it is safe to
+            // continue walking the detached list after signaling each waiter.
+            _waitersHead = null;
+            _waitersTail = null;
+
+            while (waiter is not null)
+            {
+                Waiter? next = waiter.next;
+                waiter.next = null;
+                waiter.prev = null;
+                waiter.ev.Set();
+                waiter = next;
+            }
         }
 
-        public unsafe void SignalOne()
+        public void SignalOne()
         {
             if (!_lock.IsHeldByCurrentThread)
                 throw new SynchronizationLockException();
@@ -181,7 +220,6 @@ public unsafe void SignalOne()
             if (waiter != null)
             {
                 RemoveWaiter(waiter);
-                waiter.signalled = true;
                 waiter.ev.Set();
             }
         }

src/libraries/System.Private.CoreLib/src/System/Threading/Lock.cs

@@ -52,7 +52,39 @@ public sealed class Lock
         private short _spinCount;
 
         private ushort _waiterStartTimeMs;
-        private AutoResetEvent? _waitEvent;
+
+        private object? _waitEventOrCondition;
+        private AutoResetEvent? WaitEvent
+        {
+            get
+            {
+                object? weoc = _waitEventOrCondition;
+                if (weoc is Condition c)
+                    return c._waitEvent;
+
+                return (AutoResetEvent?)weoc;
+            }
+        }
+
+        internal Condition GetOrCreateCondition()
+        {
+            // The loop terminates as _waitEventOrCondition has limited number of
+            // state transitions with no cycles.
+            while (true)
+            {
+                object? weoc = _waitEventOrCondition;
+                if (weoc is Condition c)
+                    return c;
+
+                Condition newCondition = new Condition(this);
+                newCondition._waitEvent = WaitEvent;
+
+                if (Interlocked.CompareExchange(ref _waitEventOrCondition, newCondition, weoc) == weoc)
+                {
+                    return newCondition;
+                }
+            }
+        }
 
         /// <summary>
         /// Initializes a new instance of the <see cref="Lock"/> class.
@@ -513,7 +545,8 @@ internal int TryEnterSlow(int timeoutMs, int currentThreadId)
                 NativeRuntimeEventSource.Log.IsEnabled(
                     EventLevel.Informational,
                     NativeRuntimeEventSource.Keywords.ContentionKeyword);
-            AutoResetEvent waitEvent = _waitEvent ?? CreateWaitEvent(areContentionEventsEnabled);
+
+            AutoResetEvent waitEvent = WaitEvent ?? CreateWaitEvent(areContentionEventsEnabled);
             if (State.TryLockBeforeWait(this))
             {
                 // Lock was acquired and a waiter was not registered
@@ -652,8 +685,13 @@ private bool ShouldStopPreemptingWaiters
         private AutoResetEvent CreateWaitEvent(bool areContentionEventsEnabled)
         {
             var newWaitEvent = new AutoResetEvent(false);
-            AutoResetEvent? waitEventBeforeUpdate = Interlocked.CompareExchange(ref _waitEvent, newWaitEvent, null);
-            if (waitEventBeforeUpdate == null)
+            object? weocBeforeUpdate = Interlocked.CompareExchange(ref _waitEventOrCondition, newWaitEvent, null);
+            if (weocBeforeUpdate is Condition c)
+            {
+                weocBeforeUpdate = Interlocked.CompareExchange(ref c._waitEvent, newWaitEvent, null);
+            }
+
+            if (weocBeforeUpdate == null)
             {
                 // Also check NativeRuntimeEventSource.Log.IsEnabled() to enable trimming
                 if (areContentionEventsEnabled && NativeRuntimeEventSource.Log.IsEnabled())
@@ -665,7 +703,7 @@ private AutoResetEvent CreateWaitEvent(bool areContentionEventsEnabled)
             }
 
             newWaitEvent.Dispose();
-            return waitEventBeforeUpdate;
+            return (AutoResetEvent)weocBeforeUpdate;
         }
 
         [MethodImpl(MethodImplOptions.NoInlining)]
@@ -674,8 +712,8 @@ private void SignalWaiterIfNecessary(State state)
             if (State.TrySetIsWaiterSignaledToWake(this, state))
             {
                 // Signal a waiter to wake
-                Debug.Assert(_waitEvent != null);
-                bool signaled = _waitEvent.Set();
+                Debug.Assert(WaitEvent != null);
+                bool signaled = WaitEvent.Set();
                 Debug.Assert(signaled);
             }
         }
@@ -695,14 +733,14 @@ public bool IsHeldByCurrentThread
         }
 
         internal static long ContentionCount => s_contentionCount;
-        internal void Dispose() => _waitEvent?.Dispose();
+        internal void Dispose() => WaitEvent?.Dispose();
 
         internal nint LockIdForEvents
         {
             get
             {
-                Debug.Assert(_waitEvent != null);
-                return _waitEvent.SafeWaitHandle.DangerousGetHandle();
+                Debug.Assert(WaitEvent != null);
+                return WaitEvent.SafeWaitHandle.DangerousGetHandle();
             }
         }
 
@@ -927,6 +965,28 @@ private static short DetermineMinSpinCountForAdaptiveSpin()
             return (short)-adaptiveSpinPeriod;
         }
 
+        internal bool Wait(int millisecondsTimeout)
+        {
+#pragma warning disable CS9216 // A value of type 'System.Threading.Lock' converted to a different type will use likely unintended monitor-based locking in 'lock' statement.
+            return Wait(millisecondsTimeout, this);
+#pragma warning restore CS9216 // A value of type 'System.Threading.Lock' converted to a different type will use likely unintended monitor-based locking in 'lock' statement.
+        }
+
+        internal bool Wait(int millisecondsTimeout, object associatedObject)
+        {
+            return GetOrCreateCondition().Wait(millisecondsTimeout, associatedObject);
+        }
+
+        internal void Pulse()
+        {
+            GetOrCreateCondition().SignalOne();
+        }
+
+        internal void PulseAll()
+        {
+            GetOrCreateCondition().SignalAll();
+        }
+
         private struct State : IEquatable<State>
         {
             // Layout constants for Lock._state