Skip to content

BridgeHandler callbacks need a lifetime-and-stop token: a data member, not a base class #138

Description

@Yaraslaut

The gap

morph::async::Completion<T>'s consumer-side attachment surface is then and onError (plus the Promise/makeSettleable settling seam). There is no cancel, no detach, and no way to bind a handler to a receiver's lifetime — docs/spec/core/completion.md lists cancellation under Out of scope in so many words.

But a Completion always resolves through an executor — even a local backend's immediate resolution is posted, not delivered inline (CompletionState<T>::attachThen). So the receiver can always be destroyed, or lose interest, before the handler runs. The framework gives callers no way to say so, and the natural spelling is silently wrong:

completion.then([this](GetBoardResult r) { /* `this` may be long gone */ });

Correctness depends on every author independently remembering a three-part incantation: declare a token member (last!), capture its weak form, re-check it before touching this.

Evidence that this is load-bearing

The pattern is reimplemented by hand across the tree (counts as of 2026-08-23 master; the commands regenerate them):

count regenerate with
classes declaring their own std::shared_ptr<const void> _liveness 6 rg 'shared_ptr<const void> _liveness' -g '*.hpp'
sites capturing std::weak_ptr<const void>{_liveness} 30 rg 'weak_ptr<const void>' -g '*.{hpp,cpp}'
production files using a different idiom for the same hazard (QPointer) 2 (8 uses) rg 'QPointer' -g '*.{hpp,cpp}'

When this issue was filed two days ago the counts were 5 and 23. The delta is ledger rung 5: ReportJobPoller is the sixth hand-rolled token, and its implementation plan (docs/superpowers/plans/2026-08-19-ledger-rung5.md) prescribes the incantation at four separate task sites — "_liveness last-declared", "must stay last-declared". The boilerplate is now part of the instructions for writing new code, and it compounds with every rung.

Beyond the counts:

  • Bridge guards itself with _liveness, and its comment names the hazard exactly:

    the installed handler captures this, and a co-owned backend that outlives the Bridge could otherwise fire it after destruction and dereference freed memory […] the handler additionally guards on _liveness

  • TimeoutScheduler's browser build re-derives it a third time inside the framework, and says so: "the same weak-token pattern as morph::bridge::Bridge::_liveness" (timeout_scheduler.hpp).

  • BridgeHandler itself holds the weak half (_bridgeAlive) to make teardown order-independent.

  • The declared-last convention — the token must be the last member so it expires before anything a callback touches — is separately re-documented in at least six places (event_poller.hpp, poll_qml_bridges.hpp ×2, report_job_poller.hpp, ledger_model.hpp, bridge.hpp). Every new class re-learns and re-states it.

The framework knows about the problem, solves it for itself by hand in three places, ships no primitive for anyone else, and its spec (docs/spec/concurrency_and_lifetimes.md) teaches the incantation rather than a type. Two different idioms coexist for one hazard, so a reader has to recognise both and an author has to know which context they are in.

What forgetting costs: #137 was a real stack-use-after-scope write. Presenter::track() captured a bare this, while Presenter::trackBound() — the method immediately above it in the same file — uses QPointer and documents precisely why. It was invisible in every unsanitised build and caught only when the ASan job first ran.

Two requirements, not one

Liveness is only half of it.

  1. The receiver no longer exists. Destroyed while a reply was in flight.
  2. The receiver no longer cares. The user navigated away, cancelled the dialog, typed a new query superseding the in-flight one, closed the tab. The object is perfectly alive and the callback must still not run.

(2) is not expressible in morph at all, and it is the common case in a GUI. A stale search result overwriting a newer one is a bug users actually see.

The newest code in the tree makes the case concretely: ReportJobPoller's guards check alive.expired() || _finished — a hand-rolled liveness token and a hand-rolled stop flag, side by side, because the framework provides neither half.

Design

A separate token type, held as a data member — deliberately not a base class.

