diff --git a/CHANGELOG.md b/CHANGELOG.md index 4d132956..faf2d12e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,31 @@ API surface). ### Changed +- **`Completion` has a stated value-handling contract, and `T` no longer has + to be copyable.** `std::move_constructible` is now the whole type + requirement; copyability became a *per-handler* obligation, diagnosed where + the handler is written. A handler taking `const T&` costs **zero** copies, one + taking `T` by value costs **exactly one**, and neither number depends on how + many handlers are attached or on which side of the settle they attached. + Measured with a copy-counting `T`, the budget was **N + 2M** (N handlers + attached before settling, M after) and is now **one per by-value handler**: + three `const T&` handlers before a settle plus three after went from 9 copies + to 0. Mechanically, `onOk` is erased as + `std::vector>` and `CompletionState` derives + from `std::enable_shared_from_this`, so both dispatch closures read the stored + value in place instead of carrying a copy. `Completion>` + now instantiates and fans out; `IExecutor::post` is unchanged, because the + closure captures a `shared_ptr` rather than the value. + + **Public signature change**: `then()`, `thenDetached()` and their gated + overloads take `std::function` rather than + `std::function`. Source-compatible for lambdas taking `const T&`, + `T` by value or `auto` by value, and for existing `std::function` + objects — all 312 `.then(` call sites in the tree compile unchanged. A handler + taking `T&` or `T&&` would not; none exists. Pre-1.0, per + `docs/spec/VERSIONING.md`. See `docs/spec/core/completion.md`, + "Value-handling contract", and morph#553. + - **`morph::log` no longer takes over a consumer's `stderr` by default.** `LogState::minLevel` defaults to `LogLevel::warn` instead of `LogLevel::debug`, so linking morph no longer emits its dispatch tracing @@ -187,6 +212,15 @@ API surface). `equation()` should set it to `0` rather than have it flipped underneath every build that did not. See `docs/spec/util/quantity_type.md`, *Provenance* and *Limitations*; morph#574. +- **The `RemoteServer` throughput benchmark wrote into a destroyed stack + frame.** `tests/test_server_limits.cpp`'s reply counter was a stack local of + the `BENCHMARK` body captured by reference into callbacks that run on pool + workers; the body's 1 s deadline let it return with replies still in flight, + and Catch2's next sample then constructed a fresh counter over the same stack + slot. ThreadSanitizer reported two data races, both frames in this test — a + false signal that reads as a race inside `RemoteServer`. The counter is now a + `shared_ptr>` the callbacks co-own; the deadline stays as a + benchmark timeout rather than a correctness device. Test-only. See morph#565. - **Both of the cross-field rule vocabulary's safety checks were bypassed by wrapping a rule in one combinator.** Unsatisfiability detection stopped at the first compound node, because it skipped any node without a `fields` key diff --git a/docs/spec/core/completion.md b/docs/spec/core/completion.md index 856c184d..29271ec4 100644 --- a/docs/spec/core/completion.md +++ b/docs/spec/core/completion.md @@ -13,6 +13,7 @@ than vanishing (see [Failure modes](#failure-modes)). ## Contents - [Shared state — `CompletionState`](#shared-state--completionstatet) +- [Value-handling contract](#value-handling-contract) - [Orphan detection](#orphan-detection) - [Move-only handle — `Completion`](#move-only-handle--completiont) - [Settleable promise seam — `Completion::Promise`](#settleable-promise-seam--completiontpromise) @@ -32,13 +33,28 @@ than vanishing (see [Failure modes](#failure-modes)). producer and the consumer reference through `std::shared_ptr`. All mutation is guarded by `std::mutex mtx`. +It derives from `std::enable_shared_from_this>`. That is +load-bearing: both dispatch closures capture `shared_from_this()` and read the +settled value *in place* rather than carrying a copy of it, which is what makes +the copy budget independent of handler count and what lets `T` be move-only — +the closure stores a refcounted handle, so it stays copy-constructible and +`IExecutor::post`'s `std::function` needs no change. The precondition is +that every `CompletionState` is created by `std::make_shared`, which every +site in the tree does (`Completion::makeSettleable`, `Bridge`, the backends). + +Reading `value` from a dispatch closure without holding `mtx` is safe because +`value` is **write-once**: `setValue` and `setException` both return early when +`ready`, and nothing else assigns it. The store happens under the lock before +the closure is handed to the executor, and the executor's queue supplies the +happens-before edge to the thread that runs it. + | Member | Type | Purpose | |---|---|---| | `mtx` | `std::mutex` | Guards all state and callback registration | | `value` | `std::optional` | The success value, set once | | `error` | `std::exception_ptr` | The error, set once via `setException`; never null once `ready` is `true` | | `ready` | `bool` | `true` once either `value` or `error` is set | -| `onOk` | `std::vector>` | Stored success callbacks, in attachment order; moved out on dispatch | +| `onOk` | `std::vector>` | Stored success callbacks, in attachment order; moved out on dispatch. Erased as `void(const T&)` so a handler pays for its own copy only if it asks for one — see [Value-handling contract](#value-handling-contract) | | `onErr` | `std::vector>` | Stored error callbacks, in attachment order; moved out on dispatch | | `onErrAttached` | `bool` | Suppresses orphan logging when `true`; set to `(cbExec != nullptr)` — never set on a null-executor state | | `cbExec` | `::morph::exec::IExecutor*` | Executor for callback dispatch; may be `nullptr` | @@ -98,27 +114,26 @@ attached, from one posted closure. If the state is already ready with the no handler is stored — the attach is a silent no-op, for that call only (it does not affect any other handler already stored). -**Copy vs. move of the value on dispatch.** The two dispatch paths handle the -stored value differently, and the difference is observable: +**Copy vs. move of the value on dispatch.** Both dispatch paths read the +stored value in place, and neither copies nor moves it: - *Set-after-attach* (`setValue` finds one or more already-registered `onOk` - handlers): a local `savedVal` is **copied from `setValue`'s own parameter** - (`auto savedVal = val;`), *before* `value` is ever touched, and that local is - what the fan-out closure consumes — every handler but the last gets a copy of - it, only the final handler in attachment order receives it **moved** - (`std::move(savedVal)`). Either way, `value` itself — the state's own store — - is untouched by this: it is set separately, immediately after, by a plain - `value = std::move(val)`, so it always holds a genuine, intact `T` once - `ready` is true, never a moved-from husk (morph#520; see - [Failure modes](#failure-modes)). + handlers): `value = std::move(val)` stores the value, `ready` is set, and the + drained handler vector goes into a closure that also captures + `shared_from_this()`. Each handler is invoked as `fn(*self->value)` — the + state's own store, read, never taken. - *Attach-after-ready* (`attachThen` fires now against a settled value): the - value is **copied** (`savedVal = *value`), leaving `value` intact. - -The fire-now copy is what makes a repeated `then()` on an already-settled value -state fire again with the same result — including one attached *after* the -set-after-attach path above already ran, since `value` was never disturbed by -it either. Errors have no such asymmetry — an `exception_ptr` is cheap to copy -and is copied for every handler on both paths, so `error` is never emptied. + same shape with one handler — the closure captures `shared_from_this()` and + invokes `handler(*self->value)`. + +**The value is observed, never consumed.** No dispatch path can move out of +`value`, so a `then()` attached after a set-after-attach dispatch already ran +still fires against the genuine result rather than a moved-from husk (morph#520; +see [Failure modes](#failure-modes)), and no handler in a fan-out can leave a +husk for its siblings. A handler that wants to consume takes `T` **by value** and +moves out of its own copy. Errors behave the same way for a different reason — +an `exception_ptr` is a refcounted handle, cheap to copy, and is copied for every +handler on both paths, so `error` is never emptied. `attachOnError` sets `onErrAttached = (cbExec != nullptr)` unconditionally on entry, before inspecting the state. So attaching an error handler on a @@ -126,6 +141,60 @@ null-executor state does **not** suppress orphan logging: the handler will never be posted, so the error is preserved for the destructor's orphan logger instead of being both dropped and silenced. +## Value-handling contract + +**At the type level, `T` need only be move-constructible.** +`std::move_constructible` is the whole requirement, enforced by a +`static_assert` on `CompletionState` so an unusable `T` produces one line +rather than pages from inside `std::function`. `Completion>` +instantiates and fans out. + +**Copyability is a per-handler obligation, not a per-type one.** Handlers are +erased as `std::function`. One erased type accepts every +spelling a caller already writes, because each is invocable with `const T&`: + +| handler | copies when invoked | +|---|---| +| `[](const T& v)` | 0 | +| `[](T v)` | 1 | +| `[](auto v)` (by-value generic) | 1 | +| an existing `std::function` object | 1 | + +The copy, when a handler wants one, happens at that handler's own parameter +binding — where the reader of the call site can see it, and where the compiler +diagnoses a `T` that cannot be copied. No trait detection, no `if constexpr`, no +signature introspection. + +**Copy budget: exactly one copy per handler that takes `T` by value, and zero +for every handler that takes `const T&`.** Independent of how many handlers are +attached, and of whether they attached before or after the completion settled. +Exactly, not "at most": `tests/test_completion_value_contract.cpp` pins the +count with a copy-counting `T`, because an upper bound quietly absorbs a +regression. + +Settling itself moves `T` exactly twice for a prvalue argument — into +`resolve`'s by-value parameter, then into `setValue`'s, then into `value`, with +the first elided — and dispatch adds none. + +This replaces a budget of **N + 2M** copies per settle (N handlers attached +before settling, M after) in which `T` was additionally forced to be +copy-constructible: `onOk` was erased as `std::function`, so every +handler was charged a copy whether or not it wanted one, and `attachThen`'s +fire-now path copied `*value` into a local and then captured that local *by +copy* before moving it into the handler — two copies where the handler asked +for at most one. See morph#553. + +**What this costs.** A cheap `T` pays a little more per settle: the dispatch +closure holds a `shared_ptr` to the state and so pays an atomic refcount pair +where it used to copy a small value. Against the JSON encode/decode — and often +a socket write — that surrounds a settle, that is noise, and it buys a large `T` +its per-handler copies back. It is recorded here rather than hidden, because it +is a real regression on the cheap case. + +**The large-`T` win requires `const T&` handlers.** The handler signature is the +lever a caller pulls, which is why it is documented as contract rather than left +as an implementation detail. + ## Orphan detection If a `CompletionState` is destroyed while `ready == true`, `error` is set, and @@ -286,15 +355,15 @@ throw — they are silent by construction. stored handler has fired (the vector was moved out on dispatch), a further `then()`/`onError()` call is governed by the rules above against the now-`ready` state — i.e. a matching-outcome attach fires immediately with the - settled result, a mismatched one is a no-op. A late `then()` fires with a - *copy* of the value (the fire-now path copies; see - [Shared state](#shared-state--completionstatet)), so the value is not - consumed by any fire-now dispatch. This holds regardless of which dispatch - path originally delivered the value: `value` is never moved out of `setValue` - (morph#520 — see [Shared state](#shared-state--completionstatet)'s "Copy vs. - move" bullet), so a `then()` attached after a set-after-attach dispatch fires - against the same genuine result the earlier handlers saw, not a moved-from - husk. + settled result, a mismatched one is a no-op. A late `then()` fires against the + *stored* value, read in place by a closure holding `shared_from_this()` (see + [Shared state](#shared-state--completionstatet)), so no fire-now dispatch + consumes it and a handler taking `const T&` pays nothing for it. This holds + regardless of which dispatch path originally delivered the value: `value` is + never moved out of (morph#520, morph#553 — see + [Value-handling contract](#value-handling-contract)), so a `then()` attached + after a set-after-attach dispatch fires against the same genuine result the + earlier handlers saw, not a moved-from husk. ## Client-side execute deadline @@ -443,10 +512,10 @@ that will never signal. | move assign | `Completion& operator=(Completion&&) noexcept = default` | Transfers state ownership. | | copy ctor | `Completion(Completion const&) = delete` | Move-only handle. | | copy assign | `Completion& operator=(Completion const&) = delete` | Move-only handle. | -| `then(handler)` | `Completion& then(std::function)` | Registers success callback; returns `*this` for chaining. | -| `then(scope, handler)` | `Completion& then(CallbackScope const&, std::function)` | As above, gated on the scope's liveness and stop state (see [Lifetime and stop gating](#lifetime-and-stop-gating)). | -| `then(token, handler)` | `Completion& then(CallbackToken, std::function)` | Token-taking form of the above; a default-constructed token suppresses unconditionally. | -| `thenDetached(handler)` | `Completion& thenDetached(std::function)` | Exactly `then(handler)`, spelled so a deliberately ungated callback is greppable. | +| `then(handler)` | `Completion& then(std::function)` | Registers success callback; returns `*this` for chaining. | +| `then(scope, handler)` | `Completion& then(CallbackScope const&, std::function)` | As above, gated on the scope's liveness and stop state (see [Lifetime and stop gating](#lifetime-and-stop-gating)). | +| `then(token, handler)` | `Completion& then(CallbackToken, std::function)` | Token-taking form of the above; a default-constructed token suppresses unconditionally. | +| `thenDetached(handler)` | `Completion& thenDetached(std::function)` | Exactly `then(handler)`, spelled so a deliberately ungated callback is greppable. | | `onError(handler)` | `Completion& onError(std::function)` | Registers error callback; returns `*this` for chaining. | | `onError(scope, handler)` | `Completion& onError(CallbackScope const&, std::function)` | As above, gated on the scope. Still suppresses orphan logging: a scope-refused error counts as handled. | | `onError(token, handler)` | `Completion& onError(CallbackToken, std::function)` | Token-taking form of the above. | @@ -471,7 +540,7 @@ that will never signal. |---|---|---| | `setValue(T)` | `void setValue(T)` | Producer-side; no-op if already ready. Posts one closure invoking every registered success handler, in attachment order, if any were registered. | | `setException(exception_ptr)` | `void setException(std::exception_ptr const&)` | Producer-side; no-op if already ready. Posts one closure invoking every registered error handler, in attachment order, if any were registered. | -| `attachThen(function)` | `void attachThen(std::function)` | Consumer-side; fires immediately (this handler only) if ready with value, else appends to the stored handler list. | +| `attachThen(function)` | `void attachThen(std::function)` | Consumer-side; fires immediately (this handler only) if ready with value, else appends to the stored handler list. | | `attachOnError(function)` | `void attachOnError(std::function)` | Consumer-side; fires immediately (this handler only) if ready with error, appends to the stored handler list if not yet ready, no-op if ready with a value. Sets `onErrAttached = (cbExec != nullptr)`, so orphan logging is suppressed only when an executor exists to deliver on. | | destructor | `~CompletionState()` | Orphan-detection: logs unhandled exceptions when destroyed with an error and no `onErr` attached. | @@ -486,7 +555,7 @@ that will never signal. | First-result-wins | **`setValue`/`setException` are no-ops after `ready`** | An asynchronous operation should complete exactly once; subsequent calls are silently ignored. | | Move-only handle | **`Completion` is move-only, `CompletionState` is shared via `shared_ptr`** | The handle is owned by one consumer at a time; the shared state is owned jointly by the producer and any consumer that has moved the handle. | | Empty completion | **Null state pointer makes `then`/`onError` no-ops** | Default-constructed `Completion` is a safe placeholder that never signals. | -| Value copy on fire-now | **`attachThen` copies `*value`; `setValue` moves it only into the last handler's invocation** | The set-after-attach path copies the value into every handler but the last (moving only into the final call), so no earlier handler observes a moved-from value and the value is still consumed exactly once overall. The attach-after-ready path must copy so `value` stays intact and a repeated `then()` on a settled state can still fire with the result. | +| Value handling on dispatch | **Both paths read `*value` in place; neither copies nor moves it** | Handlers are erased as `std::function` and the dispatch closures capture `shared_from_this()`, so the copy budget is exactly one per by-value handler and zero per `const T&` handler, whenever it attached. `value` is never consumed, so a `then()` attached after settling still sees the genuine result, and `T` need only be move-constructible. See [Value-handling contract](#value-handling-contract). | | Handler fan-out | **`onOk`/`onErr` are `std::vector`s, appended to on each attach** | Fixes issue #59: a second `onError()` (or `then()`) on the same still-pending `Completion` used to silently replace the first handler in a single-slot field. Composing (invoking every attached handler, in order) matches the mental model of an observer list and is what most call sites composing behavior via repeated attach actually expect. | | Per-handler exception isolation | **Each composed handler invocation is wrapped in its own `try`/`catch (...)`, logged via `logError` and swallowed** | Fan-out means every attached handler should get its turn regardless of what an earlier one does. Without per-handler isolation, one throwing handler would unwind the whole posted closure and silently skip every handler attached after it — turning a single misbehaving consumer into an outage for unrelated ones sharing the same `Completion`. | | Public settleable-promise seam | **`Completion::Promise`, reachable only via `makeSettleable()`** | Fixes issue #55: test code needing a `Completion` it can resolve/reject on demand had no seam except reaching into `morph::async::detail::CompletionState` directly. `Promise`'s constructor is private and `friend`ed only to `Completion`, so `detail::CompletionState` never has to appear in a caller's own code. | diff --git a/include/morph/core/completion.hpp b/include/morph/core/completion.hpp index aed7b87e..b78b8724 100644 --- a/include/morph/core/completion.hpp +++ b/include/morph/core/completion.hpp @@ -1,6 +1,7 @@ // SPDX-License-Identifier: Apache-2.0 #pragma once +#include #include #include #include @@ -21,8 +22,29 @@ namespace morph::async { namespace detail { // NOLINTBEGIN(cppcoreguidelines-special-member-functions) +// +// `enable_shared_from_this` is load-bearing, not decoration: the dispatch +// closures below capture a `shared_ptr` to this state and read the settled +// value *in place* instead of carrying a copy of it. That is what makes the +// copy budget independent of how many handlers are attached, and what lets `T` +// be move-only -- the closure stores a refcounted handle, so it stays +// copy-constructible and `IExecutor::post`'s `std::function` is +// untouched. Every `CompletionState` in the tree is created by +// `std::make_shared` (`Completion::makeSettleable`, `Bridge`, the backends), +// which is the precondition `shared_from_this()` needs. +// +// Reading `value` outside `mtx` from those closures is safe because `value` is +// write-once: both `setValue` and `setException` return early when `ready`, and +// nothing else ever assigns it. The store happens under the lock before the +// closure is handed to the executor, and the executor's own queue provides the +// happens-before edge to the thread that runs it. template -struct CompletionState { +struct CompletionState : std::enable_shared_from_this> { + static_assert(std::move_constructible, + "morph::async::Completion requires only that T be move-constructible. Copyability is a " + "per-handler obligation: a handler taking `const T&` imposes nothing, a handler taking `T` by " + "value requires T to be copyable and is diagnosed where that handler is written."); + std::mutex mtx; std::optional value; std::exception_ptr error; @@ -32,7 +54,15 @@ struct CompletionState { // silently discarding them. Dispatch invokes all of them, in attachment // order, from a single posted closure. See docs/spec/core/completion.md, // "Failure modes" / fan-out. - std::vector> onOk; + // + // Erased as `void(const T&)`, not `void(T)`. One erased type accepts every + // handler spelling a caller already writes -- `[](const T&)`, `[](T)`, a + // `std::function` object -- because each is invocable with + // `const T&`. A handler that wants its own value gets exactly one copy, at + // its own parameter binding, where the reader of that call site can see it; + // a handler that only observes pays nothing. Erasing as `void(T)` charged + // every handler a copy whether or not it wanted one (morph#553). + std::vector> onOk; std::vector> onErr; bool onErrAttached = false; ::morph::exec::IExecutor* cbExec = nullptr; @@ -44,62 +74,56 @@ struct CompletionState { if (ready) { return; } + // Store first, drain `onOk` last. `value` is this state's own + // store and is never moved out of: a `then()` attached *after* + // this point (attachThen's `ready && value` branch) reads it + // again, and moving out of it left it engaged but moved-from so a + // later attacher silently observed a husk (morph#520). The value + // is observed, never consumed -- structurally, now that no + // dispatch path can take it. + // + // The ordering is what gives this block the strong exception + // guarantee for a `T` whose *move* constructor can throw: nothing + // has changed when the store below runs, and `optional::emplace` + // leaves the optional disengaged if the construction throws, so an + // escape leaves `onOk` holding every handler and the state unready + // rather than a state that already looks settled with its handlers + // already lost. Draining first (as an earlier revision did) meant + // a throwing store unwound with `savedFns` -- a local -- carrying + // every handler to its destructor: permanently unsettled, no + // handlers, and silent, because the destructor's orphan logger + // only fires when `error` is set. `std::move` on a vector is + // noexcept, so the ordering costs nothing. + // + // `emplace`, not `value = std::move(val)`: assigning through + // `std::optional` requires `T` to be move-*assignable* as well as + // move-constructible, which would quietly make the `static_assert` + // above a lie for a `T` with a deleted assignment operator. + // `value` is guaranteed disengaged here -- it is written only on + // this line, and the `ready` guard above makes this line run once. + value.emplace(std::move(val)); + ready = true; if (!onOk.empty()) { - // Copy from `val` -- and before anything below mutates state -- - // rather than moving out of `value` after the fact: `value` is - // this state's own store, and a `then()` attached *after* this - // point (attachThen's `ready && value` branch) reads it again, - // so moving out of it left it engaged but moved-from, and a - // later attacher silently copied a husk (morph#520). Copying - // first, before `onOk` is drained or `value`/`ready` are set, - // gives this block the strong exception guarantee against a - // throwing copy constructor specifically: if it throws, - // nothing here has changed yet -- `onOk` still holds every - // handler and the state is still unready -- rather than a - // corrupted state that already looks settled with its - // handlers already lost. - // - // `onOk` is drained *last*, after `value`/`ready` are set, so - // the same guarantee covers a `T` whose *move* constructor can - // throw. Draining first (as an earlier revision did) meant a - // throwing `value = std::move(val)` unwound with `savedFns` -- - // a local -- carrying every handler to its destructor while - // `onOk` was already empty: permanently unsettled, no handlers, - // and silent, because the destructor's orphan logger only fires - // when `error` is set. `std::move` on a vector is noexcept and - // `savedVal` is already an independent copy, so the reordering - // costs nothing. - auto savedVal = val; - value = std::move(val); - ready = true; auto savedFns = std::move(onOk); - callback = [savedFns = std::move(savedFns), savedVal = std::move(savedVal)]() mutable { - // Every handler but the last sees a copy (the value is only - // moved into the final invocation), so an earlier handler - // cannot leave the value moved-from for a later one. Each - // handler is isolated in its own try/catch so one throwing - // handler cannot prevent its siblings from running -- - // fan-out means every attached handler gets its turn, - // independent of whether an earlier one misbehaves. An - // escaping exception here would otherwise unwind the whole - // posted closure and silently skip every handler after the - // one that threw. - for (std::size_t i = 0; i + 1 < savedFns.size(); ++i) { + callback = [self = this->shared_from_this(), savedFns = std::move(savedFns)]() { + // Every handler reads the one stored value in place; none + // can move out of it, so no handler can leave a husk for + // its siblings or for a later attacher, and the count of + // handlers costs nothing in copies. Each handler is + // isolated in its own try/catch so one throwing handler + // cannot prevent its siblings from running -- fan-out means + // every attached handler gets its turn, independent of + // whether an earlier one misbehaves. An escaping exception + // here would otherwise unwind the whole posted closure and + // silently skip every handler after the one that threw. + for (const auto& fn : savedFns) { try { - savedFns[i](savedVal); + fn(*self->value); } catch (...) { ::morph::log::logError("[completion] then handler threw; continuing with next handler"); } } - try { - savedFns.back()(std::move(savedVal)); - } catch (...) { - ::morph::log::logError("[completion] then handler threw; continuing with next handler"); - } }; - } else { - value = std::move(val); - ready = true; } } if (callback != nullptr && cbExec != nullptr) { @@ -162,13 +186,18 @@ struct CompletionState { cbExec->post(std::move(callback)); } } - void attachThen(std::function handler) { + void attachThen(std::function handler) { std::function fireNow; { std::scoped_lock const lock{mtx}; if (ready && value) { - auto savedVal = *value; - fireNow = [handler = std::move(handler), savedVal]() mutable { handler(std::move(savedVal)); }; + // Keep the state alive and read `value` in place rather than + // snapshotting it. The old shape copied `*value` into + // `savedVal` and then captured `savedVal` *by copy* before + // moving it into the handler -- two copies where the handler + // asked for at most one, and the reason a late attacher cost + // 2 copies rather than 1 (morph#553). + fireNow = [self = this->shared_from_this(), handler = std::move(handler)]() { handler(*self->value); }; } else if (!ready) { onOk.push_back(std::move(handler)); } @@ -243,7 +272,22 @@ struct CompletionState { /// `onError(fn)`, spelled so a deliberately unmanaged callback says so. /// See `callback_scope.hpp` and docs/spec/core/callback_scope.md. /// -/// @tparam T Type of the success value. +/// @par Value-handling contract +/// `T` need only be **move-constructible**; that is the whole type requirement. +/// Copyability is a *per-handler* obligation, reported where the handler is +/// written rather than imposed on the whole instantiation: +/// - a handler taking `const T&` costs **zero** copies; +/// - a handler taking `T` by value costs **exactly one**, at its own parameter +/// binding, and requires `T` to be copyable. +/// +/// The budget does not depend on how many handlers are attached, nor on whether +/// they attached before or after the completion settled. The value is +/// **observed, never consumed**: no handler can move out of the stored value, so +/// a `then()` attached after settling still sees the genuine result. A handler +/// that wants to consume takes `T` by value and moves out of its own copy. +/// See docs/spec/core/completion.md, "Value-handling contract". +/// +/// @tparam T Type of the success value. Must be move-constructible. template // NOLINTNEXTLINE(cppcoreguidelines-special-member-functions) class Completion { @@ -277,10 +321,11 @@ class Completion { /// operation completes successfully. If the operation has already completed, /// the callback is posted immediately. /// - /// @param handler Callable receiving the result by value. + /// @param handler Callable receiving the result. Take `const T&` to observe it for free; + /// take `T` by value to get your own copy, at the cost of exactly one copy. /// @return `*this` for chaining — a reference into this `Completion`, valid /// only for as long as it is. - Completion& then(std::function handler) MORPH_LIFETIMEBOUND { + Completion& then(std::function handler) MORPH_LIFETIMEBOUND { if (_state != nullptr) { _state->attachThen(std::move(handler)); } @@ -317,10 +362,11 @@ class Completion { /// @param scope Receiver-owned gate. Only a token for its *current* /// generation is captured, so a later `reset()` retires this /// attachment. - /// @param handler Callable receiving the result by value. + /// @param handler Callable receiving the result. Take `const T&` to observe it for free; + /// take `T` by value to get your own copy, at the cost of exactly one copy. /// @return `*this` for chaining — a reference into this `Completion`, valid /// only for as long as it is. - Completion& then(const CallbackScope& scope, std::function handler) MORPH_LIFETIMEBOUND { + Completion& then(const CallbackScope& scope, std::function handler) MORPH_LIFETIMEBOUND { return then(scope.token(), std::move(handler)); } @@ -332,11 +378,12 @@ class Completion { /// /// @param token Gate observing some receiver's `CallbackScope`. A /// default-constructed token suppresses unconditionally. - /// @param handler Callable receiving the result by value. + /// @param handler Callable receiving the result. Take `const T&` to observe it for free; + /// take `T` by value to get your own copy, at the cost of exactly one copy. /// @return `*this` for chaining — a reference into this `Completion`, valid /// only for as long as it is. - Completion& then(CallbackToken token, std::function handler) MORPH_LIFETIMEBOUND { - return then(std::function{token.guard(std::move(handler))}); + Completion& then(CallbackToken token, std::function handler) MORPH_LIFETIMEBOUND { + return then(std::function{token.guard(std::move(handler))}); } /// @brief Registers an error callback gated on @p scope's lifetime and stop state. @@ -378,10 +425,13 @@ class Completion { /// when the handler genuinely owns everything it touches — it captures only /// values, or a `shared_ptr` it keeps alive itself. /// - /// @param handler Callable receiving the result by value. + /// @param handler Callable receiving the result. Take `const T&` to observe it for free; + /// take `T` by value to get your own copy, at the cost of exactly one copy. /// @return `*this` for chaining — a reference into this `Completion`, valid /// only for as long as it is. - Completion& thenDetached(std::function handler) MORPH_LIFETIMEBOUND { return then(std::move(handler)); } + Completion& thenDetached(std::function handler) MORPH_LIFETIMEBOUND { + return then(std::move(handler)); + } /// @brief Registers an error callback whose lifetime is deliberately unmanaged. /// diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 6b52681d..be68863f 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -28,6 +28,7 @@ add_executable(morph_tests test_completion_extra.cpp test_completion_multi_handler.cpp test_completion_promise.cpp + test_completion_value_contract.cpp test_model.cpp test_logger.cpp test_observability.cpp diff --git a/tests/test_completion_multi_handler.cpp b/tests/test_completion_multi_handler.cpp index 2f04edaf..8aae1e3c 100644 --- a/tests/test_completion_multi_handler.cpp +++ b/tests/test_completion_multi_handler.cpp @@ -7,6 +7,7 @@ // arrives, in attachment order. #include +#include #include #include #include @@ -20,11 +21,12 @@ using LogGuard = morph::log::ScopedLoggerOverride; namespace { -/// A value whose *copy* constructor throws on demand. `setValue`'s first act on -/// a state with handlers attached is `auto savedVal = val;` -- the copy -/// morph#520 introduced -- so this is what exercises the strong exception -/// guarantee documented there. The flag travels with the value rather than -/// living in a global so two tests cannot arm each other. +/// A value whose *copy* constructor throws on demand. Under the value contract +/// (morph#553) nothing on the value path copies `T`, so an armed `ThrowOnCopy` +/// settling through `const T&` handlers is a booby trap that must never go off +/// -- which is what turns "zero copies" from a comment into a test. The flag +/// travels with the value rather than living in a global so two tests cannot +/// arm each other. struct ThrowOnCopy { int payload = 0; bool explode = false; @@ -42,6 +44,36 @@ struct ThrowOnCopy { ~ThrowOnCopy() = default; }; +/// A value whose *move* constructor throws on demand. `setValue`'s only act on +/// `T` is `value = std::move(val)`, so this is what exercises the strong +/// exception guarantee it documents: the store happens before `onOk` is +/// drained, so an escape must leave the state exactly as it was. +/// +/// `setValue(T val)` takes its argument by value and every call below passes a +/// prvalue, so C++17 guaranteed elision constructs it directly in the +/// parameter: the move inside `setValue` is the *first* move this type ever +/// sees, and arming it on construction is enough. +struct ThrowOnMove { + int payload = 0; + bool explode = false; + + ThrowOnMove() = default; + ThrowOnMove(int value, bool boom) : payload{value}, explode{boom} {} + ThrowOnMove(const ThrowOnMove&) = default; + // A throwing, non-`noexcept` move constructor is the whole point of this + // fixture -- `bugprone-exception-escape` and the two noexcept-move checks + // are correct about ordinary code and wrong about a fault injector. + // NOLINTNEXTLINE(bugprone-exception-escape,cppcoreguidelines-noexcept-move-operations,performance-noexcept-move-constructor) + ThrowOnMove(ThrowOnMove&& other) : payload{other.payload}, explode{other.explode} { + if (explode) { + throw std::runtime_error{"move ctor blew up"}; + } + } + ThrowOnMove& operator=(const ThrowOnMove&) = default; + ThrowOnMove& operator=(ThrowOnMove&&) = default; + ~ThrowOnMove() = default; +}; + } // namespace TEST_CASE("Completion: multiple onError handlers all fire, in attachment order", "[completion][issue-59]") { @@ -242,23 +274,55 @@ TEST_CASE("Completion: a then() attached after settlement observes the same valu REQUIRE(secondSeen == original); } -TEST_CASE("Completion: a throwing T copy leaves the state unsettled with every handler intact", - "[completion][issue-520]") { - // The guarantee `setValue` documents: the copy is taken *before* `onOk` is - // drained or `value`/`ready` are set, so a throwing copy constructor must - // leave the state exactly as it was -- unready, with every handler still - // attached -- rather than half-settled with its handlers already lost. +TEST_CASE("Completion: settling never copies T -- an armed throwing copy constructor never runs", + "[completion][issue-553]") { + // The value half of the contract, stated as a trap rather than a comment: + // `onOk` is erased as `std::function` and both dispatch + // paths read the stored value in place, so settling a state with `const T&` + // handlers -- attached before *or* after -- copies `T` exactly zero times. + // An armed `ThrowOnCopy` therefore settles without incident. Before + // morph#553 this threw: `setValue` copied into `savedVal` before draining + // `onOk`, and `attachThen`'s fire-now path copied twice more. SyncExecutor exec; auto state = std::make_shared>(); morph::async::Completion comp{state, &exec}; int fired = 0; - // `const&` parameters: a by-value one would be an extra copy per handler - // that this test never reads, and `std::function` accepts either. - comp.then([&](const ThrowOnCopy&) { ++fired; }); + int seen = 0; + comp.then([&](const ThrowOnCopy& v) { + ++fired; + seen = v.payload; + }); comp.then([&](const ThrowOnCopy&) { ++fired; }); - REQUIRE_THROWS_AS(state->setValue(ThrowOnCopy{7, true}), std::runtime_error); + REQUIRE_NOTHROW(state->setValue(ThrowOnCopy{7, true})); + + CHECK(state->ready); + CHECK(fired == 2); + CHECK(seen == 7); + + // The attach-after-ready path is copy-free too, and still sees the genuine + // value rather than a husk -- the value is observed, never consumed. + int lateSeen = 0; + REQUIRE_NOTHROW(comp.then([&](const ThrowOnCopy& v) { lateSeen = v.payload; })); + CHECK(lateSeen == 7); +} + +TEST_CASE("Completion: a throwing T move leaves the state unsettled with every handler intact", + "[completion][issue-520][issue-553]") { + // The guarantee `setValue` documents: `value = std::move(val)` runs before + // `onOk` is drained or `ready` is set, so a throwing move constructor must + // leave the state exactly as it was -- unready, with every handler still + // attached -- rather than half-settled with its handlers already lost. + SyncExecutor exec; + auto state = std::make_shared>(); + morph::async::Completion comp{state, &exec}; + + int fired = 0; + comp.then([&](const ThrowOnMove&) { ++fired; }); + comp.then([&](const ThrowOnMove&) { ++fired; }); + + REQUIRE_THROWS_AS(state->setValue(ThrowOnMove{7, true}), std::runtime_error); CHECK_FALSE(state->ready); CHECK_FALSE(state->value.has_value()); @@ -268,7 +332,7 @@ TEST_CASE("Completion: a throwing T copy leaves the state unsettled with every h // And the state is still usable afterwards: the failed settlement consumed // nothing, so a later non-throwing one settles normally and both handlers // -- the ones that survived the throw -- run. - state->setValue(ThrowOnCopy{9, false}); + state->setValue(ThrowOnMove{9, false}); CHECK(state->ready); CHECK(fired == 2); diff --git a/tests/test_completion_value_contract.cpp b/tests/test_completion_value_contract.cpp new file mode 100644 index 00000000..1824cf1e --- /dev/null +++ b/tests/test_completion_value_contract.cpp @@ -0,0 +1,276 @@ +// SPDX-License-Identifier: Apache-2.0 + +// The value-handling contract of morph::async::Completion (morph#553), +// pinned by counting rather than by reading the header: +// +// - `T` need only be move-constructible. `Completion>` +// instantiates and fans out. +// - A handler taking `const T&` costs exactly **zero** copies. +// - A handler taking `T` by value costs exactly **one**. +// - Neither number depends on how many handlers are attached, nor on whether +// they attached before or after the completion settled. +// +// The counts are asserted **exactly**, never as an upper bound: an upper bound +// quietly absorbs the regression this file exists to catch. Before the fix the +// budget was N + 2M -- N copies for the handlers attached before settling and +// two for each one attached after -- regardless of handler signature, because +// every handler was erased as `std::function`. +// +// Every by-value `Probe` parameter below is deliberate and carries a NOLINT: +// `performance-unnecessary-value-param` is right that the copy is avoidable, +// and the copy is exactly what is being measured. +// +// `InlineExecutor` runs each posted closure synchronously on the calling +// thread, so nothing here measures scheduling and every count is deterministic. + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "test_support.hpp" + +using SyncExecutor = morph::testing::InlineExecutor; + +namespace { + +struct Counts { + int copies = 0; + int moves = 0; +}; + +// File-scope rather than a member of `Probe`: the whole point is to count +// constructions of a type the framework moves and copies behind our back, so +// the counter cannot live in the object being counted. Reset before each +// measurement, and every test here runs on one thread. +Counts gCounts; // NOLINT(cppcoreguidelines-avoid-non-const-global-variables) + +// Copy/move *constructors* only: those are what dispatch can charge. Assignment +// is deliberately not declared -- nothing on the value path assigns a settled +// `T` (`value` is engaged exactly once, by construction), so an assignment +// operator here would be a counter no test could ever read. +struct Probe { + int payload = 0; + + Probe() = default; + explicit Probe(int value) : payload{value} {} + Probe(const Probe& other) : payload{other.payload} { ++gCounts.copies; } + Probe(Probe&& other) noexcept : payload{other.payload} { ++gCounts.moves; } + Probe& operator=(const Probe&) = delete; + Probe& operator=(Probe&&) = delete; + ~Probe() = default; +}; + +enum class Sig : std::uint8_t { + ConstRef, // [](const Probe&) + ByValue, // [](Probe) + ErasedByValue // an existing std::function object +}; + +/// How many handlers attach before the completion settles, and how many after. +struct Shape { + int before = 0; + int after = 0; +}; + +void attach(morph::async::Completion& comp, Sig sig, int& fired) { + if (sig == Sig::ConstRef) { + comp.then([&fired](const Probe&) { ++fired; }); + } else if (sig == Sig::ByValue) { + // NOLINTNEXTLINE(performance-unnecessary-value-param) + comp.then([&fired](Probe) { ++fired; }); + } else { + // The legacy spelling: a caller that already holds a + // `std::function`. It must still convert, because + // `std::function` is constructible from anything + // invocable with `const T&`. + // NOLINTNEXTLINE(performance-unnecessary-value-param) + std::function legacy = [&fired](Probe) { ++fired; }; + comp.then(std::move(legacy)); + } +} + +/// Settles a fresh completion with @p shape.before handlers attached first and +/// @p shape.after attached once it is ready, and reports the copies/moves of +/// `Probe` charged to the whole sequence. +Counts measure(Shape shape, Sig sig, int& fired) { + SyncExecutor exec; + auto pair = morph::async::Completion::makeSettleable(&exec); + auto& comp = pair.first; + auto& promise = pair.second; + + fired = 0; + // Reset *after* constructing the completion so the handle's own setup is + // not charged to the measurement; `Probe` is not touched by it either way. + gCounts = Counts{}; + + for (int i = 0; i < shape.before; ++i) { + attach(comp, sig, fired); + } + promise.resolve(Probe{42}); + for (int i = 0; i < shape.after; ++i) { + attach(comp, sig, fired); + } + return gCounts; +} + +constexpr std::array kShapes{{ + {.before = 0, .after = 0}, + {.before = 1, .after = 0}, + {.before = 2, .after = 0}, + {.before = 3, .after = 0}, + {.before = 0, .after = 1}, + {.before = 0, .after = 2}, + {.before = 1, .after = 1}, + {.before = 3, .after = 3}, +}}; + +} // namespace + +TEST_CASE("Completion value contract: a const T& handler costs zero copies, at any handler count", + "[completion][issue-553]") { + for (const auto& shape : kShapes) { + INFO("before=" << shape.before << " after=" << shape.after); + int fired = 0; + const auto counts = measure(shape, Sig::ConstRef, fired); + CHECK(counts.copies == 0); + CHECK(fired == shape.before + shape.after); + } +} + +TEST_CASE("Completion value contract: a by-value handler costs exactly one copy, wherever it attached", + "[completion][issue-553]") { + for (const auto& shape : kShapes) { + const int handlers = shape.before + shape.after; + INFO("before=" << shape.before << " after=" << shape.after); + + int firedValue = 0; + const auto byValue = measure(shape, Sig::ByValue, firedValue); + CHECK(byValue.copies == handlers); + CHECK(firedValue == handlers); + + // A pre-existing `std::function` object is the same one copy, + // charged at the same place -- that handler's own parameter binding. + int firedErased = 0; + const auto erased = measure(shape, Sig::ErasedByValue, firedErased); + CHECK(erased.copies == handlers); + CHECK(firedErased == handlers); + } +} + +TEST_CASE("Completion value contract: settling itself moves T exactly twice, whatever is attached", + "[completion][issue-553]") { + // Two moves, and only two: the `Probe{42}` prvalue is elided into + // `resolve`'s by-value parameter, which moves into `setValue`'s, which + // moves into `value`. Nothing in dispatch adds one -- which is the claim. + // Pinned exactly so a future revision cannot slip an extra materialisation + // of the stored value past this file. + for (auto sig : {Sig::ConstRef, Sig::ByValue}) { + int fired = 0; + CHECK(measure({.before = 0, .after = 0}, sig, fired).moves == 2); + CHECK(measure({.before = 3, .after = 0}, sig, fired).moves == 2); + CHECK(measure({.before = 0, .after = 3}, sig, fired).moves == 2); + CHECK(measure({.before = 3, .after = 3}, sig, fired).moves == 2); + } +} + +TEST_CASE("Completion value contract: a mixed handler set charges one copy per by-value handler and no more", + "[completion][issue-553]") { + SyncExecutor exec; + auto pair = morph::async::Completion::makeSettleable(&exec); + auto& comp = pair.first; + auto& promise = pair.second; + + int fired = 0; + gCounts = Counts{}; + + comp.then([&fired](const Probe&) { ++fired; }); // before, free + // NOLINTNEXTLINE(performance-unnecessary-value-param) + comp.then([&fired](Probe) { ++fired; }); // before, one copy + comp.then([&fired](const Probe&) { ++fired; }); // before, free + + promise.resolve(Probe{42}); + + // NOLINTNEXTLINE(performance-unnecessary-value-param) + comp.then([&fired](Probe) { ++fired; }); // after, one copy + comp.then([&fired](const Probe&) { ++fired; }); // after, free + + CHECK(fired == 5); + CHECK(gCounts.copies == 2); +} + +TEST_CASE("Completion value contract: T need only be move-constructible", "[completion][issue-553]") { + // `Completion>` did not compile before morph#553 -- + // four sites copied `T` on the value path. Instantiating is not evidence on + // its own, so this fans out to handlers attached on both sides of the + // settle and checks each one actually ran against the real value. + SyncExecutor exec; + auto pair = morph::async::Completion>::makeSettleable(&exec); + auto& comp = pair.first; + auto& promise = pair.second; + + int sum = 0; + comp.then([&sum](const std::unique_ptr& ptr) { sum += *ptr; }); + comp.then([&sum](const std::unique_ptr& ptr) { sum += *ptr; }); + + promise.resolve(std::make_unique(21)); + + comp.then([&sum](const std::unique_ptr& ptr) { sum += *ptr; }); + + CHECK(sum == 63); +} + +TEST_CASE("Completion value contract: a move-only T survives the CallbackScope-gated attach too", + "[completion][issue-553][callback-scope]") { + // The gated overloads re-erase the handler through `CallbackToken::guard`, + // which is the one place a move-only `T` could still have been copied back + // into existence. It is not: the guard is generic and forwards `const T&`. + SyncExecutor exec; + const morph::async::CallbackScope scope; + auto pair = morph::async::Completion>::makeSettleable(&exec); + auto& comp = pair.first; + auto& promise = pair.second; + + int seen = 0; + comp.then(scope, [&seen](const std::unique_ptr& ptr) { seen = *ptr; }); + promise.resolve(std::make_unique(7)); + CHECK(seen == 7); + + // And a stopped scope still refuses delivery on a move-only T. + auto second = morph::async::Completion>::makeSettleable(&exec); + int refused = 0; + second.first.then(scope, [&refused](const std::unique_ptr&) { ++refused; }); + scope.requestStop(); + second.second.resolve(std::make_unique(9)); + CHECK(refused == 0); +} + +TEST_CASE("Completion value contract: a throwing by-value handler does not disturb its siblings' copies", + "[completion][issue-553]") { + // Fan-out isolation and the copy budget are independent: a handler that + // throws after taking its copy still costs exactly one, and the handlers + // after it still run and still cost exactly what their own signature says. + SyncExecutor exec; + auto pair = morph::async::Completion::makeSettleable(&exec); + auto& comp = pair.first; + auto& promise = pair.second; + + int fired = 0; + gCounts = Counts{}; + + // NOLINTNEXTLINE(performance-unnecessary-value-param) + comp.then([](Probe) { throw std::runtime_error{"handler blew up"}; }); + comp.then([&fired](const Probe&) { ++fired; }); + // NOLINTNEXTLINE(performance-unnecessary-value-param) + comp.then([&fired](Probe) { ++fired; }); + + REQUIRE_NOTHROW(promise.resolve(Probe{42})); + + CHECK(fired == 2); + CHECK(gCounts.copies == 2); +} diff --git a/tests/test_server_limits.cpp b/tests/test_server_limits.cpp index 548534fd..dcaf7920 100644 --- a/tests/test_server_limits.cpp +++ b/tests/test_server_limits.cpp @@ -189,16 +189,30 @@ TEST_CASE("benchmark: in-process execute round-trip", "[!benchmark][remote]") { BENCHMARK("RemoteServer round-trip (echo, 5 bytes)") { std::atomic next{0}; constexpr int n = 32; - std::atomic done{0}; + // Heap-allocated and co-owned by every reply callback, not a stack + // local captured by reference (morph#565). The wait below is bounded, + // so the body can and does return with replies still in flight; a + // stack-local counter is destroyed at that point and the straggler's + // `fetch_add` writes into a dead frame. Catch2 then re-enters this body + // for the next sample and constructs a fresh counter over the same + // stack slot, which is the write ThreadSanitizer reported racing + // against the late increment (2 data races, both frames in this test). + // + // The deadline stays: it is a benchmark timeout, not a correctness + // device. Its job is to keep a wedged server from turning a slow + // result into a hang; with the counter kept alive independently it no + // longer has to be right about anything, which is what lengthening it + // could never achieve. + auto done = std::make_shared>(0); for (int i = 0; i < n; ++i) { req.callId = ++next; server->handle(morph::wire::encode(req), - [&done](const std::string&) { done.fetch_add(1, std::memory_order_relaxed); }); + [done](const std::string&) { done->fetch_add(1, std::memory_order_relaxed); }); } auto deadline = std::chrono::steady_clock::now() + 1s; - while (done.load() < n && std::chrono::steady_clock::now() < deadline) { + while (done->load() < n && std::chrono::steady_clock::now() < deadline) { std::this_thread::yield(); } - return done.load(); + return done->load(); }; }