fix(threading): a waiting writer blocks new readers (#1957, SR-AUD-204) — closes #1957

Rule-14 sweep, final member of #1957. The read-admission predicate was
!writerActive_ alone -- it asked whether a writer HELD the lock, never
whether one was WAITING for it -- so a steady arrival of readers starved a
blocked writer indefinitely.

.NET's own comment is the derivation: WaitOnEvent sets WAITING_WRITERS and
WAITING_UPGRADER as soon as the first writer waits, under "Setting these bits
will prevent new readers from getting in" (ReaderWriterLockSlim.cs:1005-1010).
Both bits sit above MAX_READER, so its single test `_owners < MAX_READER`
refuses a reader whenever a writer holds OR awaits the lock. This port keeps
named fields, so the bit becomes a counter and the predicate gains one term.

Both kinds of writer count, as both .NET bits do -- an upgrade-to-write
blocks readers too, which is the easy half to miss and is pinned separately.
A timed-out writer stops blocking, via an RAII guard mirroring .NET's
finally, because that failure mode would be permanent rather than transient.
Writer preference cannot deadlock a recursive reader: every thread already
holding a read, write or upgrade lock returns before the predicate.

This is a fairness change -- a reader-heavy workload that never blocked can
now block. That is the point, and it is .NET's documented behaviour.

sizeof(ReaderWriterLockSlim) is 120 before and after.

Two mutations, both caught. Every probing reader runs on its own thread, and
that is not incidental: NoRecursion makes a second read acquisition on the
holding thread throw, and an earlier draft did exactly that and took the
executable down with "terminate called without an active exception".

ThreadSanitizer, with two limits stated rather than implied: the full target
CANNOT build under TSan, because Thread::MemoryBarrier() is
std::atomic_thread_fence, which gcc rejects under -Werror=tsan -- a
pre-existing incompatibility reached from an unrelated file. So the evidence
is a focused probe, clean over 1,020,489 read acquisitions; the probe was
shown able to report (a deliberate unsynchronised counter yields 2 race
warnings); and the probe does NOT reproduce the starvation, reporting
writes=6089 even pre-repair, because its readers do not overlap continuously
-- the deterministic gtest does that instead.

#1957 is now CLOSED: SR-AUD-202 landed as #2341, and SR-AUD-201, SR-AUD-210
and SR-AUD-204 all landed today. Its section-9 approval question is answered
by SA-3, granted after the design record was written.

Downstream measured: 0 sites in cna, 0 in mobile-eggbert.

Gate: 17,467 run, 17,467 passed, 0 failed, 0 skipped across 38 executables
(+5 on 17,462; SharpRuntimeTests_Threading 486 -> 491; no other executable
moved). Module graph unchanged at 41/93.

Author: robertvokac

CLAUDE.md

@@ -0,0 +1,138 @@
+<!-- SPDX-License-Identifier: MIT -->
+<!-- Copyright (c) Robert Vokac and contributors -->
+
+# Migration — a waiting writer now blocks new readers (ticket #1957, SR-AUD-204)
+
+*2026-08-19.* `System::Threading::ReaderWriterLockSlim` gives writers precedence: once a writer is
+waiting, a **new** reader waits behind it instead of walking straight in.
+
+**This is a fairness change. A reader-heavy workload that never blocked can now block.** That is
+the point — it is what stops the writer starving — and it is .NET's documented behaviour. Read §3
+before upgrading.
+
+Landed under **SA-5**, with SA-3's layout condition discharged as **layout-neutral**:
+`sizeof(ReaderWriterLockSlim)` is **120 before and after**.
+
+---
+
+## 1. What was wrong
+
+The read-admission predicate was `!writerActive_` — it asked only whether a writer *held* the
+lock, never whether one was *waiting for* it. So:
+
+1. reader A holds the lock;
+2. writer W blocks in `TryEnterWriteLock`, waiting for `readers_ == 0`;
+3. reader B arrives — and was admitted immediately;
+4. reader C arrives — admitted;
+5. …and `readers_` never reaches zero, so **W waits for ever**.
+
+Nothing bounded that. A steady arrival of readers starved the writer indefinitely.
+
+## 2. What .NET does
+
+.NET keeps the same signal, packed into its single `_owners` word:
+
+```csharp
+private const uint WRITER_HELD     = 0x80000000;
+private const uint WAITING_WRITERS = 0x40000000;
+private const uint WAITING_UPGRADER= 0x20000000;
+private const uint MAX_READER      = 0x10000000 - 2;
+...
+// Setting these bits will prevent new readers from getting in.
+if (_numWriteWaiters == 1)        SetWritersWaiting();      // ReaderWriterLockSlim.cs:1005-1010
+if (_numWriteUpgradeWaiters == 1) SetUpgraderWaiting();
+```
+
+Both waiting bits sit **above** `MAX_READER`, so .NET's single admission test
+`if (_owners < MAX_READER)` refuses a new reader whenever a writer holds the lock **or** is
+waiting for it. One comparison, two conditions.
+
+This port keeps its state in named fields rather than one packed word, so the bit becomes a
+counter, `waitingWriters_`, and the predicate gains one term.
+
+**Both kinds of writer count**, exactly as both .NET bits do: a plain writer *and* an
+upgrade-to-write. Counting only plain writers would leave the upgrade path starvable, which is
+the easy half to miss — a test pins it.
+
+**A writer that times out stops blocking readers.** .NET clears its bit in `WaitOnEvent`'s
+`finally` (`ReaderWriterLockSlim.cs:1039-1042`); here an RAII guard decrements on every exit —
+acquired, timed out, or thrown — and wakes the readers it was holding back. A guard that
+decremented only on success would wedge every future reader, and that failure mode is
+**permanent**, not transient.
+
+## 3. What changes for a caller
+
+| Situation | Was | Is |
+|---|---|---|
+| no writer waiting | reader enters | reader enters — **unchanged** |
+| a writer is waiting | reader enters, writer starves | reader **waits** |
+| an upgrader is waiting for the write lock | reader enters | reader **waits** |
+| the waiting writer times out | — | readers flow again immediately |
+| a thread re-entering a lock it already holds | unaffected | unaffected (§4) |
+
+`TryEnterReadLock(timeout)` can now return `false` where it used to return `true`. If you were
+relying on readers always winning, you were relying on the starvation this removes.
+
+## 4. Writer preference cannot deadlock a recursive reader
+
+Every path that returns *before* the predicate is untouched:
+
+* a thread that already holds a **read** lock returns early (or throws, under `NoRecursion`);
+* a thread holding the **write** or **upgrade** lock has implied read access and returns early.
+
+So only a genuinely **new** reader can be delayed — which is precisely .NET's contract.
+
+## 5. Evidence
+
+Two mutations, **both caught**:
+
+| Mutation | Caught by |
+|---|---|
+| M1 — the read predicate ignores waiting writers | `Fix1957_ANewReaderWaitsBehindABlockedWriter`, `Fix1957_AnUpgraderWaitingForWriteAlsoBlocksNewReaders` |
+| M2 — the guard decrements only on success | `Fix1957_ANewReaderWaitsBehindABlockedWriter`, `Fix1957_ATimedOutWriterStopsBlockingReaders` |
+
+**Every probing reader runs on its own thread**, and that is not incidental: the default
+`LockRecursionPolicy` is `NoRecursion`, so a second read acquisition on the thread that already
+holds one throws `LockRecursionException` rather than testing admission. An earlier draft did
+exactly that and took the executable down with `terminate called without an active exception`,
+because the escaping exception left two `std::thread`s joinable.
+
+Gate: **17,467 run, 17,467 passed, 0 failed, 0 skipped** across 38 executables — `+5` on 17,462,
+exactly the five new cases (`SharpRuntimeTests_Threading` 486 → 491). No other executable moved.
+Module graph unchanged at 41/93.
+
+## 6. ThreadSanitizer
+
+`docs/ThreadingNamespaceReviewPlan.md` §12 requires TSan for this member specifically. Three
+things are worth stating precisely, because two of them are limitations.
+
+**The full test target cannot be built under TSan, and that is pre-existing.**
+`Thread::MemoryBarrier()` is `std::atomic_thread_fence`, which gcc rejects outright:
+*"'atomic_thread_fence' is not supported with '-fsanitize=thread' [-Werror=tsan]"*. It is reached
+from an unrelated test file and has nothing to do with this change —
+`ReaderWriterLockSlim.hpp` does not include `Thread.hpp` at all. This is the same incompatibility
+#2298 recorded.
+
+**So the evidence is a focused probe** (`build-probe/1957_probe4_rwls_tsan.cpp`): four readers,
+two writers and an upgrader hammering the repaired type for three seconds. Result:
+
+```
+reads=1020489 refusedReads=0 writes=6747 shared=6747     TSan: clean, no reports
+```
+
+**And the probe was shown capable of reporting**, per the plan's §19.4 rule that a silent
+sanitizer is evidence about the probe until proven otherwise: the same probe with one deliberate
+unsynchronised counter added produces **2** `WARNING: ThreadSanitizer: data race` reports. The
+clean result above is therefore meaningful.
+
+**What the probe does NOT show, stated rather than implied**: it does not reproduce the
+starvation. Run against the *pre-repair* predicate it still reports `writes=6089` — its writers
+use a timed `TryEnterWriteLock` and its readers sleep, so `readers_` reaches zero often enough
+for writers to get in anyway. Reproducing starvation needs readers that overlap *continuously*,
+which is what the deterministic unit test does by holding one reader open across the writer's
+arrival. The probe's job here is the race question; the gtest's is the fairness question.
+
+## 7. Downstream, measured
+
+`ReaderWriterLockSlim` appears in **zero** places in `cna` and **zero** in `mobile-eggbert`.
+Neither repository was modified.

docs/Migration-ReaderWriterWriterPreference.md

@@ -89,6 +89,23 @@ namespace System::Threading {
         mutable std::mutex stateMtx_;
         mutable std::condition_variable cv_;
         intcs readers_ = 0;
+        // Ticket #1957 / SR-AUD-204, cause T-E/2. Without this the read-admission predicate was
+        // `!writerActive_` alone, so a steady stream of new readers could enter past a writer
+        // already blocked in TryEnterWriteLock and starve it INDEFINITELY.
+        //
+        // .NET keeps the same signal, packed into its single `_owners` word: WaitOnEvent sets
+        // WAITING_WRITERS (and WAITING_UPGRADER) as soon as the first writer begins waiting,
+        // under the comment "Setting these bits will prevent new readers from getting in"
+        // (ReaderWriterLockSlim.cs:1005-1010). Both bits sit above MAX_READER, so .NET's single
+        // admission test `_owners < MAX_READER` refuses a new reader whenever a writer holds the
+        // lock OR is waiting for it. This counter is that bit, spelled separately because this
+        // port keeps its state in named fields rather than one packed word.
+        //
+        // BOTH kinds of writer count, exactly as both .NET bits do: a plain writer and an
+        // upgrade-to-write. .NET clears each in WaitOnEvent's `finally`
+        // (ReaderWriterLockSlim.cs:1039-1042), so a writer that TIMES OUT stops blocking readers;
+        // the RAII guard in TryEnterWriteLock does the same here.
+        intcs waitingWriters_ = 0;
         bool writerActive_ = false;
         bool upgradeableActive_ = false;
         // Ticket #1955 / cause T-A of docs/ThreadingNamespaceReviewPlan.md. This was an
@@ -215,7 +232,13 @@ namespace System::Threading {
             }
 
             std::unique_lock<std::mutex> lk(stateMtx_);
-            if (!waitFor(lk, millisecondsTimeout, [&] { return !writerActive_; })) return false;
+            // #1957/SR-AUD-204: `waitingWriters_ == 0` is the new term. A thread that already
+            // holds the read, write or upgrade lock never reaches here -- every one of those
+            // cases returned above -- so writer preference can only delay a genuinely NEW
+            // reader, which is precisely .NET's contract and cannot deadlock a recursive one.
+            if (!waitFor(lk, millisecondsTimeout,
+                         [&] { return !writerActive_ && waitingWriters_ == 0; }))
+                return false;
             ++readers_;
             counts.reader = 1;
             counts.readerCountsTowardGlobal = true;
@@ -271,6 +294,24 @@ namespace System::Threading {
                     "lock. If an upgrade is necessary, use an upgrade lock in place of the read lock.");
 
             std::unique_lock<std::mutex> lk(stateMtx_);
+
+            // Announce the wait BEFORE waiting, so readers arriving from now on are refused --
+            // .NET sets its bit at the same point, before the wait rather than after it. The
+            // guard decrements on every exit (acquired, timed out, or thrown) and wakes the
+            // readers it was holding back, which is what .NET's `finally` does via
+            // ClearWritersWaiting + ExitAndWakeUpAppropriateReadWaiters.
+            //
+            // Declared after `lk` so it is destroyed BEFORE the lock is released: the decrement
+            // and the notify both happen under the mutex.
+            ++waitingWriters_;
+            struct WaitingWriterGuard {
+                intcs&                   count;
+                std::condition_variable& cv;
+                ~WaitingWriterGuard() {
+                    if (--count == 0) cv.notify_all();
+                }
+            } waitingWriterGuard{waitingWriters_, cv_};
+
             bool acquired = upgradingToWrite
                 ? waitFor(lk, millisecondsTimeout, [&] { return readers_ == 0; })
                 : waitFor(lk, millisecondsTimeout, [&] { return !writerActive_ && readers_ == 0 && !upgradeableActive_; });

modules/threading/include/System/Threading/ReaderWriterLockSlim.hpp

@@ -993,3 +993,146 @@ TEST(BarrierPostPhaseReadabilityTests, Fix1957_TheOtherMembersStillRefuseReentra
     EXPECT_EQ(phase.load(), 0);
     EXPECT_EQ(participants.load(), 1);
 }
+
+// =============================================================================================
+// Ticket #1957 / SR-AUD-204 — a waiting writer blocks subsequent readers.
+//
+// The read-admission predicate was `!writerActive_` alone, with no waiting-writer term, so a
+// steady stream of new readers entered past a writer already blocked in TryEnterWriteLock and
+// could starve it INDEFINITELY.
+//
+// .NET keeps the same signal packed into its single `_owners` word: WaitOnEvent sets
+// WAITING_WRITERS as soon as the first writer begins waiting, under the comment "Setting these
+// bits will prevent new readers from getting in" (ReaderWriterLockSlim.cs:1005-1010). Both that
+// bit and WAITING_UPGRADER sit above MAX_READER, so .NET's single admission test
+// `_owners < MAX_READER` refuses a new reader whenever a writer holds the lock OR is waiting.
+//
+// Landed under SA-5, with SA-3's layout condition discharged as layout-neutral:
+// sizeof(ReaderWriterLockSlim) is 120 before and after (build-probe/1957_probe3_rwls.cpp).
+//
+// THIS IS A FAIRNESS CHANGE: a reader-heavy workload that never blocked can now block. That is
+// the point -- it is what stops the writer starving -- and it is .NET's documented behaviour.
+//
+// EVERY probing reader below runs on ITS OWN THREAD. The default LockRecursionPolicy is
+// NoRecursion, so a second read acquisition on the thread that already holds one throws
+// LockRecursionException rather than testing admission -- an earlier draft did exactly that and
+// took the executable down with `terminate called without an active exception`, because the
+// escaping exception left two std::threads joinable.
+// =============================================================================================
+
+namespace {
+    /// Tries to take a read lock on a fresh thread and reports whether it got in.
+    bool ReaderOnItsOwnThreadCanEnter(System::Threading::ReaderWriterLockSlim& lock,
+                                      SharpRuntime::intcs timeoutMs) {
+        std::atomic<bool> entered{false};
+        std::thread probe([&] {
+            if (lock.TryEnterReadLock(timeoutMs)) {
+                entered.store(true);
+                lock.ExitReadLock();
+            }
+        });
+        probe.join();
+        return entered.load();
+    }
+}
+
+TEST(ReaderWriterWriterPreferenceTests, Decl1957_TheWaitingWriterCountIsLayoutNeutral) {
+    static_assert(sizeof(System::Threading::ReaderWriterLockSlim) == 120,
+                  "#1957/SR-AUD-204 must not change ReaderWriterLockSlim's layout");
+    static_assert(alignof(System::Threading::ReaderWriterLockSlim) == 8);
+    EXPECT_EQ(sizeof(System::Threading::ReaderWriterLockSlim), 120u);
+}
+
+TEST(ReaderWriterWriterPreferenceTests, Fix1957_ANewReaderWaitsBehindABlockedWriter) {
+    // THE DEFECT. A reader holds the lock; a writer blocks behind it; a NEW reader arrives on
+    // another thread. Before the repair that reader walked straight in, so an endless supply of
+    // readers starved the writer.
+    System::Threading::ReaderWriterLockSlim lock;
+    std::atomic<bool> writerWaiting{false};
+    std::atomic<bool> writerAcquired{false};
+
+    lock.EnterReadLock();   // first reader, held by this thread
+
+    std::thread writer([&] {
+        writerWaiting.store(true);
+        lock.EnterWriteLock();
+        writerAcquired.store(true);
+        lock.ExitWriteLock();
+    });
+
+    while (!writerWaiting.load()) std::this_thread::yield();
+    std::this_thread::sleep_for(std::chrono::milliseconds(150));
+    ASSERT_FALSE(writerAcquired.load()) << "the writer must still be blocked behind the reader";
+
+    EXPECT_FALSE(ReaderOnItsOwnThreadCanEnter(lock, 200))
+        << "a new reader entered past a waiting writer -- the starvation this ticket removes";
+
+    lock.ExitReadLock();    // release the original reader; the writer now gets in
+    writer.join();
+    EXPECT_TRUE(writerAcquired.load());
+
+    // ...and once the writer is done, readers flow again.
+    EXPECT_TRUE(ReaderOnItsOwnThreadCanEnter(lock, 1000));
+}
+
+TEST(ReaderWriterWriterPreferenceTests, Fix1957_ATimedOutWriterStopsBlockingReaders) {
+    // .NET clears its bit in WaitOnEvent's `finally` (ReaderWriterLockSlim.cs:1039-1042), so a
+    // writer that gives up must stop holding readers back. A guard that decremented only on
+    // success would wedge every future reader -- the failure mode is permanent, not transient.
+    System::Threading::ReaderWriterLockSlim lock;
+    lock.EnterReadLock();
+
+    std::thread writer([&] {
+        EXPECT_FALSE(lock.TryEnterWriteLock(100)) << "the writer cannot get in past the reader";
+    });
+    writer.join();
+
+    EXPECT_TRUE(ReaderOnItsOwnThreadCanEnter(lock, 1000))
+        << "a timed-out writer must stop blocking readers";
+    lock.ExitReadLock();
+}
+
+TEST(ReaderWriterWriterPreferenceTests, Fix1957_TheUncontendedPathsAreUnchanged) {
+    // Writer preference must cost nothing when no writer is waiting.
+    System::Threading::ReaderWriterLockSlim lock;
+    EXPECT_TRUE(lock.TryEnterReadLock(0));
+    lock.ExitReadLock();
+    EXPECT_TRUE(lock.TryEnterWriteLock(0));
+    lock.ExitWriteLock();
+    lock.EnterReadLock();
+    lock.ExitReadLock();
+    lock.EnterWriteLock();
+    lock.ExitWriteLock();
+    EXPECT_TRUE(ReaderOnItsOwnThreadCanEnter(lock, 0));
+}
+
+TEST(ReaderWriterWriterPreferenceTests, Fix1957_AnUpgraderWaitingForWriteAlsoBlocksNewReaders) {
+    // .NET sets WAITING_UPGRADER for the upgrade-to-write case as well, and it too sits above
+    // MAX_READER -- so BOTH kinds of writer block new readers. Counting only plain writers would
+    // leave the upgrade path starvable, which is the easy half to miss.
+    System::Threading::ReaderWriterLockSlim lock;
+    std::atomic<bool> upgraderWaiting{false};
+    std::atomic<bool> upgraded{false};
+
+    lock.EnterReadLock();   // a reader the upgrader must wait to drain
+
+    std::thread upgrader([&] {
+        lock.EnterUpgradeableReadLock();
+        upgraderWaiting.store(true);
+        lock.EnterWriteLock();      // waits for readers_ == 0
+        upgraded.store(true);
+        lock.ExitWriteLock();
+        lock.ExitUpgradeableReadLock();
+    });
+
+    while (!upgraderWaiting.load()) std::this_thread::yield();
+    std::this_thread::sleep_for(std::chrono::milliseconds(150));
+    ASSERT_FALSE(upgraded.load()) << "the upgrader must still be waiting for the reader";
+
+    EXPECT_FALSE(ReaderOnItsOwnThreadCanEnter(lock, 200))
+        << "a new reader entered past an upgrader waiting for the write lock";
+
+    lock.ExitReadLock();
+    upgrader.join();
+    EXPECT_TRUE(upgraded.load());
+}