Inheritance is the wrong tool here: requiring every consumer of BridgeHandler to derive from a framework base constrains its hierarchy for what should be an implementation detail, and penalises types that already have a base, are QObjects, or are aggregates. A member composes; a base class does not. (#150 tried the base-class shape and was closed for exactly this.)

class BoardPresenter {
    // ... whatever base, or none

    void load() {
        handler.execute(GetBoard{})
            .then(_callbacks, [this](GetBoardResult r) { render(r); });
    }

    void onUserNavigatedAway() {
        _callbacks.requestStop();   // in-flight replies stop being delivered
    }

    void onNewQuery(QString q) {
        _callbacks.reset();         // supersede: old replies dead, new ones deliverable
        // ... issue the new request under the fresh generation
    }

    morph::async::CallbackScope _callbacks;   // plain data member; declared last
};

The handler captures a weak token derived from the scope. A callback runs only if, at delivery time, the scope is both alive and not stopped. Destroying the scope and calling requestStop() have the same effect on pending callbacks; they differ only in whether the owner still exists. reset() is the third verb, for requirement (2)'s supersede case: everything captured so far goes permanently dead, the scope itself becomes deliverable again — without it, "a new query cancels the old one" has no spelling short of heap-reallocating the scope per request.

Sketch:

namespace morph::async {

class CallbackToken;     // weak observer, captured by the handler

class CallbackScope {    // owned by the receiver, as a member
  public:
    CallbackScope();
    ~CallbackScope();                     // implicitly stops everything pending
    CallbackScope(const CallbackScope&) = delete;   // identity, not a value
    CallbackScope(CallbackScope&&) = delete;        // pinned; reset() covers regeneration

    void requestStop() noexcept;          // explicit: "I no longer care"
    void reset();                         // tokens issued so far go dead; scope live again
    [[nodiscard]] bool stopRequested() const noexcept;
    [[nodiscard]] CallbackToken token() const noexcept;

    // Guard an arbitrary callable with the same gate — QTimer ticks,
    // IExecutor::post closures, poller dispatch, subscribe sinks:
    template <typename F>
    [[nodiscard]] auto guard(F&& fn) const;   // wrapped fn no-ops once inactive
};

class CallbackToken {
  public:
    [[nodiscard]] bool active() const noexcept;   // scope alive AND not stopped (advisory)
    // guard(fn) here too; CallbackScope::guard forwards to it
};

}

Completion<T> gains then(scope, fn) / onError(scope, fn) overloads (token-taking forms too), and the unguarded spellings are renamed to something that must be typed on purpose — thenDetached / onErrorDetached — so a genuinely detached callback says so and the unguarded form stays greppable in review. Mechanically the gate lives inside the stored handler (wrapped at attach), so it needs no CompletionState changes and composes with handler fan-out and the attach-after-ready fire-now path as they are.

guard() matters as much as the overloads: of today's 30 capture sites, roughly a third protect QTimer ticks, posted closures, poller dispatch and event sinks rather than direct then/onError attachments (backend_rig.hpp's QTimer::singleShot, the QML bridges' event sinks, event_poller.hpp's dispatch). Without a standalone guard those sites keep the incantation and only the Completion subset improves.

BridgeHandler::subscribe<R> is the remaining surface: sinks are stored std::functions that fire repeatedly — the longest-lived callbacks in the system — delivered through the same executor marshalling (they are completion callbacks, per concurrency_and_lifetimes.md). A subscribe(scope, cb) overload gates delivery; whether a dead sink is also lazily pruned from the subscriber list is an implementation choice on top.

The guarantee's boundary

This belongs in the spec section from day one, because it is the part an API like this is silently assumed to promise and cannot:

  • Executor-affine use gets the full guarantee. When the scope's destruction / requestStop() / reset() happen on the delivery executor's thread — the normal case: receiver and its callbacks both live on the GUI thread — check-then-run is atomic with respect to the scope operations, and a gated callback never touches a dead or stopped receiver.
  • Cross-thread stop is advisory. A scope destroyed on a different thread from the delivery executor can expire between the token check and the handler body. That is exactly the boundary of today's weak_ptr idiom (locking the token pins the token, never the receiver); it carries over unchanged, and external synchronisation there remains the caller's job.
  • Deliberately no block-until-drained. requestStop() / ~CallbackScope do not wait for in-flight callbacks (QObject::disconnect-style). A GUI-thread destructor blocking on a pool-thread callback that is itself blocked posting back to the GUI executor is a deadlock by construction — the same self-join family concurrency_and_lifetimes.md already warns about. If a stronger cross-thread guarantee is ever wanted, it can arrive later as a separate opt-in without breaking this contract.
  • Declared last, destroyed first stays a rule — now stated once in the scope's own docs instead of re-derived per class: the member goes after (below) everything its callbacks touch, same rationale Bridge::_liveness documents today.
  • Teardown that pumps. Members are destroyed after the destructor body, so a destructor body that can pump a nested event loop (sendSync-style) can still deliver into a half-dead receiver. The escape hatch is explicit: such a destructor calls requestStop() first. One line in the spec.
  • A suppressed callback is destroyed, not leaked: its closure (and captures) are released on the delivery executor at the moment delivery is refused. A scope-suppressed error still counts as handled for orphan logging — suppression is deliberate; whether suppression additionally emits a trace-level log for diagnosability is an open implementation choice.
  • Optional hardening: a debug-build assert that scope operations happen on the delivery executor's thread, once IExecutor has a cheap "am I on your thread?" query.

