Skip to content

Commit 1cd7425

Browse files
committed
fix(threading): AutoResetEvent and ManualResetEvent are WaitHandles (#1958, SR-AUD-209)
This closes #1958, the last of its eight members. Both types had no base class and no vtable, each carrying its own mutex, condition variable and signalled flag -- a third and fourth copy of logic EventWaitHandle already had. They are now what .NET declares: sealed classes deriving from EventWaitHandle whose ENTIRE BODY is one constructor (AutoResetEvent.cs:6-9, ManualResetEvent.cs:6-9). Neither declares a member of its own, so this deletes the duplicated bodies rather than adding a base to them. WHY IT MATTERED: because they were not WaitHandles, WaitHandle::WaitAll and WaitAny -- repaired by #1952 and documented ever since -- could not accept them at all. Not a wrong answer: the code did not compile. TWO THINGS THE FINDING DID NOT NAME, both required: 1. EventWaitHandle had no closed state. #1956 gave Mutex, AutoResetEvent and ManualResetEvent a closed_ flag; EventWaitHandle was its fourth case and was missed, so Close() reached WaitHandle's EMPTY Dispose() and did nothing. Deriving without fixing that would have silently reverted #1956 for both events. 2. EventWaitHandle::Set() lost wakeups -- it stored and notified without holding mtx_, so a waiter that had evaluated the predicate as false but not yet slept missed the notification. AutoResetEvent::Set() took the lock, so deriving would have INTRODUCED the race into a type that did not have it. Probed over 900 single-waiter rounds: EventWaitHandle lost 2, AutoResetEvent lost 0. cna holds this type by value in six places, all for async completion. Layout: both events 96 -> 112, EventWaitHandle 104 -> 112; consumers rebuild. 112 was measured after 104 was asserted and the build rejected it. The pin asserts the relationship -- both events must be exactly sizeof(EventWaitHandle) -- as well as the figures. WaitOne() returns bool rather than void, a widening at every call site. The initialState parameter loses its default, as .NET has none and it had zero call sites. Seven mutations, six caught. M6 (revert Set() to the unlocked form) is NOT caught and cannot be caught deterministically: ~0.2% per round means a bounded test would catch it about a third of the time. A first cut carried exactly such a 200-round case and it was REMOVED rather than kept, on the reasoning #1957/SR-AUD-201 and #2031 recorded. A multi-waiter amplification was tried and is invalid -- it reports 100% "loss" for the locked form too, because repeated Set() calls on an AutoReset event coalesce into one signal. Downstream: zero AutoResetEvent/ManualResetEvent sites in both consumers; all 14 hits are EventWaitHandle and need a rebuild, not an edit. cna builds against the sibling checkout on develop, so both repairs reach it at the merge. Gate: 17,594 run / 17,594 passed / 0 failed / 0 skipped across 38 executables, recounted from the per-executable logs (+4, Threading 523 -> 527). Build: build/ only, --parallel 2 throughout.
1 parent ea74a4b commit 1cd7425

7 files changed

Lines changed: 312 additions & 181 deletions

File tree

CLAUDE.md

Lines changed: 1 addition & 1 deletion
Large diffs are not rendered by default.
Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
<!-- SPDX-License-Identifier: MIT -->
2+
<!-- Copyright (c) Robert Vokac and contributors -->
3+
4+
# Migration: `AutoResetEvent` and `ManualResetEvent` are `WaitHandle`s (#1958 / SR-AUD-209)
5+
6+
**Landed:** 2026-08-19, branch `next`. **Ticket:** #1958, finding SR-AUD-209. **This closes #1958.**
7+
8+
## What changed
9+
10+
Both types had **no base class and no vtable**, and each carried its own mutex, condition
11+
variable and signalled flag — a third and fourth copy of logic `EventWaitHandle` already had.
12+
They are now what .NET declares:
13+
14+
```cpp
15+
class AutoResetEvent final : public EventWaitHandle { explicit AutoResetEvent(bool); };
16+
class ManualResetEvent final : public EventWaitHandle { explicit ManualResetEvent(bool); };
17+
```
18+
19+
That is not a paraphrase of the reference — it *is* the reference. .NET's
20+
`AutoResetEvent.cs` and `ManualResetEvent.cs` are nine lines each, and their entire bodies are
21+
22+
```csharp
23+
public sealed class AutoResetEvent : EventWaitHandle
24+
{
25+
public AutoResetEvent(bool initialState) : base(initialState, EventResetMode.AutoReset) { }
26+
}
27+
```
28+
29+
Neither declares a member of its own. `Set`, `Reset`, `WaitOne`, `Close` and `Dispose` are all
30+
inherited.
31+
32+
## Why it mattered
33+
34+
Because they were not `WaitHandle`s, `WaitHandle::WaitAll` and `WaitHandle::WaitAny` **could not
35+
accept them at all** — not "returned the wrong answer", the code did not compile. Those entry
36+
points were repaired by #1952 and documented ever since. SR-AUD-209 was the one divergence in
37+
`System::Threading` that left a documented API unusable.
38+
39+
## Two things the finding did not name, both required
40+
41+
**1. `EventWaitHandle` had no closed state.** #1956 gave `Mutex`, `AutoResetEvent` and
42+
`ManualResetEvent` a `closed_` flag so that `Close()` really closes; `EventWaitHandle` was its
43+
fourth case and was missed, so `Close()` there reached `WaitHandle`'s **empty** `Dispose()` and did
44+
nothing. Deriving the two events without fixing that would have **silently reverted #1956 for both
45+
of them**. The guard now lives in `EventWaitHandle` and the derived types inherit it, which is
46+
where .NET puts it too (`WaitHandle.cs:87-98,118`).
47+
48+
**2. `EventWaitHandle::Set()` lost wakeups.** It stored and notified **without holding `mtx_`**,
49+
so a waiter that had evaluated the predicate as false but had not yet atomically released the lock
50+
and slept missed the notification and blocked until some later `Set()`. `AutoResetEvent::Set()`
51+
took the lock, so deriving it would have **introduced** the race into a type that did not have it.
52+
53+
Measured with `build-probe/2209_probe1_lost_wakeup.cpp` over 900 single-waiter rounds:
54+
`EventWaitHandle` lost **2**, `AutoResetEvent` lost **0**. This is the type six `cna` data members
55+
hold **by value**, all of them for async completion — precisely the shape a lost wakeup hangs.
56+
57+
## Layout — consumers must rebuild
58+
59+
| type | before | after |
60+
|---|---|---|
61+
| `AutoResetEvent` | 96 | **112** |
62+
| `ManualResetEvent` | 96 | **112** |
63+
| `EventWaitHandle` | 104 | **112** |
64+
65+
`EventWaitHandle`'s growth was **asserted at 104 first and the build rejected it**: #1956's flags
66+
fitted into existing padding on three other types and that expectation was carried over rather
67+
than measured. It does not fit here.
68+
69+
This is a vtable and base-class change (SA-3 excludes it; the user approved it directly on
70+
2026-08-19) plus an ABI change on a type `cna` holds by value in six places, so **every consumer
71+
must rebuild**. The layout pin asserts the *relationship* — both events must be exactly
72+
`sizeof(EventWaitHandle)` — as well as the absolute figures, so it says "these declare no members
73+
of their own" rather than merely "these are 112 bytes today".
74+
75+
## Source changes
76+
77+
- **`WaitOne()` returns `bool`**, not `void`. Nothing that compiled stops compiling — ignoring a
78+
returned value is legal — so this is a widening at every call site.
79+
- **The `initialState` parameter has no default.** .NET's has none; the default this port
80+
published had **zero** call sites across `modules/`, `test/` and both downstream consumers.
81+
- Both classes are `final`, matching `sealed`.
82+
83+
## Downstream
84+
85+
Measured 2026-08-19: **zero** `AutoResetEvent` and **zero** `ManualResetEvent` sites in `cna` and
86+
`mobile-eggbert`. All 14 downstream hits are `EventWaitHandle`, six of them data members by value
87+
(`StorageDevice.cpp`, `NetworkSession.hpp`, `Gamer.hpp`, `Guide.cpp`, `AvatarDescription.cpp`,
88+
`LeaderboardReader.cpp`). Those need a **rebuild**, not an edit — and they gain both repairs
89+
above, which is the substantive downstream effect of this ticket.
90+
91+
## Evidence, including what is not testable
92+
93+
Seven mutations. Six caught; **M6 — reverting `Set()` to store without the lock — is not caught,
94+
and cannot be caught deterministically.** At roughly 0.2% loss per round, a bounded test would
95+
detect it about a third of the time, and a test that is intermittently green is not evidence
96+
(#2352). A first cut of the suite carried such a case at 200 rounds and it was **removed rather
97+
than kept**, on the reasoning #1957/SR-AUD-201 and #2031 recorded for their own window-closing
98+
mutations. A multi-waiter amplification was tried and is invalid: repeated `Set()` calls on an
99+
AutoReset event coalesce into one signal, so the harness reported 100% "loss" for the **locked**
100+
form too — it measures AutoReset semantics, not the race. The reasoning sits in the test file
101+
where the case would have been.

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

Lines changed: 27 additions & 91 deletions
Original file line numberDiff line numberDiff line change
@@ -2,107 +2,43 @@
22
// Copyright (c) Robert Vokac and contributors
33
// Portions based on .NET runtime API (MIT License, Copyright .NET Foundation and Contributors)
44
#pragma once
5-
#include <mutex>
6-
#include <atomic>
7-
#include <condition_variable>
85

9-
#include "SharpRuntime/SharpRuntimeHelper.hpp"
10-
#include "System/ObjectDisposedException.hpp"
11-
#include "System/Threading/WaitHandle.hpp"
6+
#include "System/Threading/EventResetMode.hpp"
7+
#include "System/Threading/EventWaitHandle.hpp"
128

139
namespace System::Threading {
1410

15-
using SharpRuntime::intcs;
16-
1711
/**
18-
* Represents a thread synchronization event that resets automatically after releasing a single waiting thread.
19-
*
20-
* Wraps std::condition_variable. Partial C++ counterpart of .NET System.Threading.AutoResetEvent.
21-
*
22-
* @note Status: Implemented
12+
* @brief Represents a thread synchronization event that resets automatically after
13+
* releasing a single waiting thread.
14+
*
15+
* C++ counterpart of .NET `System.Threading.AutoResetEvent`, which is
16+
* `public sealed class AutoResetEvent : EventWaitHandle` whose entire body is
17+
* `public AutoResetEvent(bool initialState) : base(initialState, EventResetMode.AutoReset)`
18+
* (`AutoResetEvent.cs:6-9`). It declares **no members of its own**; `Set`, `Reset`,
19+
* `WaitOne`, `Close` and `Dispose` are all inherited.
20+
*
21+
* @note **Derived since ticket #1958 / SR-AUD-209 (2026-08-19).** This class used to have
22+
* no base and no vtable, holding its own mutex, condition variable and signalled flag --
23+
* a third copy of logic `EventWaitHandle` already had. Because it was not a `WaitHandle`,
24+
* `WaitHandle::WaitAll`/`WaitAny` **could not accept it at all**, which is what made
25+
* SR-AUD-209 the one finding in the namespace that left a documented API unusable.
26+
*
27+
* `WaitOne()` consequently returns `bool` rather than `void`. Nothing that compiled stops
28+
* compiling -- ignoring a returned value is legal -- so the change is a widening at every
29+
* call site, but `sizeof` grows and a vtable appears, so consumers must rebuild.
2330
*/
24-
class AutoResetEvent {
25-
std::mutex mutex_;
26-
std::condition_variable cv_;
27-
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-
}
31+
class AutoResetEvent final : public EventWaitHandle {
4432
public:
45-
/** @param initialState If true, the event starts in the signaled state. */
46-
explicit AutoResetEvent(bool initialState = false) : signaled_(initialState) {}
47-
48-
/** Sets the event to signaled, releasing one waiting thread; the event then resets automatically. */
49-
void Set() {
50-
ThrowIfClosed();
51-
{ std::lock_guard<std::mutex> lk(mutex_); signaled_ = true; }
52-
cv_.notify_one();
53-
}
54-
55-
/** Resets the event to non-signaled. */
56-
void Reset() {
57-
ThrowIfClosed();
58-
std::lock_guard<std::mutex> lk(mutex_);
59-
signaled_ = false;
60-
}
61-
62-
/** Blocks until the event is signaled (then auto-resets). */
63-
void WaitOne() {
64-
ThrowIfClosed();
65-
std::unique_lock<std::mutex> lk(mutex_);
66-
cv_.wait(lk, [this]{ return signaled_; });
67-
signaled_ = false;
68-
}
69-
70-
/**
71-
* Blocks until signaled or timeout elapses.
72-
* @param milliseconds Maximum time to wait.
73-
* @return True if the event was signaled (then auto-resets); false on timeout.
74-
* @throws System::ArgumentOutOfRangeException if @p milliseconds is less than -1.
75-
* @note Verified against every sibling wait-handle type in this codebase
76-
* (ManualResetEvent, EventWaitHandle, Mutex, Semaphore, SemaphoreSlim, CountdownEvent,
77-
* ManualResetEventSlim), all of which call WaitHandle::ValidateTimeout(...) here. This
78-
* was previously the one call site missing it: a timeout below -1 (e.g. -2) silently
79-
* reached cv_.wait_for(..., milliseconds(-2), ...), which per C++'s negative-duration
80-
* semantics returns almost immediately instead of throwing.
81-
*/
82-
bool WaitOne(intcs milliseconds) {
83-
ThrowIfClosed();
84-
WaitHandle::ValidateTimeout(milliseconds);
85-
std::unique_lock<std::mutex> lk(mutex_);
86-
// -1 (Timeout.Infinite) waits indefinitely; std::chrono's wait_for treats a
87-
// negative duration as already-expired, so it must be special-cased.
88-
bool ok;
89-
if (milliseconds == -1) {
90-
cv_.wait(lk, [this]{ return signaled_; });
91-
ok = true;
92-
} else {
93-
ok = cv_.wait_for(lk, std::chrono::milliseconds(milliseconds), [this]{ return signaled_; });
94-
}
95-
if (ok) signaled_ = false;
96-
return ok;
97-
}
98-
9933
/**
100-
* @brief Closes the handle. Every later Set, Reset or WaitOne throws
101-
* System::ObjectDisposedException.
34+
* @param initialState If true, the event starts in the signaled state.
10235
*
103-
* Idempotent, as .NET's is: Close() is `=> Dispose()` and disposing twice is defined.
36+
* The parameter has no default, as .NET's has none. The default this port used to
37+
* publish had **zero** call sites, measured across `modules/`, `test/` and both
38+
* downstream consumers, so removing it migrates nothing.
10439
*/
105-
void Close() { closed_.store(true, std::memory_order_release); }
40+
explicit AutoResetEvent(bool initialState)
41+
: EventWaitHandle(initialState, EventResetMode::AutoReset) {}
10642
};
10743

10844
} // namespace System::Threading

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

Lines changed: 48 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
#include <mutex>
88
#include "SharpRuntime/SharpRuntimeHelper.hpp"
99
#include "System/Threading/WaitHandle.hpp"
10+
#include "System/ObjectDisposedException.hpp"
1011
#include "System/Threading/EventResetMode.hpp"
1112

1213
namespace System::Threading {
@@ -20,6 +21,18 @@ namespace System::Threading {
2021
std::mutex mtx_;
2122
std::condition_variable cv_;
2223

24+
// Ticket #1958 / SR-AUD-209. #1956 gave AutoResetEvent, ManualResetEvent and Mutex a
25+
// closed state; EventWaitHandle was its fourth case and was missed, so Close() here
26+
// reached WaitHandle's EMPTY Dispose() and a closed handle stayed fully usable. That gap
27+
// had to be closed before AutoResetEvent and ManualResetEvent could derive from this
28+
// class, or deriving would have silently reverted #1956 for both of them.
29+
std::atomic<bool> closed_{false};
30+
31+
void ThrowIfClosed() const {
32+
if (closed_.load(std::memory_order_acquire))
33+
throw System::ObjectDisposedException("The handle has been closed.");
34+
}
35+
2336
public:
2437
/**
2538
* @brief Constructs an EventWaitHandle with the specified initial state and reset mode.
@@ -56,20 +69,50 @@ namespace System::Threading {
5669
throw System::ArgumentException("Value of flags is invalid.", "mode");
5770
}
5871

59-
/** Sets the event to the signalled state. */
72+
/**
73+
* @brief Sets the event to the signalled state.
74+
* @throws System::ObjectDisposedException if the handle has been closed.
75+
*
76+
* The store and the notification happen under `mtx_`. Ticket #1958 / SR-AUD-209
77+
* measured that doing them WITHOUT the lock loses wakeups: a waiter that has evaluated
78+
* the predicate as false but has not yet atomically released the lock and slept misses
79+
* the notification entirely and blocks until some later Set(). Probed over 900 rounds,
80+
* the unlocked form lost 2 and the locked form lost 0 -- and this is the type six `cna`
81+
* data members hold by value, all of them for async completion, which is exactly the
82+
* shape a lost wakeup hangs.
83+
*/
6084
void Set() {
61-
set_.store(true, std::memory_order_release);
85+
ThrowIfClosed();
86+
{ std::lock_guard<std::mutex> lk(mtx_); set_.store(true, std::memory_order_release); }
6287
if (mode_ == EventResetMode::ManualReset)
6388
cv_.notify_all();
6489
else
6590
cv_.notify_one();
6691
}
6792

68-
/** Sets the event to the non-signalled state. */
69-
void Reset() { set_.store(false, std::memory_order_release); }
93+
/**
94+
* @brief Sets the event to the non-signalled state.
95+
* @throws System::ObjectDisposedException if the handle has been closed.
96+
*/
97+
void Reset() {
98+
ThrowIfClosed();
99+
std::lock_guard<std::mutex> lk(mtx_);
100+
set_.store(false, std::memory_order_release);
101+
}
102+
103+
/**
104+
* @brief Closes the handle; every later Set, Reset or WaitOne throws
105+
* System::ObjectDisposedException.
106+
*
107+
* .NET's `WaitHandle.Close()` is `=> Dispose()` and this port's base spells it the same
108+
* way, so overriding Dispose() is what makes Close() effective here. Idempotent, as
109+
* .NET's is.
110+
*/
111+
void Dispose() override { closed_.store(true, std::memory_order_release); }
70112

71113
/** Blocks until the event is signalled; auto-resets if the mode is AutoReset. */
72114
bool WaitOne() override {
115+
ThrowIfClosed();
73116
std::unique_lock<std::mutex> lock(mtx_);
74117
cv_.wait(lock, [this]{ return set_.load(std::memory_order_acquire); });
75118
if (mode_ == EventResetMode::AutoReset)
@@ -82,6 +125,7 @@ namespace System::Threading {
82125
* @throws System::ArgumentOutOfRangeException if @p milliseconds is less than -1.
83126
*/
84127
bool WaitOne(intcs milliseconds) override {
128+
ThrowIfClosed();
85129
ValidateTimeout(milliseconds);
86130
std::unique_lock<std::mutex> lock(mtx_);
87131
// -1 (Timeout.Infinite) waits indefinitely; std::chrono's wait_for treats a

0 commit comments

Comments
 (0)