From 7599401da10402a7aa7d286620390e0313aecc52 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Fri, 25 Sep 2026 17:04:58 +0200 Subject: [PATCH] core: a handler attached to a settled Completion is isolated like one attached before setValue/setException wrap each composed handler in try/catch, log and continue. attachThen/attachOnError's fire-now path, taken when the handler is attached after settlement, posted the handler bare, so a throwing handler escaped into the executor. Which path runs is a race between the producer settling and the consumer attaching; under ASan the backend thread settles first often enough that ladder's "Presenter::track() calls finishOne() even when onOk itself throws" failed on master (run 36115939775) with the throw escaping pumpUntil. Both fire-now closures now carry the same isolation. Two tests attach a throwing handler after settlement on the inline executor, which makes the race deterministic; both fail without the fix. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Y4eif7wQNNhSkHUKYqq5Xq --- docs/spec/core/completion.md | 8 ++++++- include/morph/core/completion.hpp | 23 +++++++++++++++++-- tests/test_completion_multi_handler.cpp | 30 +++++++++++++++++++++++++ 3 files changed, 58 insertions(+), 3 deletions(-) diff --git a/docs/spec/core/completion.md b/docs/spec/core/completion.md index a96908b82..96692f704 100644 --- a/docs/spec/core/completion.md +++ b/docs/spec/core/completion.md @@ -94,6 +94,12 @@ throwing handler is logged and skipped without preventing the handlers attached after it from running — fan-out means every attached handler gets its turn, independent of an earlier one misbehaving. +A handler attached **after** settlement is posted on its own by +`attachThen`/`attachOnError` and gets the same `try`/`catch`. Which path a +handler takes is a race between the producer settling and the consumer +attaching, so a throw must be logged on both or it reaches the executor — the +Qt event loop, say — only when the attach happened to lose. + `setException` additionally sets `onErrAttached = (cbExec != nullptr)` — but only along the branch where at least one `onErr` handler was already registered. It marks the error handled (suppressing the orphan logger) **only @@ -628,7 +634,7 @@ for the same argument applied to the journal codec. | Empty completion | **Null state pointer makes `then`/`onError` no-ops** | Default-constructed `Completion` is a safe placeholder that never signals. | | 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** | A single-slot field would let a second `onError()` (or `then()`) on the same still-pending `Completion` silently replace the first handler. Composing (invoking every attached handler, in order) matches the mental model of an observer list and is what call sites composing behaviour via repeated attach 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`. | +| Per-handler exception isolation | **Each handler invocation — composed at settlement, or fired alone by a late attach — 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()`** | Without it, test code needing a `Completion` it can resolve/reject on demand has 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. | ## Limitations diff --git a/include/morph/core/completion.hpp b/include/morph/core/completion.hpp index 7adc66726..85d5f3d17 100644 --- a/include/morph/core/completion.hpp +++ b/include/morph/core/completion.hpp @@ -196,7 +196,19 @@ struct CompletionState : std::enable_shared_from_this> { // capturing that local *by copy* before moving it into the // handler costs two copies where the handler asked for at most // one, so a late attacher would pay 2 rather than 1. - fireNow = [self = this->shared_from_this(), handler = std::move(handler)]() { handler(*self->value); }; + // + // Isolated exactly as setValue's composed closure isolates each + // handler: which of the two paths a handler takes depends only + // on whether it was attached before or after settlement, so a + // throw must not reach the executor on one path and be logged + // on the other. + fireNow = [self = this->shared_from_this(), handler = std::move(handler)]() { + try { + handler(*self->value); + } catch (...) { + ::morph::log::logError("[completion] then handler threw; continuing with next handler"); + } + }; } else if (!ready) { onOk.push_back(std::move(handler)); } @@ -215,7 +227,14 @@ struct CompletionState : std::enable_shared_from_this> { onErrAttached = (cbExec != nullptr); if (ready && error) { auto savedErr = error; - fireNow = [handler = std::move(handler), savedErr]() mutable { handler(savedErr); }; + // Isolated as in setException's closure -- see attachThen. + fireNow = [handler = std::move(handler), savedErr]() mutable { + try { + handler(savedErr); + } catch (...) { + ::morph::log::logError("[completion] onError handler threw; continuing with next handler"); + } + }; } else if (!ready) { onErr.push_back(std::move(handler)); } diff --git a/tests/test_completion_multi_handler.cpp b/tests/test_completion_multi_handler.cpp index 7c4365161..5665af956 100644 --- a/tests/test_completion_multi_handler.cpp +++ b/tests/test_completion_multi_handler.cpp @@ -215,6 +215,36 @@ TEST_CASE("Completion: a throwing last then handler is isolated the same as a no REQUIRE(firstFired); } +TEST_CASE("Completion: a throwing then handler attached after settlement is isolated too", "[completion]") { + // A handler attached after the value is already stored takes the attach's + // own fire-now path rather than setValue's composed closure. It must get + // the same isolation: otherwise whether a throwing handler reaches the + // executor depends only on which side of settlement the attach landed. + const LogGuard guard; + SyncExecutor exec; + auto state = std::make_shared>(); + morph::async::Completion comp{state, &exec}; + state->setValue(1); + + bool laterFired = false; + REQUIRE_NOTHROW(comp.then([&](int) { throw std::runtime_error{"handler blew up"}; })); + comp.then([&](int) { laterFired = true; }); + REQUIRE(laterFired); +} + +TEST_CASE("Completion: a throwing onError handler attached after settlement is isolated too", "[completion]") { + const LogGuard guard; + SyncExecutor exec; + auto state = std::make_shared>(); + morph::async::Completion comp{state, &exec}; + state->setException(std::make_exception_ptr(std::runtime_error{"err"})); + + bool laterFired = false; + REQUIRE_NOTHROW(comp.onError([&](const std::exception_ptr&) { throw std::runtime_error{"handler blew up"}; })); + comp.onError([&](const std::exception_ptr&) { laterFired = true; }); + REQUIRE(laterFired); +} + TEST_CASE("Completion: mismatched attach (onError on a value-ready state) is still a no-op for all handlers", "[completion][issue-59]") { SyncExecutor exec;