On the name

CallbackContext is the obvious candidate and should be avoided: morph::session::Context already exists in this codebase and means something entirely different (authenticated principal, token, request id). Two unrelated "Context" types in one framework is a readability tax.

Candidates, in preference order:

  1. CallbackScope / CallbackToken — the scope is owned, the token is observed. Mirrors std::stop_source/std::stop_token, which every C++20 reader already knows, while adding the liveness half that stop_source lacks. Mild tension with this repo's Scoped* prefix, which denotes RAII installers (ScopedContext, ScopedActionLog, ScopedLoggerOverride) — this is a plain member, not an installer.
  2. CallbackGate / GateToken — no collision at all; "gate" states what it does to a callback.
  3. DeliveryScope / DeliveryToken — emphasises that it gates delivery, not the work itself.

Relationship to std::stop_token, #116 and #118

Deliberately not std::stop_token alone: that covers requirement (2) but not (1), since destroying a stop_source does not request stop. Interoperating with it — accepting an externally supplied std::stop_token so callbacks tie into an existing cancellation tree — is worth designing in early, because #116 (deadline/cancellation propagation across the executor abstraction) is the work-side half of this story: this issue gates delivery of results nobody wants; #116 would cancel the work producing them. Those must end up one vocabulary, not two — if #116 lands, requestStop() is its natural upstream trigger, and CallbackToken should be expressible as / constructible from a std::stop_token rather than becoming a third species. Likewise #118 (periodic-task facility): a recurring task's "stop ticking" handle should be this same scope, not a parallel invention.

completion.md already draws the right line for the client-side deadline ("bounds how long the caller waits … and does nothing to the work still in flight underneath"); the scope's stop is the same kind of thing, generalised from "too late" to "don't care", and the spec wording should keep that distinction sharp. And if Completion ever grows a coroutine awaitable adapter (its Limitations say "No co_await" today), the token is what that adapter's cancellation maps onto — mirroring std::stop_token keeps that door open too.

Scope

  • New header under include/morph/core/ (e.g. callback_scope.hpp), below bridge.hpp in the include graph: EventPoller, the testkit and example GUI code must be able to use it without pulling in Bridge, and Bridge itself must be able to use it.
  • Token pair in morph::async; then/onError overloads on Completion<T> (scope- and token-taking) plus the renamed detached spellings; guard(); a subscribe(scope, cb) overload on BridgeHandler.
  • Migrate the ~30 capture sites: the 6 hand-rolled _liveness tokens collapse into it, ReportJobPoller's _finished becomes requestStop(), and the 2 QPointer files can follow where the receiver isn't relying on QObject-specific semantics.
  • Bridge's own guards become uses of the primitive rather than a private reimplementation (TimeoutScheduler's browser-build token guards internal state, not user callbacks — optional).
  • Public API change, so specs: a new section in docs/spec/core/completion.md (rewording its "No cancellation" limitation to distinguish delivery-stop from work-cancellation), bridge.md, the liveness-token sections and cheat-sheet of concurrency_and_lifetimes.md, and docs/spec/VERSIONING.md.
  • Tests: carry over core: HasLifetime and lifetime-bound Completion overloads (stage 1, draft) #150's verification strategy — call counters held in shared_ptrs that outlive the receiver, so "the callback body did not run" is directly observable rather than resting on a sanitizer catching UB; mutation-check the gate (disabling the expiry check must fail assertions). Add the requirement-(2) cases core: HasLifetime and lifetime-bound Completion overloads (stage 1, draft) #150 never had: stop-then-deliver, reset-then-deliver-old-token, reset-then-deliver-new-token.

Migration is mechanical and stageable: land the primitive plus the renamed overloads first, convert callers per subsystem, and only then deprecate the unguarded spellings. End state: then(fn) without a scope is a deprecation warning, so every call site either names a scope or says thenDetached — the compile-time pressure #150 got from its concept constraint, recovered without the base class.

Related

Metadata

Metadata

Assignees

No one assigned

    Labels

    area: coreSubsystem: coreenhancementNew feature or requesttriage: validWell-framed; implement as written

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions