Skip to content

Commit 9f3114c

Browse files
robertvokacrobertvokac
committed
fix(threading): disposal is a real state across System::Threading (#1956)
Rule-14 sweep. The recorded gate was "approval question 1" -- may the repair turn previously-succeeding public calls into throws? SA-5 grants exactly that, verbatim ("including where a call that succeeds today starts throwing"), granted two weeks after the design record was written. Four findings, all verified against the reference first: * Mutex/AutoResetEvent/ManualResetEvent::Close() were EMPTY BODIES, so Close() then WaitOne(0) returned success -- while the headers already claimed Close "closes the handle", so documentation and behaviour disagreed. They gain an atomic flag and every operation now throws ObjectDisposedException, as .NET does (WaitHandle.cs:87-98, 118). * ThreadLocal::IsValueCreated answered `false` when disposed -- indistinguishable from "alive, no value yet". .NET throws (ThreadLocal.cs:478-488). * ReaderWriterLockSlim::Dispose() succeeded with a lock held; .NET throws SynchronizationLockException (ReaderWriterLockSlim.cs:1250-1258). * ITimer::Change returned true unconditionally after disposal. ITimer::Change is DELIBERATELY EXCLUDED from the throwing group and now returns false: .NET's Timer.Change opens `if (_canceled) { return false; }` (Timer.cs:539-542), so making it throw for symmetry would contradict the interface. Mutation M6 is the one that pin exists to stop. Mutex overrides Dispose() rather than shadowing Close(), which is .NET's own arrangement, so m.Close(), m.Dispose() and a call through a WaitHandle& all reach the guard. No source break. Every affected sizeof is unchanged -- the flags land in existing padding. Six mutations, all caught. M6 was invalid as first written (a missing include) and was reformulated rather than counted. One deliberate narrowing, and it is a gap in the design record rather than an implementation shortcut: .NET's Dispose performs TWO checks and section 20.1 item 4 named only the second. The waiters half needs per-mode waiter counts this port lacks, so the behaviour is a strict subset of .NET's -- it never refuses a disposal .NET would accept. Filed as #2389. Downstream measured, all six types separately: 0 sites in cna, 0 in mobile-eggbert. Gate: 17,476 run, 17,476 passed, 0 failed, 0 skipped across 38 executables (+9 on 17,467; SharpRuntimeTests_Threading 491 -> 500; no other executable moved). Module graph unchanged at 41/93.
1 parent dc2ef76 commit 9f3114c

10 files changed

Lines changed: 423 additions & 11 deletions

File tree

‎CLAUDE.md‎

Lines changed: 1 addition & 1 deletion
Large diffs are not rendered by default.

‎docs/Migration-ThreadingDisposalIsARealState.md‎

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,119 @@
1+
<!-- SPDX-License-Identifier: MIT -->
2+
<!-- Copyright (c) Robert Vokac and contributors -->
3+
4+
# Migration — disposal is a real state across `System::Threading` (ticket #1956)
5+
6+
*2026-08-19.* Four types stop treating disposal as decoration. `Mutex`, `AutoResetEvent` and
7+
`ManualResetEvent` refuse every operation after `Close()`; `ThreadLocal<T>::IsValueCreated`
8+
refuses after `Dispose()`; `ReaderWriterLockSlim::Dispose()` refuses while a mode is held. One
9+
member is **deliberately excluded** and keeps returning `false`.
10+
11+
**Calls that succeed today start throwing.** Read §2 before upgrading.
12+
13+
Landed under **SA-5**, which grants this ticket's approval question verbatim — *"including where
14+
a call that succeeds today starts throwing"* — with SA-3's layout condition discharged: **every
15+
affected `sizeof` is unchanged**, the flags landing in padding the types already had.
16+
17+
---
18+
19+
## 1. What was wrong, per finding
20+
21+
| Finding | Was | Measured symptom |
22+
|---|---|---|
23+
| **SR-AUD-208** | `Mutex/AutoResetEvent/ManualResetEvent::Close()` were **empty bodies** | `Close()` then `WaitOne(0)` returned **success** |
24+
| **SR-AUD-219** | `ThreadLocal<T>::IsValueCreated` never checked disposal | a disposed instance answered **`false`** — indistinguishable from "alive, no value yet" |
25+
| **SR-AUD-203** | `ReaderWriterLockSlim::Dispose()` set the flag unconditionally | disposing **with a lock held** succeeded, leaving the holder owning a mode on a disposed object |
26+
| *(T-G/timer)* | `ITimer::Change` returned `true` unconditionally | a **disposed** timer reported it had been rescheduled |
27+
28+
The three `Close()` bodies are the worst of these, because the headers **already claimed** that
29+
`Close` "closes the handle". The documentation and the behaviour disagreed, so either the comment
30+
was false or the API was decorative.
31+
32+
## 2. What changes
33+
34+
| Call | Was | Is |
35+
|---|---|---|
36+
| `Mutex::WaitOne()` / `WaitOne(ms)` / `ReleaseMutex()` after `Close()` | succeeded | `ObjectDisposedException` |
37+
| `AutoResetEvent`/`ManualResetEvent` `Set`/`Reset`/`WaitOne` after `Close()` | succeeded | `ObjectDisposedException` |
38+
| `Close()` twice | no-op | no-op — **idempotent, unchanged** |
39+
| `ThreadLocal<T>::getIsValueCreatedProperty()` after `Dispose()` | `false` | `ObjectDisposedException` |
40+
| `ReaderWriterLockSlim::Dispose()` with a mode held | succeeded | `SynchronizationLockException` |
41+
| `ReaderWriterLockSlim::Dispose()` with nothing held | succeeded | succeeded — unchanged, and still idempotent |
42+
| **`ITimer::Change` after `Dispose()`** | `true` | **`false` — and still does not throw** |
43+
44+
## 3. The one member excluded, and why that is not an inconsistency
45+
46+
`ITimer::Change` must **not** throw. .NET's `Timer.Change` opens
47+
48+
```csharp
49+
if (_canceled)
50+
{
51+
return false; // Timer.cs:539-542
52+
}
53+
```
54+
55+
A `false` **return** is the documented `ITimer` contract. Making it throw for symmetry with the
56+
wait handles would contradict the interface this type implements — so the design record excluded
57+
it, and the reference confirms the exclusion. The asymmetry is **pinned by a test**
58+
(`Decl1956_ITimerChangeReturnsFalseAndDoesNotThrow`) rather than left to look like an oversight,
59+
so a later "consistency" pass cannot quietly make it throw.
60+
61+
The flag lives in the `.cpp`-local `SystemTimeProviderTimer`, so no public type gained a member
62+
for it. `Dispose()` became idempotent there too.
63+
64+
## 4. `Mutex` overrides `Dispose()` rather than shadowing `Close()`
65+
66+
.NET's arrangement is `public virtual void Close() => Dispose();` (`WaitHandle.cs:87`). This port
67+
already had `WaitHandle::Close() { Dispose(); }`, but `Mutex` **shadowed** it with an empty
68+
`Close()`. The repair removes the shadow and overrides `Dispose()` instead, so both spellings —
69+
and a call through a `WaitHandle&` — reach the same guard. `m.Close()` is unaffected as a
70+
spelling; a test covers all three routes.
71+
72+
## 5. One deliberate narrowing, stated rather than glossed
73+
74+
.NET's `ReaderWriterLockSlim.Dispose` performs **two** checks, in this order
75+
(`ReaderWriterLockSlim.cs:1250-1258`):
76+
77+
```csharp
78+
if (WaitingReadCount > 0 || WaitingUpgradeCount > 0 || WaitingWriteCount > 0) throw ...;
79+
if (IsReadLockHeld || IsUpgradeableReadLockHeld || IsWriteLockHeld) throw ...;
80+
```
81+
82+
**The design record named only the second, and this port implements only the second.** The first
83+
needs per-mode *waiter* counts, of which this port has only `waitingWriters_` (added the same day
84+
by SR-AUD-204); counting waiting readers and upgraders is additional state on three more paths.
85+
That is filed as **#2389**.
86+
87+
The narrowing is a strict **subset** of .NET's: this port never refuses a disposal .NET would
88+
accept. That direction matters — the opposite would break callers .NET supports.
89+
90+
## 6. Evidence
91+
92+
Six mutations, **all caught**:
93+
94+
| Mutation | Caught by |
95+
|---|---|
96+
| M1 — `Mutex::Dispose` does not set the flag | `Fix1956_AClosedMutexRefusesEveryOperation`, `Fix1956_MutexCloseReachesTheOverriddenDispose` |
97+
| M2 — `AutoResetEvent::Set` loses its guard | `Fix1956_AClosedAutoResetEventRefusesEveryOperation` |
98+
| M3 — `ThreadLocal::IsValueCreated` loses its guard | `Fix1956_ADisposedThreadLocalRefusesIsValueCreated` |
99+
| M4 — `ReaderWriterLockSlim::Dispose` ignores a held mode | `Fix1956_DisposingAHeldReaderWriterLockThrows` |
100+
| M5 — `ITimer::Change` claims success after disposal | `Decl1956_ITimerChangeReturnsFalseAndDoesNotThrow` |
101+
| M6 — `ITimer::Change` throws instead of returning `false` | the same test — **this is the mutation the exclusion exists to stop** |
102+
103+
M6 is the reason that pin is worth having: without it, "make disposal consistent" would look like
104+
an improvement and would break the `ITimer` contract.
105+
106+
Gate: **17,476 run, 17,476 passed, 0 failed, 0 skipped** across 38 executables — `+9` on 17,467,
107+
exactly the nine new cases (`SharpRuntimeTests_Threading` 491 → 500). No other executable moved.
108+
All 491 pre-existing cases passed unchanged before the new ones were added. Module graph
109+
unchanged at 41/93.
110+
111+
M6 was invalid as first written (a missing include, not a behaviour) and was reformulated rather
112+
than counted.
113+
114+
## 7. Downstream, measured
115+
116+
All six affected types measured separately rather than assumed to match: `Mutex`,
117+
`AutoResetEvent`, `ManualResetEvent`, `ThreadLocal`, `ReaderWriterLockSlim` and `ITimer` each
118+
appear in **zero** places in `cna` and **zero** in `mobile-eggbert`. Neither repository was
119+
modified.

‎modules/threading/include/System/Threading/AutoResetEvent.hpp‎

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,11 @@
33
// Portions based on .NET runtime API (MIT License, Copyright .NET Foundation and Contributors)
44
#pragma once
55
#include <mutex>
6+
#include <atomic>
67
#include <condition_variable>
78

89
#include "SharpRuntime/SharpRuntimeHelper.hpp"
10+
#include "System/ObjectDisposedException.hpp"
911
#include "System/Threading/WaitHandle.hpp"
1012

1113
namespace System::Threading {
@@ -23,24 +25,43 @@ namespace System::Threading {
2325
std::mutex mutex_;
2426
std::condition_variable cv_;
2527
bool signaled_;
28+
// Ticket #1956 / cause T-G (SR-AUD-208). Close() was an EMPTY BODY, so a closed handle
29+
// stayed fully usable: measured, Close() followed by WaitOne(0) returned success. .NET's
30+
// WaitHandle.Close() is `=> Dispose()`, Dispose(bool) is `_waitHandle?.Close()`, and every
31+
// wait path then opens with `ObjectDisposedException.ThrowIf(waitHandle is null, this)`
32+
// (WaitHandle.cs:87-98, 118). The header here already CLAIMED Close "closes the handle",
33+
// so the documentation and the behaviour disagreed.
34+
//
35+
// std::atomic<bool> is 1 byte and 1-byte aligned, and it lands in padding these types
36+
// already had -- the sizes are unchanged, pinned by a layout test. Landed under SA-5 (a
37+
// call that succeeds today starts throwing) with SA-3's layout condition discharged.
38+
std::atomic<bool> closed_{false};
39+
40+
void ThrowIfClosed() const {
41+
if (closed_.load(std::memory_order_acquire))
42+
throw System::ObjectDisposedException("The handle has been closed.");
43+
}
2644
public:
2745
/** @param initialState If true, the event starts in the signaled state. */
2846
explicit AutoResetEvent(bool initialState = false) : signaled_(initialState) {}
2947

3048
/** Sets the event to signaled, releasing one waiting thread; the event then resets automatically. */
3149
void Set() {
50+
ThrowIfClosed();
3251
{ std::lock_guard<std::mutex> lk(mutex_); signaled_ = true; }
3352
cv_.notify_one();
3453
}
3554

3655
/** Resets the event to non-signaled. */
3756
void Reset() {
57+
ThrowIfClosed();
3858
std::lock_guard<std::mutex> lk(mutex_);
3959
signaled_ = false;
4060
}
4161

4262
/** Blocks until the event is signaled (then auto-resets). */
4363
void WaitOne() {
64+
ThrowIfClosed();
4465
std::unique_lock<std::mutex> lk(mutex_);
4566
cv_.wait(lk, [this]{ return signaled_; });
4667
signaled_ = false;
@@ -59,6 +80,7 @@ namespace System::Threading {
5980
* semantics returns almost immediately instead of throwing.
6081
*/
6182
bool WaitOne(intcs milliseconds) {
83+
ThrowIfClosed();
6284
WaitHandle::ValidateTimeout(milliseconds);
6385
std::unique_lock<std::mutex> lk(mutex_);
6486
// -1 (Timeout.Infinite) waits indefinitely; std::chrono's wait_for treats a
@@ -74,8 +96,13 @@ namespace System::Threading {
7496
return ok;
7597
}
7698

77-
/** Releases resources (no-op; provided for .NET API compatibility). */
78-
void Close() {}
99+
/**
100+
* @brief Closes the handle. Every later Set, Reset or WaitOne throws
101+
* System::ObjectDisposedException.
102+
*
103+
* Idempotent, as .NET's is: Close() is `=> Dispose()` and disposing twice is defined.
104+
*/
105+
void Close() { closed_.store(true, std::memory_order_release); }
79106
};
80107

81108
} // namespace System::Threading

‎modules/threading/include/System/Threading/ManualResetEvent.hpp‎

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,11 @@
33
// Portions based on .NET runtime API (MIT License, Copyright .NET Foundation and Contributors)
44
#pragma once
55
#include <mutex>
6+
#include <atomic>
67
#include <condition_variable>
78

89
#include "SharpRuntime/SharpRuntimeHelper.hpp"
10+
#include "System/ObjectDisposedException.hpp"
911
#include "System/Threading/WaitHandle.hpp"
1012

1113
namespace System::Threading {
@@ -23,24 +25,43 @@ namespace System::Threading {
2325
std::mutex mutex_;
2426
std::condition_variable cv_;
2527
bool signaled_;
28+
// Ticket #1956 / cause T-G (SR-AUD-208). Close() was an EMPTY BODY, so a closed handle
29+
// stayed fully usable: measured, Close() followed by WaitOne(0) returned success. .NET's
30+
// WaitHandle.Close() is `=> Dispose()`, Dispose(bool) is `_waitHandle?.Close()`, and every
31+
// wait path then opens with `ObjectDisposedException.ThrowIf(waitHandle is null, this)`
32+
// (WaitHandle.cs:87-98, 118). The header here already CLAIMED Close "closes the handle",
33+
// so the documentation and the behaviour disagreed.
34+
//
35+
// std::atomic<bool> is 1 byte and 1-byte aligned, and it lands in padding these types
36+
// already had -- the sizes are unchanged, pinned by a layout test. Landed under SA-5 (a
37+
// call that succeeds today starts throwing) with SA-3's layout condition discharged.
38+
std::atomic<bool> closed_{false};
39+
40+
void ThrowIfClosed() const {
41+
if (closed_.load(std::memory_order_acquire))
42+
throw System::ObjectDisposedException("The handle has been closed.");
43+
}
2644
public:
2745
/** @param initialState If true, the event starts in the signaled state. */
2846
explicit ManualResetEvent(bool initialState = false) : signaled_(initialState) {}
2947

3048
/** Sets the event to signaled, releasing all waiting threads. */
3149
void Set() {
50+
ThrowIfClosed();
3251
{ std::lock_guard<std::mutex> lk(mutex_); signaled_ = true; }
3352
cv_.notify_all();
3453
}
3554

3655
/** Resets the event to non-signaled. */
3756
void Reset() {
57+
ThrowIfClosed();
3858
std::lock_guard<std::mutex> lk(mutex_);
3959
signaled_ = false;
4060
}
4161

4262
/** Blocks until the event is signaled. */
4363
void WaitOne() {
64+
ThrowIfClosed();
4465
std::unique_lock<std::mutex> lk(mutex_);
4566
cv_.wait(lk, [this]{ return signaled_; });
4667
}
@@ -52,6 +73,7 @@ namespace System::Threading {
5273
* @throws System::ArgumentOutOfRangeException if @p milliseconds is less than -1.
5374
*/
5475
bool WaitOne(intcs milliseconds) {
76+
ThrowIfClosed();
5577
WaitHandle::ValidateTimeout(milliseconds);
5678
std::unique_lock<std::mutex> lk(mutex_);
5779
// -1 (Timeout.Infinite) waits indefinitely; std::chrono's wait_for treats a
@@ -63,8 +85,13 @@ namespace System::Threading {
6385
return cv_.wait_for(lk, std::chrono::milliseconds(milliseconds), [this]{ return signaled_; });
6486
}
6587

66-
/** Releases resources (no-op; provided for .NET API compatibility). */
67-
void Close() {}
88+
/**
89+
* @brief Closes the handle. Every later Set, Reset or WaitOne throws
90+
* System::ObjectDisposedException.
91+
*
92+
* Idempotent, as .NET's is: Close() is `=> Dispose()` and disposing twice is defined.
93+
*/
94+
void Close() { closed_.store(true, std::memory_order_release); }
6895
};
6996

7097
} // namespace System::Threading

‎modules/threading/include/System/Threading/Mutex.hpp‎

Lines changed: 27 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99

1010
#include "SharpRuntime/SharpRuntimeHelper.hpp"
1111
#include "System/ApplicationException.hpp"
12+
#include "System/ObjectDisposedException.hpp"
1213
#include "System/Threading/WaitHandle.hpp"
1314

1415
namespace System::Threading {
@@ -29,6 +30,17 @@ namespace System::Threading {
2930
std::recursive_timed_mutex mutex_;
3031
std::atomic<std::thread::id> owner_{};
3132
std::atomic<int> depth_{0};
33+
// Ticket #1956 / cause T-G (SR-AUD-208). Close() was an EMPTY BODY, so a closed mutex
34+
// stayed fully usable: measured, Close() then WaitOne(0) returned success. .NET's
35+
// WaitHandle.Close() is `=> Dispose()` (WaitHandle.cs:87), Dispose(bool) closes the
36+
// SafeWaitHandle, and every wait path then throws ObjectDisposedException
37+
// (WaitHandle.cs:118). The header here already CLAIMED Close "closes the mutex handle".
38+
std::atomic<bool> closed_{false};
39+
40+
void ThrowIfClosed() const {
41+
if (closed_.load(std::memory_order_acquire))
42+
throw System::ObjectDisposedException("The handle has been closed.");
43+
}
3244

3345
void onAcquired() {
3446
if (depth_.fetch_add(1, std::memory_order_relaxed) == 0)
@@ -48,12 +60,13 @@ namespace System::Threading {
4860
Mutex(bool initiallyOwned, const std::string& /*name*/) { if (initiallyOwned) { mutex_.lock(); onAcquired(); } }
4961

5062
/** Acquires the mutex, blocking until it is available. */
51-
bool WaitOne() override { mutex_.lock(); onAcquired(); return true; }
63+
bool WaitOne() override { ThrowIfClosed(); mutex_.lock(); onAcquired(); return true; }
5264
/**
5365
* @brief Blocks until the mutex is available or millisecondsTimeout elapses; returns true on success.
5466
* @throws System::ArgumentOutOfRangeException if @p millisecondsTimeout is less than -1.
5567
*/
5668
bool WaitOne(intcs millisecondsTimeout) override {
69+
ThrowIfClosed();
5770
ValidateTimeout(millisecondsTimeout);
5871
// -1 (Timeout.Infinite) waits indefinitely; std::chrono's try_lock_for treats a
5972
// negative duration as already-expired, so it must be special-cased rather than
@@ -73,13 +86,24 @@ namespace System::Threading {
7386
* @throws System::ApplicationException if the calling thread does not own the mutex.
7487
*/
7588
void ReleaseMutex() {
89+
ThrowIfClosed();
7690
if (owner_.load(std::memory_order_relaxed) != std::this_thread::get_id())
7791
throw System::ApplicationException("Object synchronization method was called from an unsynchronized block of code.");
7892
onReleasing();
7993
mutex_.unlock();
8094
}
81-
/** Closes the mutex handle. */
82-
void Close() {}
95+
/**
96+
* @brief Closes the mutex handle. Every later WaitOne or ReleaseMutex throws
97+
* System::ObjectDisposedException.
98+
*
99+
* Overrides `Dispose()` rather than shadowing `Close()`, so the inherited
100+
* `WaitHandle::Close()` -- which is `{ Dispose(); }` -- reaches it. That is .NET's own
101+
* arrangement: `public virtual void Close() => Dispose();` (WaitHandle.cs:87). The
102+
* spelling `m.Close()` is unaffected.
103+
*
104+
* Idempotent, as .NET's is.
105+
*/
106+
void Dispose() override { closed_.store(true, std::memory_order_release); }
83107
};
84108

85109
} // namespace System::Threading

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

Lines changed: 33 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -399,7 +399,39 @@ namespace System::Threading {
399399
* still owns a mode, where .NET throws SynchronizationLockException -- is cause T-G
400400
* and belongs to approval-gated ticket #1956; it is deliberately not changed here.
401401
*/
402-
void Dispose() override { disposed_.store(true, std::memory_order_release); }
402+
/**
403+
* @brief Disposes the lock.
404+
* @throws System::Threading::SynchronizationLockException if the calling thread still
405+
* holds the read, write or upgradeable-read mode.
406+
*
407+
* Ticket #1956 / cause T-G (SR-AUD-203, dispose-while-held half). This used to set the
408+
* flag unconditionally, so disposing with a lock held succeeded and left the holder
409+
* owning a mode on a disposed object. .NET refuses
410+
* (`ReaderWriterLockSlim.cs:1250-1258`).
411+
*
412+
* @note **.NET performs TWO checks here and this port performs one**, which is stated
413+
* rather than glossed. The reference tests, in this order:
414+
* @code
415+
* if (WaitingReadCount > 0 || WaitingUpgradeCount > 0 || WaitingWriteCount > 0) throw ...;
416+
* if (IsReadLockHeld || IsUpgradeableReadLockHeld || IsWriteLockHeld) throw ...;
417+
* @endcode
418+
* The second is implemented here. The first needs per-mode WAITER counts, of which this
419+
* port has only `waitingWriters_` (added by SR-AUD-204); counting waiting readers and
420+
* upgraders is additional state on three more paths and is ticket **#2389**. The
421+
* narrowing is therefore a strict subset of .NET's -- this port never refuses a disposal
422+
* .NET would accept.
423+
*/
424+
void Dispose() override {
425+
auto& map = threadCounts();
426+
auto it = map.find(id_);
427+
if (it != map.end() &&
428+
(it->second.reader > 0 || it->second.writer > 0 || it->second.upgrade > 0)) {
429+
throw System::Threading::SynchronizationLockException(
430+
"The lock is being disposed while still being used. It either is being held "
431+
"by a thread and/or has active waiters waiting to acquire the lock.");
432+
}
433+
disposed_.store(true, std::memory_order_release);
434+
}
403435

404436
/** Returns whether the current thread holds a read lock. */
405437
[[nodiscard]] bool getIsReadLockHeldProperty() const {

0 commit comments

Comments
 (0)