Repository navigation
Fix the lost wakeup in resume_impl; remove generator<T> - #184
Merged
Merged
Conversation
resume_impl carried two halves of one lost wakeup:
* the entry path never re-checked is_busy() after cont.resume(), so a
behavior that advanced and immediately re-suspended on its next
co_await fell through to the empty-mailbox branch, try_block()
succeeded, and an actor suspended on a PENDING await was parked
(verdict `awaiting`, keep_scheduled = false -> the worker drops it);
* the entry blocked-check sat above the Q6 block, so once parked the
actor could never drain its ready continuation: every later resume
died at the blocked-check before reaching Q6.
Future completion is flag-only -- release_promise sets promise_released,
it neither pushes to the awaiting actor's mailbox nor enqueues it -- so a
bare scheduler->enqueue() was exactly the wake-up that could not help.
Hoist Q6 above the blocked-check and re-check is_busy() unconditionally
after cont.resume().
Replace the two work-in-progress lost-wakeup tests with a single
lost_wakeup_test.cpp. Its load-bearing assertion is on the VERDICT of the
drain step: it must be `resume`, not `awaiting`. Verified RED against
ca0378b's header (drain_verdict == awaiting, trace ends `consumer=awaiting`)
and green against this change -- the test it replaces passed against the
pre-fix header and so guarded nothing.
The multi-threaded soak is kept as the only coverage of this route under
contention, but now decides pass/fail by progress instead of a 15 s
wall-clock budget, so a loaded or sanitized runner no longer reports a
lost wakeup that never happened. It reads the completion count after
scheduler->stop() rather than before, and discharges the consumer's
recorded scheduling obligations instead of poking the producer on a blind
timer. Tagged [stress] and labelled for `ctest -LE stress`.
BREAKING CHANGE: actor_zeta::generator, actor_zeta::stream_error and every
piece of streaming plumbing are gone. Actor methods must return
unique_future<T>; send() always yields std::pair<bool, unique_future<T>>.
Why: the feature had no correct end-to-end consumption route. generator<T>
offers no synchronous next(), so the only way to advance a stream was
co_await -- and the advertised way to do that (`while (co_await gen)` inside
an actor method) went through the one await_transform overload that never
called propagate_awaited_state. That left the consuming behavior alive but
not busy, a state the scheduler cannot tell from idle: it either parked the
actor on a pending await (lost wakeup) or move-assigned over the live
coroutine frame on the next inbound message (use-after-free, plus leaked
dispatch/method frames and their shared_states). The producer also resumed
the consumer's coroutine on its own thread, which future_awaiters.hpp
explicitly forbids by design.
Four further defects shipped alongside: yield_awaiter claimed ownership of
one coroutine frame from two generator_states (double destroy());
take_producer_handle() returned a handle without clearing it; and
`co_yield stream_error{...}` ended the consumer loop before its body ran,
making in-loop has_error() checks dead code.
Nothing depended on it. No test in the tree ever asserted a yielded value --
test/generator's 21 cases all check lifecycle (valid/cancel/detach/move) and
the file says so at its "verify compilation" section; the three integration
tests asserted only REQUIRE(gen.valid()).
Removal detail worth flagging for reviewers:
* dispatch(): the surviving unique_future branch is de-indented verbatim.
The order get_result_promise() -> transfer_ownership() ->
invoke_actor_method() is load-bearing and now says so: both must run
before the first co_await, past which resume_impl's message_guard
destroys the message.
* dispatch_traits: is_coroutine is narrowed, but coroutine_parameter_check
stays -- it gates no_const_ref and no_rvalue_move_only (the dangling
const& guard and the GCC 11.4 operator new workaround) for every actor
method, generator or not.
* send(): both if-constexpr chains are flattened rather than having their
generator arm deleted, so no non-void path can fall off the end.
* message layout is unchanged: result_slot_, cleanup_fn_ and
ownership_transferred_ are all still used by the unique_future path.
Also refuse HAVE_EXPERIMENTAL_COROUTINES with an #error. generator.hpp used
std::coroutine_handle directly and reached every actor through
make_message.hpp, so that configuration used to fail loudly at compile time.
Without it only detail::noop_coroutine() remains -- which returns a NULL
handle on <experimental/coroutine>, undefined behaviour in a
symmetric-transfer position. A loud break must not become a silent one.
The dispatcher-pattern streaming tests are replaced by fetch_row_batch,
which keeps the two-actor row-forwarding hop under test and now asserts the
rows themselves, including the manager's prefix, instead of only that a
handle was constructed.
…k into one helper The guards added alongside the lost-wakeup fix -- `current_behavior_ && !current_behavior_.done()` on the empty-mailbox and no-progress paths, plus the widened tail condition -- targeted a state that cannot occur. Their premise was an "inter-await null window": a behavior transiently clearing its awaited chain between consecutive co_awaits while the scheduler looks on. That window does not span a resume(). propagate_awaited_state() runs inside await_transform, strictly before await_ready(), so publication precedes every suspension; and the whole chain advances synchronously inside one cont.resume(). A census of every suspend point reachable from a behavior_t chain found exactly one that suspended without publishing -- next_awaiter, reached through await_transform(generator<U>&) -- and that overload no longer exists. The full suite is byte-identical with and without the guards, which is the expected result for dead code. They were also misplaced for the case their own comment claimed: both sat below the entry blocked-check, so an already-parked actor never reached them. And they did not protect the one site that would have mattered -- the pre-pop gate -- because they only returned early on an EMPTY mailbox, while a use-after-free needs an inbound message to drive the loop to pop_front(). The tail condition reverts to plain is_busy(): is_busy() implies !done(), so `is_busy() || (handle && !done())` had two dead sub-terms and was exactly `!done()`. Fold the three verbatim copies of the park block into park(). The diff that added the guards is why: two sites got the same guard, the third got a different spelling, and the blocked-mailbox site got none. The helper also gives the never-(awaiting, keep_scheduled=true) invariant one place to live -- it was documented nowhere, though every copy depended on it.
…t terminate
An actor waiting on something its mailbox cannot deliver returns `resume`
with zero messages handled. Re-enqueueing carries no yield of its own --
work_sharing::enqueue is mutex, push_back, notify_one, and dequeue returns
immediately because the queue is non-empty -- so one such job pegs a core.
Yield only on that case: throughput exhaustion always reports
messages_processed > 0 and pays nothing.
scheduler_test_t had two independent defects, and fixing either alone leaves
it broken:
* run_once() re-queued a `resume` job at the FRONT, letting one spinning
actor monopolise the deque and starve every other job. Now push_back,
matching work_sharing::enqueue.
* stop() looped `while (run() > 0)`, and run() counts run_once() calls,
not progress. run_once() returns true whenever the deque is non-empty,
so against a legitimately spinning actor stop() never returned. It now
drains until a full sweep of the queue handles no messages, using the
messages_processed that run_once() previously discarded.
Both were latent: stop() had zero callers and every existing run_once() site
runs against an empty deque. Add the missing caller as a regression guard --
a consumer suspended on a future whose producer is never scheduled, which is
exactly the shape that used to hang. The assertion is that stop() returns.
Also drop dummy_worker: constructed in run_once() and never used.
BREAKING CHANGE: cooperative_actor::resume() and job_ptr::resume() are [[nodiscard]]. Call sites that discard the verdict no longer compile. `resume_result::resume` is an obligation, not a status: it means the caller must put the actor back in a run queue, and nothing else will. A future completing is flag-only -- it neither pushes to the mailbox nor enqueues the actor -- and send() only reports needs_sched when the inbox was blocked. The contract was always true; work_sharing honours it, and so does every real driver. What hid violations was the park: an actor suspended on a pending co_await used to block its own inbox, so the next send() reported needs_sched and the sender re-scheduled it. It no longer parks, so the verdict is the only signal left, and a driver that drops it strands the actor. Making that a compile error is cheaper than making it a hang. 197 call sites across test/, examples/ and benchmark/ discarded it. Most now say (void) -- deliberate, because the next line of a hand-staged sequence is the discharge. test/coroutine-threading/ is treated differently: it is the suite that exercises this machinery, and blanket-(void) there would preserve the exact blindness that let the lost wakeup hide. Its staged resumes go through resume_step(), and the FINAL resume of each sequence goes through resume_leaves_idle(), whose result is asserted: the actor must not still be asking to be scheduled once the sequence is over, i.e. the test's "N resumes are enough" premise must actually hold. All seven sequences pass, so they are now guarded rather than merely assumed. resume_leaves_idle() returns bool and deliberately contains no Catch2 macros: five of the seven call sites run on worker threads, and Catch2 v2 assertions are main-thread-only -- INFO/REQUIRE build a std::ostringstream, and calling them concurrently is a data race TSan reports at sstream. Threaded callers latch the result into an atomic and assert after join(), matching how this file already handles future_available. scheduler.hpp's nested shutdown_helper::resume is left alone: it is reached only through a function pointer, so the attribute would be inert there. Verified: full suite green under ASan (137/137) and TSan (137/137), and a clean rebuild with examples and benchmarks enabled produces no warnings.
Behavioral guidelines for reducing common LLM coding mistakes: think before coding, keep changes surgical and simple, surface assumptions and tradeoffs instead of silently picking one, and define verifiable success criteria. Unrelated to the actor-zeta changes in this branch; it was sitting staged in the working tree and is committed on its own so it does not ride along in them.
All three workflows declared `on.push.branches: [main]`, but this repository has no `main` -- its default branch is `master`. Pushes to the default branch therefore built nothing, and `pull_request` was the only trigger that ever fired, which is why every run in the history is a pull_request run.
build-asan/, build-tsan/, build-exc/ and build-bench/ are variant build directories; CMakeUserPresets.json is regenerated by every `conan install`.
… delete
lifo_inbox and linked_list derived their deleter from std::unique_ptr<T>,
i.e. std::default_delete. A mailbox::message lives inside a PMR block behind
a BlockHdr, and message_deleter frees the BASE pointer through the owning
resource -- so `delete p` handed the global allocator a pointer that is not
the allocation base.
Reproduced under ASan on the one reachable path, pushing into an already
closed inbox, which is what default_mailbox_impl does when a send races
shutdown:
AddressSanitizer: attempting free on address which was not malloc()-ed
lifo_inbox<message>::push_front(...) lifo_inbox.hpp:37
SUMMARY: bad-free in std::default_delete<message>::operator()
The other paths were correct only by accident: close_impl() passes the right
deleter explicitly and pop_front_impl() re-adopts into a message_ptr before
the wrong one can run. This takes that correctness off luck.
Both queues now take a Deleter parameter defaulting to std::default_delete,
so lifo_inbox<inode> and linked_list<job_ptr> are unaffected. message
declares its own deleter_type, and a static_assert makes instantiating
either queue with a mismatched deleter a COMPILE error -- when message first
declared it, the build failed on exactly the three broken instantiations.
In C++20 u8"..." is const char8_t[], which std::runtime_error does not
accept. These three literals sit behind `throw`, so they are only compiled
when exceptions are enabled -- a configuration nothing built, which is why a
C++17-era literal survived the C++20 migration unnoticed:
error: no matching conversion for functional-style cast from
'const char8_t[7]' to 'std::runtime_error'
With this, `-DEXCEPTIONS_DISABLE=OFF` builds and the whole suite passes in
that mode too. Prerequisite for putting it in CI.
Removes, all from unique_future<T> and its state: * the nested `awaiter` struct, * `unique_future<T>::operator co_await()`, * the free `operator co_await(unique_future<T>&&)`, * `behavior_t::resume()`, * `shared_state<T>::take_continuation()` (both specializations). Inside an actor coroutine nothing changes: `co_await std::move(fut)` still works, served by future_awaiter_mixin::await_transform. What goes is awaiting a future from a coroutine that is NOT an actor method -- that resumed an actor's frame on a foreign thread, outside the `running` bit, with no serialization. A foreign consumer suspended that way would also hang forever: completion is flag-only, and no cooperative_actor drives it. Drive such a future from outside instead: poll is_ready() + failed() + take_ready(). behavior_t::resume() and take_continuation() had zero callers; the continuation is claimed with an explicit exchange at each of the three sites that need it. BREAKING CHANGE: code doing `co_await` on a unique_future outside an actor coroutine no longer compiles.
unique_future caches the producing coroutine's handle, and final_awaiter
destroyed that frame on the very path where the consumer's future was still
alive and still holding it. Nothing cleared the member, so coroutine_handle()
handed back a pointer into freed memory:
AddressSanitizer: heap-use-after-free
READ of size 8 ... coroutine_handle<...>::done()
freed by ... final_awaiter::await_suspend(...)
Reproduced in two shapes. One is a driver loop re-reading the gate after the
drain that completed the coroutine. The other needs no loop at all:
initial_suspend is suspend_never, so a method coroutine with no reachable
suspension point runs to completion -- and frees its frame -- inside the call,
before the future is handed back.
This is not hypothetical: an external driver that hand-rolls this path needs a
guard for exactly this case, and ASan is what catches it when it is missing.
Two changes, both needed. The frame now parks at final_suspend and release()
reclaims it, which is what behavior_t has always done and what cppcoro,
libunifex, folly and Asio all do; steps 3 and 4 still destroy, because there
the future is already gone and nobody else would. And the getter withholds
the handle once the state reports ready -- a driver that caches the handle in
a local before its loop never re-runs a getter-side check, so parking is what
makes the cached copy valid.
Allocation accounting across seven scenarios is identical to before: no new
leak, no double free.
Invariant SETTLED-OUTCOME. is_ready() reports promise_released, and a promise that released without ever writing a value or an error produced a future that answered is_ready()==true, failed()==false and had nothing to take. take_ready()'s assert was the only thing between that and a read of unset storage -- and an assert is a precondition check, gone under NDEBUG, i.e. in every Release build that ships. I1, producer totality. release_promise() is the ONLY writer of promise_released in the library, so it is made total: a release with no result bit writes an error and folds error_set into the SAME release-ordered RMW. No observer can then see the released bit without a result bit, so `is_ready() => has_result()` holds at every instant, on every thread, in every build mode. I2, monotonic flags. take_value() now SETS `consumed` rather than clearing value_set. Clearing would make the repair fire on a state whose value was legitimately consumed; nothing but promise_finalizing is ever cleared. I3, holds_value() -- value_set and not error_set and not consumed. This, not is_ready(), is what a value gate should read. No public signature changes: 189 take_ready() and 218 is_ready() call sites in this tree are untouched, and no test asserted a resultless-released state. The test file also carries the deallocation-attribution rework, which concerns the same function: the previous `promise_won <= deallocation_count` assertion was a tautology that a release_promise() hard-coded to `return true` satisfied. Attributing each deallocation to a thread role makes the self-report checkable, and the mutant now fails it.
unhandled_exception() contained only assert(false) and RETURNED. Returning is not undefined -- it is SPECIFIED to mean "handled, carry on to final_suspend", so the exception was swallowed and the coroutine reported normal completion. In a Release build with exceptions on, that produced is_ready()==1, failed()==0 and a garbage value, with no diagnostic anywhere. The totality repair already removed the memory-safety half of this: the future now reports failed(). What was still lost is which failure it was. So capture std::current_exception() into the state and rethrow it at extraction -- in take_ready() and in both await_resume()s, before the has_error() gate, since a captured exception sets error_set and would otherwise trip it first. Rethrowing from await_resume is what makes a chain work: the exception surfaces at the awaiting coroutine's co_await, reaches ITS own unhandled_exception(), and is captured into its state. A distinct error code (errc::interrupted) is stamped alongside, so a consumer that only polls failed()/error() and never extracts -- a real pattern, not a hypothetical one -- still sees a real failure and can tell it apart from state_not_recoverable. Gated on __cpp_exceptions, the compiler's own answer, with an #error if it contradicts the project's EXCEPTIONS_DISABLE. That guard earned its keep immediately: the new test target sets -fexceptions per-target and the build failed until it dropped the project macro too. The test target is deliberately built with -fexceptions while the rest of the suite is -fno-exceptions, and compiles the library source itself rather than linking the -fno-exceptions archive, so the whole executable is one mode. It avoids Catch2 for the same reason.
An assert is a precondition check: it is compiled out under NDEBUG on purpose,
because the condition is declared impossible. These five were not preconditions
about program state -- they were questions about TYPES, decidable where they
were asked.
find_method_index() was the worst of them, and not even an assert: it silently
returned 0 for an unregistered method, so msg_id<Actor, M> collapsed onto the
first registered method, the matching `case` in behavior() duplicated another
label, and the compiler pointed at an unrelated line. Now:
error: static assertion failed ... this method is not registered in the
actor's dispatch_traits -- add it to `using dispatch_traits = ...`
The other four are the resource-extraction aborts in unique_future's and
behavior_t's promises. Zero arguments means nothing could carry a memory
resource -- a property of the coroutine's signature. Whether ANY argument can
supply one is a property of the argument types. Both now fail to compile.
The predicate is lifted out of extract_resource_impl() so the static_assert and
the runtime path share one definition and cannot disagree about a type; the
whole tree compiles against it unchanged.
What stays an assert is the part that really is runtime state: resource()
returning null.
Ten call sites in this tree -- and nine of the
ten passed `[]{ std::this_thread::yield(); }`, a pump that pumps nothing. That
is a bounded spin wearing a driver's clothes. Only one site had a real pump
(sched.run_once()).
It also carried the defect this branch has been removing everywhere else: it
looped on is_ready() and then called take_ready() without checking failed().
That cannot be fixed inside it -- it returns T by value, with no error channel
in the signature -- while CLAUDE.md advertised it as the canonical top-level
driver, i.e. taught the unsafe gate.
Replaced by one local helper per file rather than ten copies of a loop:
* test_cross_thread.cpp gets an std::optional-returning await_from_scheduler.
That return type is not decoration: three of its five sites run inside
consumer threads, where a Catch2 macro is a data race, so they fold the
failure into the counters the post-join assertions check.
* test/task/main.cpp, the one real pump, becomes an explicit bounded loop.
* the three examples share a helper that checks failed() and aborts loudly.
No CHANGELOG "Removed" entry: run_until_complete was added under [Unreleased]
and never shipped, so the Added entry simply goes.
The existing matrix builds seven configurations, all with
EXCEPTIONS_DISABLE=ON and none with a sanitizer. Both serious defects fixed on
this branch were silent without one -- a future handing out a freed coroutine
frame, and lifo_inbox freeing PMR memory through the global allocator -- and
neither an ordinary build nor the test suite notices either.
Three jobs, on every push and PR:
asan use-after-free and bad-free. Locally 144/144 in 150s.
tsan data races, including Catch2 macros called from worker threads,
which are a race in Catch2 v2 itself. Locally 144/144 in 313s.
exceptions EXCEPTIONS_DISABLE=OFF -- the mode the exception_ptr capture and
rethrow path only exists in. Locally 144/144.
Benchmarks are off in these jobs: a sanitizer makes the numbers meaningless
while adding minutes.
detect_leaks is off for now. Two leaks are already known -- a future abandoned
while its producer is still suspended never releases its coroutine frame -- and
leak checking would redden the job from day one and bury the memory-safety
signal it exists for.
A code review of this branch produced 15 findings; 14 are addressed here and in the commits above. The ones that live in tests, benchmarks and examples: Gates. Every readiness gate now checks failed(), not just is_ready(). The manual-drain helpers keyed on promise_released alone, which a promise dying without a value also sets -- under NDEBUG a cancelled producer passed the gate and the consumer read unset storage, returning 10 instead of 52 while reporting success. The timed benchmark loops and the hand-rolled waiters in the examples had the same hole. Assertions that could not fail. `verdict != resume_result::shutdown` is a tautology: shutdown comes only from the scheduler's internal shutdown_helper, never from cooperative_actor::resume(). Replaced with `!= resume`, which is the verdict that would actually mean an undischarged obligation. Staging that had collapsed. Replacing staged resume(1) calls with enqueue+stop() made several drive steps process zero messages, because stop() drains to quiescence. Restored per-message driving and pinned it with `REQUIRE(info.messages_processed == 1)`, which is what catches a stage silently becoming a no-op. drive() now returns whether it discharged the resume obligation instead of dropping it silently at the cap -- both outcomes are legitimate here, so the answer is returned rather than asserted. And the ~100 `(void)` casts are gone, project-wide ban. They were not noise: release_promise() reports whether IT deallocated the state, and asserting that turned up three sites where the answer is true, pinning the Last-One-Out protocol that the cast had been hiding. Not addressed: examples/balancer routes by round-robin rather than by key, so 2 of every 3 lookups miss. Pre-existing, unrelated to this branch.
A pass over every comment touched on this branch, plus the four docs. Removed the narration -- "the review found", "PR #182 fix lives here", "this is a PURE refactor", "these tests will NOT COMPILE until the document is implemented", stale TODOs. Kept every "this used to be X, which caused Y": that is why a guard exists, and a guard whose reason is deleted gets deleted next. Six comments were not verbose but WRONG, which is worse: * `task::promise_type` and `sync_wait` named as consumers of the awaiter mixin -- no such types exist anywhere in header/. * `header/actor-zeta/core.hpp` in CLAUDE.md's file tree -- no such file. * `set_coroutine()` cited in a leak test -- no such symbol. * `docs/actor-zeta-race-comprehensive-fix.md` referenced three times, with fourteen pointers into its sections -- the file does not exist. * future_awaiters.hpp listed a "generic passthrough await_transform" among its members, while two other comments in the same file say there is deliberately none, and no such code exists. * shared_state.hpp asserted CI pins EXCEPTIONS_DISABLE=ON everywhere -- false since the sanitizer workflow added an OFF entry, and it denied the existence of the one configuration its #ifdef block serves. Deduplicated where a reader would meet the same text twice in one file -- notably shared_state<void>, which carried verbatim copies of the primary template's docs, up to 19 lines each. Copies in separate standalone examples were left alone: nobody reads those files in sequence. Docs follow the code: no surviving reference to run_until_complete or run_loop.hpp, and the cancellation API that no longer exists is gone from the guide.
behavior_t.hpp:136,139 referenced detail::has_resource_method, which was defined only in future.hpp and not included here. The file had its own near-identical concept, has_resource_method_behavior, three lines up. Our tests never caught it: they include the umbrella <actor-zeta.hpp>, which pulls future.hpp in first. A consumer that includes detail/behavior_t.hpp directly sees the other order, and stops building. Rather than point behavior_t.hpp at its own copy, give the concept one home in detail/type_traits.hpp, which both files already include, and delete both copies. A duplicated concept is what allowed one of them to drift out of scope in the first place. Two more headers were never self-sufficient: dispatch_traits.hpp used assert without <cassert> and needed a complete actor::address_t; implements.hpp and send.hpp inherited both failures through it. The new test/header-selfsufficiency target generates one translation unit per .hpp by walking header/, so a newly added header is covered without anyone remembering to list it. 51 of 56 headers passed before this commit; all 56 pass now.
result_storage keeps the value in `union storage_t { char dummy_; T value_; }`
with a separate has_value_. take() and both get() overloads asserted has_value_
and then read storage_.value_ regardless -- so under NDEBUG a state with an
error and no value move-constructed a T out of bytes that never held one and
ran ~T() on them. Silent undefined behaviour, in the build that ships.
The state is ordinary. send() to an actor whose mailbox is closing returns
queue_closed; ~message runs cleanup_fn_, which does set_error(operation_canceled)
+ release_promise(). is_ready() then reports true -- it is the promise_released
bit -- while no value was ever written.
An assert cannot be the fix here: NDEBUG is precisely what turns the miss into
UB. T take() has no way to return "nothing" either. So the extraction points
refuse unconditionally, in every build, and say why -- is_ready() is not a value
gate, failed() is.
co_await is an extraction point too. Both awaiters asserted !has_error() and
then took the value, so the same state reached through the actor's own drain
path had the same UB. They refuse now as well; cancellation is observed by
polling failed(), which is what the external-drive example already documents.
behavior_t::is_awaited_ready() is left alone deliberately, with a note: it gates
on promise_released and must keep letting an errored state through, or an actor
awaiting a settled-with-error future would never drain and park forever. The
refusal belongs at the extraction, not at the gate.
The test builds with -DNDEBUG whatever the build type says, and traps the
refusal with a SIGABRT handler so the outcome reaches ctest as an exit code
rather than a crash. Mutation-checked: dropping the guard in take() turns it
red again.
The shape could never work. send() returns {needs_sched, future}, where
needs_sched is the obligation to put the target in a run queue and nothing in
the library discharges it. The pair awaiter stored the bool and handed it back
from await_resume() -- after the wait -- but the wait cannot end until the
target has run, and a coroutine holds an address_t and no scheduler, so it
cannot schedule the target itself. The actor spun in the run queue forever with
no diagnostic.
Not "wrong under some conditions": the bool is delivered past the point where
it was needed, always. So the overload stays, and its body is a static_assert
that names the two-step form. Deleting it outright would have produced "no
matching function for call to await_transform", which says nothing.
Thirteen sites in test/dispatcher-pattern used the shape --
manager_dispatcher.hpp x12 and client.hpp x1 -- all written as
`auto [_, r] = co_await send(...)`, i.e. dropping the obligation as well. They
now use the idiom the same file already documents at aggregate_sizes(): consume
the pair in place, then co_await the future on its own. The suite passed before
only because main.cpp enqueues every actor by hand at every step.
That unconditional enqueue stays. It is not a workaround: none of those actors
holds a scheduler, so the manual driver IS the discharge. Its comment now says
so, and says what it used to cover for.
The new test/co-await-send-prohibited target checks both directions -- the
forbidden shape through try_compile at configure time (a test that only ran the
right shape would not notice the wrong one coming back), and the two-step form
as an ordinary test.
try_block() had no counterpart. park() blocks the inbox on the way out, and nothing unblocked it on the way in -- only a producer's push_front ever did, as a side effect. So blocked() stopped meaning what the rest of the code reads it as, for the whole duration of a run that did not start with a send. blocked() encodes "no job exists for this actor and the next producer owes the scheduling". Neither half holds once resume_impl is running: we hold the `running` bit, so a message arriving now is drained by this instance, and one landing after the drain is picked up by scheduled_while_running. Left blocked, a concurrent send() takes the unblocked_reader branch and is handed needs_sched = true for an actor that is already running -- a second job node for one actor, which try_acquire_running then has to absorb at the cost of two wasted rounds. Reached whenever something other than a send drives the actor: a bare scheduler->enqueue(), or the manual resume() loops in examples/external-drive. The CAS fails harmlessly when the inbox was not blocked, which is the ordinary case. The two blocked() checks inside resume_impl are now unreachable with default_mailbox -- probed across the whole suite, neither fired once. They stay because MailBox is a template parameter and a mailbox that blocks itself would revive them. Also corrects the Q6 comment, which justified the hoist by an actor "already parked with a since-completed future". No such actor exists: try_block() is called only from park() and the constructor, park() is reachable only with !is_busy(), and at construction there is no behavior yet. The hoist's real job is the re-check that keeps a live behavior out of park().
enqueue_impl checked is_destroying() and then called mailbox().push_back().
The destructor published `destroying` and waited -- but the wait covered only
the `running` bit, and a sender is not running. So the destructor could finish,
~default_mailbox_impl could free the queues, and a sender already past the gate
would write into them.
Reproduced under ASan, deterministically:
heap-use-after-free ... READ of size 8 thread T1
#2 lifo_inbox::push_front lifo_inbox.hpp:35
#3 default_mailbox_impl::push_back_impl default_mailbox.ipp:14
#6 cooperative_actor::enqueue_impl cooperative_actor.hpp:128
freed by thread T0 in ~cooperative_actor
The is_destroying() check is what makes this a defect rather than a contract
violation: it exists so a concurrent send is safe during teardown, and it says
"do not start" without ever saying "wait for the ones that already started".
The gate and the registration are now one read-modify-write on state_, which
grew from uint8_t to uint32_t: the three flags keep the low bits and the count
of in-flight senders lives above them. One word means one modification order,
so either a sender's registration precedes the destructor's read of the count
or `destroying` precedes the sender's read of the flags -- the two sides cannot
miss each other, and neither seq_cst fence is needed. Two words would have
needed both.
uint32_t rather than the five spare bits of uint8_t: those cap the count at 31,
and the 32nd registration would carry into a bit that does not exist, leaving
the count reading zero with senders still inside.
try_schedule_after_enqueue is not duplicated but moved: renamed
leave_and_maybe_schedule, it now always CASes, applying sub_sender and claiming
the `scheduled` bit in the same step. Its two early `return false` exits became
branch conditions, because a registered sender must deregister whatever the
answer about the bit is -- and splitting the two would leave a window where the
sender is uncounted and has not claimed yet, which is exactly when the
destructor is entitled to proceed. The selection is bit-for-bit the old one,
including reading `!is_running` from the pre-CAS value, so the park() window
still yields needs_sched == false and a single job node. enqueue_impl's switch
goes with it: three branches that asked only whether the result was
unblocked_reader.
Two places had to learn about the count: try_acquire_running's debug assert
compared whole-word enumerators, and make_state() rebuilds the word from zero
(its one call site now asserts the count is already drained).
What is guaranteed is narrow: a sender that has REGISTERED is waited for. One
still between its first load and a successful registration holds a raw pointer
to an object that may already be gone, and nothing here can change that.
Verified: 149/149 normal and under ASan; the packed protocol reviewed against
the modification order of state_, with TSan and ASan runs of 21 targets plus a
six-parked-senders stress and a park()-window probe.
exponential_backoff computed `1 << (attempt - kYieldPhaseEnd)` and then clamped
the RESULT to kMaxSleepMicroseconds. The shift itself was never bounded:
- at attempt 41 the exponent is 31, so `1 << 31` is INT_MIN, which compares
below the cap and becomes a negative duration that does not sleep at all --
the backoff silently turns into a busy loop;
- from attempt 42 the exponent reaches 32, which is undefined behaviour on a
32-bit int.
Not theoretical. wait_for_activity_to_drain() spins here for as long as a
resume or an in-flight sender takes, so any wait over roughly 22ms gets there,
and test/shutdown-sender-race parks a sender for 50ms on purpose:
cooperative_actor.hpp:125:30: runtime error: shift exponent 32 is too large
for 32-bit type 'int'
20 runs out of 20. CI would not have caught it -- ubuntu_sanitizers has asan
and tsan jobs but no ubsan one.
The sleep already saturates at attempt 20 (1 << 10 = 1024 exceeds the cap), so
capping the exponent there loses nothing and makes the shift well-defined for
every attempt.
All three cases in test_needs_scheduling_race.cpp detected a timeout, rescued
the actor with an unconditional scheduler->enqueue(), and then asserted against
a count that subtracted the rescues back out:
REQUIRE(processed_count >= total_sent - timeouts_detected);
A genuine strand printed a WARN and passed. The rescues are gone, the
subtraction is gone, and the senders now honour needs_sched and nothing else --
a timeout fails the test.
The file's header described a race that does not exist. It claimed
try_schedule_after_enqueue "returns FALSE (does not set scheduled flag!)" for a
running actor; it always sets the bit and only suppresses the return value.
Both sides read-modify-write the same atomic, so its modification order totally
orders them and each outcome leaves exactly one job. The header now says that,
and says these are guards rather than repros.
Guards are not enough on their own here. Mutating leave_and_maybe_schedule to
skip the bit for a running actor -- precisely the timeline the old header
described -- leaves all three of them green: the window between park()'s
try_block() and ~resume_guard's CAS is a handful of instructions and they only
reach it by luck.
So test/park-window drives it deterministically, with a mailbox that parks the
runner inside the window. It runs two phases, because the window is guarded
twice and one phase alone would only ever exercise the first:
- park inside try_block_impl(): the sender's push unblocks the inbox, so
check_race_window() downgrades awaiting -> resume by itself;
- park inside blocked_impl() during check_race_window(), returning the value
read before the sender arrived: check_race_window() then short-circuits and
park() commits to `awaiting`, leaving nothing but the `scheduled` bit the
sender set and the upgrade in ~resume_guard.
The second phase goes red under that mutation and green without it; the first
stays green either way, which is the point of running both.
150/150 normal and under ASan.
resume() returned `done` when try_acquire_running() failed. `done` means finished. What had actually happened is that another thread holds `running`, try_acquire just set the scheduled bit on its behalf, and that thread will discharge the obligation by returning `resume`. The verdict this caller owes is "drop your node, the wakeup belongs to somebody else" -- which is what `awaiting` means, and what the worker already does with it. Harmless today only by accident: worker::run reacts to `done` by calling policy_.after_completion(), whose hook is empty in the shipped unprofiled policy. A profiling policy that retires an actor there, or an external driver that treats `done` as terminal -- which is a reasonable reading of the name -- would retire an actor that is merely contended. The test needs no threads. Re-entering resume() from inside behavior() is deterministic: the actor's own `running` bit is set, so the nested call takes the already-running branch every time. It pins three things -- the nested verdict is `awaiting` with zero messages, the outer verdict is `resume` (the scheduled bit the nested call set must end up with a job behind it), and the actor is still reachable afterwards. Restoring `done` turns it red.
Parking a behavior that is still suspended on a co_await is the lost wakeup the whole ordering in resume_impl exists to prevent: `awaiting` pairs with keep_scheduled = false, so the actor leaves with no job in any queue and a continuation nobody will ever drain. Every call site is dominated by an is_busy() check that returns `resume` first, so the state is unreachable -- but only as a property of the control flow, which a future edit can break without anything noticing. The assert makes it checked. done() is "no behavior, or one parked at final_suspend", i.e. exactly the complement of the state in question. Zero cost under NDEBUG; it did not fire once across the suite. No test accompanies it, and that is deliberate. I wrote one -- a single-threaded driver pairing each verdict with what the next send() reports -- and then mutation-checked it: removing the awaiting->resume fix-up, removing ~resume_guard's `|| is_scheduled(current)`, and removing park()'s downgrade all left it green. A sequential driver cannot reach any of those paths, because all three need a send that lands while `running` is held. A test that cannot fail is what the previous commit was about removing, so it is not going in. Of those three lines, only the fix-up is load-bearing, and test/park-window and test/contended-resume both catch its removal. The other two are redundant with it: ~resume_guard's CAS is on the same word a sender uses to set the bit, so a concurrent set forces a retry and scheduled_while_running ends up carrying the same information; and park()'s downgrade only fires in a window that guarantees the sender saw unblocked_reader and therefore set the bit. Both stay -- they are a single instruction each and the argument that they are redundant is exactly the kind that should have a belt.
Four public ways to finish a promise -- set_value(U&&), set_value() for void,
error(ec), and dying with no outcome -- each ended with the same eleven lines:
publish promise_finalizing, release_promise(), bail if it deallocated,
try_complete_finalize(), bail if that says the future got there first, null the
pointer. Character-identical once the comments are stripped; the only real
difference was which result each wrote first.
settle() is those eleven lines, once. The four exits are two lines each now.
Two details worth naming rather than burying:
- the short-circuit IS the logic. release_promise() returning true means it
deallocated, so try_complete_finalize() must not run at all. Its own answer
then only says whether the future side deallocated instead -- and the
promise is finished with the state either way, which is why the old code's
three `return`s all converged on the same line. `(void)` is banned here, so
the answer is named and [[maybe_unused]], the same idiom message.hpp uses
for release_promise().
- set_value(U&&) no longer materialises a T. It used to do
set_value(T(std::forward<U>(value))), which built a T and then moved it into
emplace(); shared_state::set_value forwards since the specializations were
merged, so the argument goes straight through. One move gone from every
cross-actor reply carrying a non-trivial type.
final_awaiter::await_suspend keeps its own copy of steps 2-4 deliberately: it
takes the continuation first and its two early exits destroy the frame and
return noop_coroutine() rather than nulling a pointer. Only the prefix is
shared, and lifting four lines out of twelve would cost more than it saves.
152/152 in the normal build, with EXCEPTIONS_DISABLE=OFF, and under ASan.
There was no way to move an exception from one shared_state into another. promise<T> could write a value or an error code, so anything caught had to be flattened to a code on the way across -- and the code it would get is broken_pipe, from the destructor's fallback, which says nothing about what happened. Public, because filling a promise by hand is a supported pattern rather than an internal detail: a router takes msg->get_result_promise<T>() and completes it itself, as examples/delegation and examples/balancer do. A router that catches something needs this. Symmetric to error(ec) in shape and in position. set_exception() stamps errc::interrupted alongside the exception_ptr, so a consumer that only polls failed()/error() -- never extracting -- still sees a real failure and can tell it apart from "released without an outcome". Guarded on __cpp_exceptions because shared_state::set_exception exists only there: with -fno-exceptions the compiler emits no catch wrapper for a coroutine body, so nothing could ever be captured to pass in. Six lines, because the two commits before this one made them available -- the merged shared_state gave set_exception for every T including void, and settle() is the producer's exit written once. Written against the old code it would have been thirteen, eleven of them a fifth copy of that exit. Tested directly for both T and void in test/exception-propagation: catch, hand it to the promise, then check the polling view (failed, errc::interrupted) and the extracting view (take_ready rethrows the original). 152/152 in the normal build and with EXCEPTIONS_DISABLE=OFF.
A method that threw did not reach the caller. dispatch's co_await rethrew into its own body, the body unwound, and result_promise -- a local -- was destroyed before any handler could run. ~promise settled the caller with broken_pipe and no exception, while dispatch's own state captured the exception that nobody reads. The caller was told something failed and never what. Then the exception reached behavior_t's co_await of dispatch(), whose unhandled_exception() is assert(false) + std::terminate(). Every example and every test in the repo writes `co_await dispatch(...)`, so with exceptions enabled a user throw killed the process. The new case in test/exception-propagation exited 134 -- SIGABRT -- before this change. The catch settles the caller's promise with the exception instead. set_value never ran, so the promise is still unsettled and this is the only outcome it will get. The caller now sees the original exception on extraction, and errc::interrupted rather than broken_pipe on a poll. Guarded on __cpp_exceptions rather than unconditional: with -fno-exceptions the compiler emits no catch wrapper for a coroutine body at all, so there is nothing to catch, and promise<T>::exception() does not exist there either. The #ifdef sits mid-body, which is ugly -- but it is library code, not anything a user writes. This also removes the reason the process terminated, since nothing escapes dispatch any more. behavior_t::unhandled_exception() still needs fixing for a throw from behavior() itself, past dispatch; that is the next commit. 152/152 in the normal build, with EXCEPTIONS_DISABLE=OFF, and under ASan.
…ocess behavior_t::unhandled_exception() was assert(false) + std::terminate(). Since dispatch() now catches, the only way to reach it is a throw from behavior() itself, past dispatch -- but that still ended the process over a bug in user code. The new case in test/exception-propagation exited 134 before this change. It is discarded, because there is nowhere to put it: behavior_t is the root of the await chain, it has no shared_state, and its result is read by nobody. So unlike unique_future<T>'s promise there is no state to capture into and no consumer to rethrow at. The choice was to kill the process, to invent a channel, or to let it go. It does not go quietly. The handler rethrows into a local catch purely to read what() -- this is the last place the exception exists -- and prints it along with what to do instead: move the work into a dispatched method, where a throw reaches the caller's future. A failure that vanishes without a trace is worse than one that stops the process, and that principle is already why refuse_valueless_extraction() prints before it aborts. The coroutine then proceeds to final_suspend and parks, so done() is true and the actor goes on to the next message. The test checks exactly that: the message was taken, and the actor still handles the next one. Under -fno-exceptions the branch stays assert + terminate. No catch wrapper is emitted for a coroutine body there, so reaching it means the build configuration disagrees with itself, not that user code threw. 152/152 in the normal build, with EXCEPTIONS_DISABLE=OFF, and under ASan.
The suite grew alongside the fixes, so it ended up testing the shapes those
fixes made work and nothing else. Three gaps, all of them in the parts most
likely to break:
- Every throw happened BEFORE the method's first co_await, so await_ready()
was always true, the continuation was never parked, and the resume path was
never exercised at all. The new case parks the method on a pending gate --
asserted, not assumed: the verdict is `resume` and the caller is still
waiting -- then settles the gate by hand, and only the resumed body throws.
That sends the exception through the Q6 drain rather than an await_ready()
shortcut.
- Only unique_future<int> was used, so the void branch of
owning_awaiter::await_resume -- the one with nothing to return -- never ran.
Now a unique_future<void> awaits another one.
- Nothing ran on a scheduler. The cross-thread case uses a real
sharing_scheduler with two workers, polls the future from the main thread,
and stops the scheduler before the actor goes.
A fourth gap closed itself: owning_pair_awaiter no longer exists, since the
pair<bool, future> overload became a static_assert.
Also links Threads::Threads. The target compiles source/src.cpp itself, which
pulls in launch_thread, and now runs a scheduler too -- it happened to work on
macOS and would not have on glibc older than 2.34.
Library code untouched. 152/152 in the normal build, with EXCEPTIONS_DISABLE=OFF
and under ASan.
The MSVC branch stripped every /EH and then added /EHsc back, which turns
exceptions ON, while also defining __EXCEPTIONS_DISABLE__. shared_state.hpp
raises a #error on exactly that disagreement, so EXCEPTIONS_DISABLE=ON -- the
default -- did not build on MSVC at all. There is no Windows job in
.github/workflows, so nothing noticed.
Removing every /EH is how MSVC is told not to; _HAS_EXCEPTIONS=0 is the half
the MS STL reads. The #error then stays quiet, which was checked in both
directions: consistent configurations compile, a disagreeing one still fails
with its message.
Second defect in the same block, and this one is not MSVC-specific: three
string(REGEX REPLACE ...) calls expanded ${CMAKE_CXX_FLAGS} unquoted. With the
variable empty that is a hard CMake error --
string sub-command REGEX, mode REPLACE needs at least 6 arguments.
-- so configuring without a toolchain that pre-seeds the flags would have
failed outright. It survived because the conan toolchain always sets some.
Quoted in the MSVC /EH and /GR calls and in the GNU/Clang -frtti one.
Not verified on a real MSVC: there is none here, and adding a Windows CI job
blind is a separate decision.
152/152.
None of this is reachable. shutdown_guard_t is the substantial one. It existed to call begin_shutdown() automatically for every actor, and test_shutdown_aggressive.cpp explained that this made a teardown race "IMPOSSIBLE to reproduce" because the guard runs "before base class destructor". The ordering was never real: shutdown_guard_ was declared FIRST among the members, so it was destroyed LAST -- after state_, mailbox_ and current_behavior_ had already gone. It ran after everything it claimed to protect, on members that no longer existed, and ~cooperative_actor's own body had already published `destroying` and waited. begin_shutdown() goes with it; the guard was its only caller. The comment that drew a conclusion from that ordering is rewritten to say what actually protects the teardown -- the destructor body's wait_for_activity_to_drain(), which waits out the thread holding `running` and any sender already past enqueue_impl's gate -- and that the file is a stress test rather than a proof. worker::execute_later() and work_sharing::internal_enqueue() are a dead chain: nothing called the first, it was the only caller of the second, and the second was a byte-for-byte copy of external_enqueue. Also gone: shared_state::flags_snapshot(), result_storage::empty() and has_value(), and state_flags::both_released together with the check that compared the constant to its own definition. Kept on purpose: holds_value() is I3's extraction predicate with six checks in tests, message::has_result_slot() is a reasonable public query on a public type, and foreach_resumable is live -- scheduler.hpp calls it during stop(). 152/152 in the normal build, with EXCEPTIONS_DISABLE=OFF and under ASan.
The comments and CI notes named specific consuming projects. Nothing in this repository depends on them, and a library should describe the shape it supports rather than who happens to use it. The external-drive test in particular read as a transcription of two other codebases; it now says what the driver pattern is.
…from Both places already explain why they are correct. Neither checked the premise. resume_impl's mailbox().try_unblock() is justified by "we hold `running`, so a message arriving now is drained by this instance". Without the bit nobody is committed to draining, and clearing the blocked tag hands the next sender `success` and needs_sched == false -- an actor stranded with a message in it. resume() acquires the bit before calling in, so a failure here means the call moved. dispatch()'s get_result_promise() casts result_slot_ without checking it, so a message that arrived without one yields a promise over nullptr. make_message() always installs a slot and a restamping router carries it along, so the state is unreachable -- but settle()'s own assert would fire later and point at the promise, while this one points at the message. Both were verified to be evaluated and true rather than dead text: inverting them turns 49 of the 152 tests red. 152/152 in all four configurations -- normal, EXCEPTIONS_DISABLE=OFF, ASan and TSan.
The sanitizer matrix had asan and tsan, and neither sees undefined behaviour that is not a memory or threading fault. exponential_backoff() was shifting by 32 on every single run of the shutdown-sender-race test -- and by 31 one attempt earlier, producing INT_MIN microseconds and a sleep that does not sleep -- while both jobs stayed green. That is the gap this closes. -fno-sanitize-recover=all so a report fails the job. The default prints and carries on, which for a matrix entry that exists to catch silent UB means it would report nothing anybody looks at. Verified locally with the same flags on clang: 152/152, no reports. And the configuration is not vacuous -- restoring the old exponential_backoff body brings back cooperative_actor.hpp:135:30: runtime error: shift exponent 32 is too large for 32-bit type 'int' Not verified on gcc-13, which is what the job actually runs; there is no gcc on this machine. gcc's ubsan is stricter than clang's in places, so the first run may find something.
…in release Six CAS loops in this file each carried the same six lines -- a backoff step, an increment, and an #ifndef NDEBUG block -- differing only in the message. That is the duplication worth removing here. Not the loop skeleton. The seven loops are not the same shape: ~resume_guard accumulates a side effect after a failed CAS, operates on a reference member rather than state_, and they return three different things. A common template would need a retry callback, which is the same code behind a lambda -- and the invariant arguments this file's comments rely on are about these loops as written. The #ifdef goes entirely. Both of its branches stopped the process: assert in debug, a bare std::terminate() in release. So it was never a debug-only check, just one refusal written twice, silently the second time -- and release is where you least want it silent. It now prints which loop spun and why that means a broken state machine rather than contention, then aborts, in both builds. The cost is one comparison against a constant, on a path that has just done an atomic read-modify-write. Checked that the guard did not become dead text: with kMaxCasAttempts set to 1 it prints and aborts. Also adds <cassert>, <cstdio> and <cstdlib>, which this header never had despite a dozen asserts in it. It compiled standalone only because behavior_t.hpp brings <cassert> in, and <cstdio> only since the commit that made behavior_t report an escaped exception -- so the self-sufficiency test was passing by luck here. 152/152 in all five configurations: normal, EXCEPTIONS_DISABLE=OFF, ASan, UBSan and TSan.
…eaking changes
Four changes break calling code and each gets its own guide with before/after:
- co_await send(...) no longer compiles. It never could complete -- the
needs_sched half is delivered only after the await, and the await cannot end
until someone has scheduled the target with it. The guide gives the two-step
form, and the shorter variant for a target that drives itself.
- Extraction refuses on a future that holds no value, in every build, where it
used to assert and then read an inactive union member anyway. The guide says
what is_ready() actually reports and to gate on failed().
- A contended resume() reports `awaiting` rather than `done`, so a driver that
treated `done` as "retire this actor" now sees the verdict that always meant
"drop the node, somebody else owns the wakeup".
- Exceptions reach the caller and stop killing the process. The guide covers
both consumer shapes -- extracting, which rethrows the original, and polling,
which sees errc::interrupted -- and says what happens to a throw from
behavior() itself, which has no caller to reach.
Plus the rest under Fixed, Added and Changed: header self-sufficiency, the
teardown use-after-free, the backoff shift and the ubsan job that would have
caught it, try_unblock finally having a caller, promise<T>::exception(), the
state word widening, and the shared_state<void> merge.
Not touched here, and worth deciding separately: CLAUDE.md still opens with "NO
RTTI, NO EXCEPTIONS -- code MUST compile with -fno-rtti -fno-exceptions". The
first half holds, but there is now a supported exceptions-enabled mode with a CI
job and a test behind it, and that sentence does not say so.
18.1% of lines carried comment text; 5.8% do now, across 90 files. Code lines went up by 1882 over the same period, so the ratio fell on its own merits. The rule applied: keep what explains a non-obvious decision, an ordering constraint, an invariant, or a trap. Delete what restates the code, narrates the work rather than the program, or names something that no longer exists. The last category was larger than expected. Comments referenced needs_scheduling(), set_result, set_ready, has_active_future, future_state, shutdown_guard_t, begin_shutdown, try_schedule_after_enqueue, wait_for_resume_to_complete and five documents under docs/ -- none of which exist. Several were not merely stale but wrong: "refcount starts at 2" on a shared_state that has no refcount; "the assert in ~cooperative_actor catches pending futures" where no such assert exists; "dtor_ is never incremented" where rtt.hpp increments it; "poll_pending() is called inside behavior() automatically" in a test that never calls it; "local variables may be corrupted after co_await", which is not how a coroutine frame works. The load-bearing comments survive, compressed: the SETTLED-OUTCOME invariants, why `awaiting` never pairs with keep_scheduled, why the drain block sits above park(), the coroutine_handle() ownership contract, why await_transform being a member is what makes every suspension publish its awaited state, the pre-suspension ordering in dispatch(), and every memory-order argument. Verified that only comments changed: a comment stripper that respects string, char and raw-string literals produces identical token streams against HEAD for all 90 files. (The first attempt at this check used `c++ -fpreprocessed -dD -E`, which yields zero bytes on these headers and therefore compared nothing -- worth saying, since it was reported as a verification three times before the mistake surfaced.) 153/153 in the normal build, with EXCEPTIONS_DISABLE=OFF, under ASan and under UBSan.
Merging shared_state<void> into the primary template left
result_storage<void>::take() an empty body -- correctly, since there is no value
to move. But the refusal for a valueless extraction lived in
result_storage::take(), so for T = void it disappeared with it. Under NDEBUG the
assert goes too, and then:
promise<void> p(res);
auto f = p.get_future();
p.error(std::make_error_code(std::errc::operation_canceled));
std::move(f).take_ready(); // returned normally
A failed void operation reported success. unique_future<int> aborted on the same
state; unique_future<void> did not.
The check moves up to take_ready(), where the error is visible, so it no longer
depends on the storage having something to guard. Both overloads share it, and
the rule now reads the same for every T.
RED first: the new void case in test/cancelled-extraction -- which builds with
-DNDEBUG forced on, because with asserts live the gap is invisible -- fails
before this change and passes after.
Found by an agent auditing PROMISE_FUTURE_GUIDE.md against the headers: the
guide said "take_ready() only asserts", the code had moved past that for T and
not for void.
This commit also carries the comment trimming for future.hpp from the pass
before it; the two could not be separated cleanly in one file.
The completion signature was void(int) -- there was nowhere for a failure to go
-- and the poller did:
if (fut->is_ready()) { int v = std::move(*fut).take_ready(); ... }
is_ready() reports promise_released, which a promise dying without a value also
sets. So the example extracted on readiness rather than on having a value, which
is exactly the mistake future.hpp warns about, and which now aborts the process
rather than reading unset storage. It does not fire today only because the
sample doubler always reaches co_return -- but the example teaches the shape.
void(std::error_code, int) instead, Asio's own convention, with the poller
gating on failed(). The consumer takes the result through as_tuple rather than
use_awaitable: the latter throws on error, and this example builds with
-fno-exceptions where asio::detail::throw_exception aborts.
A second consumer awaits a future that completes without a value -- what a
cancelled send leaves behind -- so the failure path is demonstrated rather than
described, and main() now checks that it was taken.
1238 lines to 853. More importantly, seven places taught code that does not
work:
- README showed `co_await std::move(future)` at file scope. unique_future has
no operator co_await; awaiting works only inside an actor coroutine, through
await_transform.
- The promise/future guide said take_ready() "only asserts" and that the
assert is gone under NDEBUG. It refuses in every build.
- The same guide said failures are observed only through failed()/error().
With EXCEPTIONS_DISABLE=OFF the exception reaches the caller.
- The CHANGELOG's own generator-removal guide showed an "after" example that
never discharged needs_sched -- the exact hang its neighbouring guide warns
about.
- CLAUDE.md's single-threaded pump broke out of its loop on
`!info.messages_processed`, which is what a behavior legitimately suspended
on a co_await reports.
- CLAUDE.md's Unsafe Pattern 2 blamed take_ready() for a use-after-free that
is actually the worker's next resume() on a freed actor. The rule was right,
the reason was not.
- CLAUDE.md recommended intrusive_ptr<Actor>; nothing instantiates it outside
its own header, and spawn() returns unique_ptr<Actor, pmr::deleter_t>.
Stale besides: "header-only" without mentioning source/src.cpp, README badges
pointing at a branch that does not exist, compiler versions that predate the CI
matrix, a `futures.reserve()` marked "Required!" that is not, an ownership table
describing the removed future_state model, a CLion toolchain path that does not
exist, and a claim that `(void)` casts are banned project-wide by something
other than convention.
Earlier in this session CLAUDE.md's Safe Pattern 3 was corrected separately: it
relied on ~scheduler_t calling stop(), which it does not -- there is no
destructor, so destroying a started scheduler runs ~std::thread on joinable
threads. Verified by running it: exit 134. The corrected pattern ran 100 rounds
under ASan clean.
…l without a rescue The re-scheduling workaround was a family of five, not one: two timer-based rescues that re-enqueued an actor whose future was not ready within ~1 ms, two drive loops that pinned both ends of a chain every turn, and a pump whose else-branch enqueued on a blind 200 us timer. All five hid the same thing. A dropped needs_sched is not a delay, it is death. The sender that unblocks a parked mailbox claims the `scheduled` bit and owes the enqueue; drop it and the bit stays set with no job in any queue, so every later send() reports needs_sched == false. cooperative_actor.hpp says so at park(): "needs_sched false forever". Nothing asserts, nothing logs. Each removal is red before it is green: the chaining test times out at the first iteration without its rescue, and the pump mutation that the blind enqueue used to mask now fails in 2.6 s. address_t is send-only by design, so an actor holding one records what send() reported and its owner claims it -- the shape lost_wakeup_test already used. Recorded in CLAUDE.md as "Who May Schedule", because nothing stated it and the first attempt here gave three actors a scheduler they must not have. Mirror defects, where the obligation was dropped with no rescue at all: six sends in test/shutdown never ran and four cases asserted REQUIRE(true); the scheduled ping-pong benchmark left its partner stranded and measured half a round trip plus a fixed 100 us sleep. Also: a test for the self-await deadlock, which mixed_example.cpp has pointed at since bd087d3 without it existing; the dead two-argument address_t constructor, which had no callers and left enqueue_fn_ null; WARNINGS_AS_ERRORS, off by default and on in CI, where -Wall -Wextra had been advisory for GCC and Clang while MSVC already had /WX; and an exceptions axis across the gcc, clang and macos matrices, so EXCEPTIONS_DISABLE=OFF is no longer one job on one compiler. Comments corrected rather than trimmed where they had gone false: "take_ready() merely asserts, which Release builds drop" in eight places, untrue since the refusal became unconditional; a dangling "Q6" label; and two of my own, stale the moment the dispatcher actors went back to address_t. 154/154 in five configurations. Each affected binary run 100 times per configuration -- 4000 runs, no failures -- plus the stress test at 8 threads, 2000 iterations and sharing_scheduler(1, 1), where every message forces a park.
Both failures came from the axis itself: EXCEPTIONS_DISABLE=OFF had been one sanitizer job on one compiler, so neither diagnostic had anywhere to surface. clang, every version, and a real portability bug: examples/external-drive captured `future` from `auto [needs_sched, future] = send(...)` into the poller lambda. Capturing a structured binding is ill-formed and clang says so. Bound to a named reference instead. gcc, exceptions only, and a false positive: -Wmismatched-new-delete fires on every coroutine in the tree. A frame is allocated by promise_type::operator new(size, args...) and freed by operator delete(void*, size_t), because [dcl.fct.def.coroutine] has no "placement delete with the same arguments" form -- that pairing is what the standard prescribes. GCC applies the ordinary matching rules to the frame. It only fires with exceptions enabled, where the unwind path makes the deallocation visible to the analysis. Suppressed for GCC, with the reason in CMakeLists.
…p a volatile accumulator Two failures that had nothing to do with the exceptions axis, and one that CI had been reporting all along on a job nobody read. spawn.hpp uses std::unique_ptr and included only <memory_resource>. libc++ provides unique_ptr transitively from there, libstdc++ does not, so the header self-sufficiency test -- added by this branch precisely to catch this -- failed on every GCC build while passing on macOS. Three more headers had the same latent gap: work_sharing.hpp, cooperative_actor.hpp, dispatch_traits.hpp. test_available_race_chaining's busy-work loop compound-assigned to a volatile int, deprecated in C++20 and an error under -Werror on clang. The value is co_returned, so a plain int keeps the loop from being elided. Also of note: the twelve GCC jobs were not twelve failures. Three failed on -Wmismatched-new-delete; fail-fast cancelled the other nine, which have no logs.
Same defect as the previous commit, in test_cross_thread: a compound assignment to a volatile int, deprecated in C++20 and fatal under -Werror on clang 14-18 and on the macOS runners. I had fixed only the site CI named instead of sweeping the pattern; there were two, and both are gone now. benchmark/message keeps its volatile: plain assignment is not deprecated, only compound assignment and ++/--. Worth recording: AppleClang 21 does not emit -Wdeprecated-volatile at all, even when the flag is passed explicitly -- the diagnostic is gone from newer clang. The local toolchain is ahead of every compiler in the matrix, so "clean locally" no longer implies "clean in CI" now that warnings are errors.
Linux ASan reported a heap-use-after-free in tests_shutdown_aggressive: a worker called resume() on an actor freed by another thread. The report is right and the test was wrong. Its header claimed ~cooperative_actor protects this. It protects part of it -- publish `destroying`, then wait out the thread holding `running` and any sender past enqueue_impl's gate -- but not a job already in the scheduler's queue. The actor holds no scheduler and cannot revoke it, so a worker picking that job up after the actor is gone reads freed memory. CLAUDE.md lists exactly that as unsafe; the file asserted the opposite, and one case said so out loud with REQUIRE(true) and "the pass is reaching here without a sanitizer report". Both cases now drain their own work before the actor dies. The stress they exist for -- 48 actors spawned and torn down from four threads under an eight-worker scheduler -- is unchanged; what is gone is the UB window, which is not a property to assert. REQUIRE(true) became the message count, and a stalled drain now fails instead of hanging. This had been masked: the sanitizer job never reached the test stage, because spawn.hpp failed to compile there. Fixing the include exposed it.
Second site of the same defect: test/external-drive captured `fut` from a structured binding into its poller thread. Capturing a structured binding is ill-formed; clang rejects it, gcc lets it through. I had fixed only the examples copy, because that was the one CI named -- fail-fast had cancelled the job that would have named this one. A scoped scan of the tree finds exactly these two, and both are now bound to a named reference.
kotbegemot
force-pushed
the
async-rewrite
branch
6 times, most recently
from
September 23, 2026 23:24
740888d to
292a787
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes a lost wakeup in
cooperative_actor::resume_impl, and removes thegenerator<T>streaming subsystem that made the underlying scheduler stateambiguous in the first place.
The lost wakeup
A behavior suspended on
co_awaitpublishes its awaited chain, soresume_implsees
is_busy(). When the awaited future goes ready the Q6 block drains thecontinuation, the coroutine advances, and if it issues another request it
re-suspends immediately with a fresh chain. Two halves of one bug:
is_busy()aftercont.resume(), so controlfell through to the empty-mailbox branch,
try_block()succeeded, and an actorsuspended on a pending await was parked — verdict
awaiting,keep_scheduled = false, so the worker dropped the job;drain its ready continuation: every later resume died before reaching Q6.
Future completion is flag-only —
release_promisesetspromise_released, itneither pushes to the mailbox nor enqueues the actor — so a bare
scheduler->enqueue()was exactly the wake-up that could not help.Fix: hoist Q6 above the blocked-check, and re-check
is_busy()unconditionallyafter
cont.resume().Breaking changes
generator<T>is removed. It had no correct end-to-end consumption route:no synchronous
next(), so the only way to advance a stream wasco_await, andthe advertised way to do that (
while (co_await gen)inside an actor method)went through the one
await_transformoverload that never calledpropagate_awaited_state. That left the consuming behavior alive-but-not-busy —a state the scheduler cannot tell from idle — so it either parked the actor on a
pending await or move-assigned over the live coroutine frame on the next inbound
message (use-after-free, plus leaked
dispatch/method frames and theirshared_states). The producer also resumed the consumer's coroutine on its ownthread, which
future_awaiters.hppforbids by design.Four further defects shipped with it:
yield_awaiterclaimed ownership of onecoroutine frame from two
generator_states (doubledestroy());take_producer_handle()returned a handle without clearing it; andco_yield stream_error{...}ended the consumer loop before its body ran,making in-loop
has_error()checks dead code.Nothing depended on it — no test in the tree ever asserted a yielded value.
Migration guidance is in CHANGELOG.md, along with the constraints any future
streaming design has to satisfy.
resume()andjob_ptr::resume()are[[nodiscard]]. An actor suspended onco_awaitno longer parks, so it no longer blocks its own inbox, sosend()nolonger reports
needs_schedfor it. The verdict is now the only signal that thecaller must re-queue the actor, and dropping it strands the actor. That contract
was always true —
work_sharinghonours it — the park merely masked violations.<experimental/coroutine>is refused with an#error.generator.hppusedstd::coroutine_handledirectly and reached every actor throughmake_message.hpp, so that configuration used to fail loudly at compile time.Without it only
detail::noop_coroutine()remains, which returns a NULL handlethere — undefined behaviour in a symmetric-transfer position. A loud break must
not become a silent one.