diff --git a/docs/spec/concurrency_and_lifetimes.md b/docs/spec/concurrency_and_lifetimes.md index 2eff27a92..011e1ac7e 100644 --- a/docs/spec/concurrency_and_lifetimes.md +++ b/docs/spec/concurrency_and_lifetimes.md @@ -281,6 +281,23 @@ that bounded wait into an unbounded one. Four dispositions, by site: `BridgeLifetime` across its whole touch of `this` (`_mtx`, `loadBackend()`). Safe to hold the gate here: nothing in that span calls into consumer code or a blocking backend path, only a mutex and a pointer comparison. + + Since morph#593 that callback may also run **on the registering thread + itself**: unless the backend answers `IBackend::BindWait::kCallerMustNotBlock`, + `registerHandlerImpl` waits for the bind completion and then delivers the + outcome from its own frame, so `registerHandler` returns a bound handler. That + reinstates, for one statement, exactly the blocking window every backend had + before morph#568, when the fallback was the synchronous + `registerModelWithContext`: a thread parked inside `registerHandler` is a + thread not running `~Bridge`, and a *different* thread destroying the `Bridge` + while `registerHandler` is still on this one was already a misuse then and is + no more possible now. What is new is only that the parking is visible in + `Bridge` rather than inside the backend verb. The two + `kCallerMustNotBlock` backends never park at all, which is the point: for + `QtWebSocketBackend` under `asyncRegistrationEnabled` the reply arrives on the + parked thread's own event loop, so parking would not be a slow teardown but a + deadlock — the same shape of objection that rules a gate out for the reconnect + handler below. - **`installReconnectHandler`'s reconnect callback.** Left as a `liveness()` check, deliberately not moved to `BridgeLifetime` — this is the case the first paragraph above warns about. The handler runs on the backend's @@ -298,13 +315,17 @@ that bounded wait into an unbounded one. Four dispositions, by site: `CallbackToken::active()` check and then takes `_attachMtx` and calls `loadBackend()`, so the two-step shape is present in the source. What closes the window is not a gate but a contract on the backend: - `IBackend::registerModelAsync`'s doc comment now states that a backend + `IBackend::registerModelAsync`'s doc comment states that a backend overriding any `*Async` hook must deliver its callbacks on a thread from - which `~Bridge` cannot run concurrently. `QtWebSocketBackend` — the only - backend in the tree that overrides them — satisfies this by construction - rather than by care: it must itself be used from the Qt event loop thread, - and fires all four callbacks from `onTextMessage` on that same thread, so the - check and the use cannot straddle a destructor. Gating these instead would + which `~Bridge` cannot run concurrently. Since morph#568 **no backend + overrides them**: each of the three sites now reaches + `IBackend::bindModel`/`promoteModel` instead, naming + `exec::detail::inlineExecutor()` as the delivery executor. That reproduces the + old delivery thread exactly — the continuation runs wherever the backend + settled — so the window is unchanged, and `QtWebSocketBackend` is still safe + for the same reason it was: it must itself be used from the Qt event loop + thread, and settles every reply from `onTextMessage` on that same thread, so + the check and the use cannot straddle a destructor. Gating these instead would make `~Bridge` block behind `_attachMtx`, which the synchronous `attachHandler` holds across a full `attachModel` round trip — the same shape of objection that rules a gate out for the reconnect handler. **The safety @@ -313,14 +334,20 @@ that bounded wait into an unbounded one. Four dispositions, by site: thread would reopen morph#486's use-after-free, and that is a contract break rather than a latent race to be rediscovered. - A structural alternative now exists alongside these four hooks and is what - replaces them: `IBackend::bindModel`/`promoteModel` take the executor the - continuation is delivered on as an argument, so the delivery thread is chosen - by the caller — which knows what its own teardown looks like — instead of by - the backend, which does not. That does not by itself close the window above; - it relocates the decision from a documented obligation on fifteen - implementors to a value one call site produces. Nothing in `Bridge` uses it - yet. See [core/backend.md](core/backend.md#the-structural-registration-surface--bindmodel-and-promotemodel) + The structural surface that replaces these four hooks — + `IBackend::bindModel`/`promoteModel` — takes the executor the continuation is + delivered on as an argument, so the delivery thread is chosen by the caller, + which knows what its own teardown looks like, instead of by the backend, which + does not. `Bridge` now reaches it at all four sites (morph#568). **That does + not close the window above, and morph#568 does not claim it does**: `Bridge` + owns no event loop, so the executor it names is + `exec::detail::inlineExecutor()` — "deliver wherever you settled", which is + what the prose contract already required. What changed is where the decision + lives: one value produced at four `Bridge` call sites, rather than a + documented obligation on every `IBackend` implementor. Closing the window + means giving `Bridge` an executor bound to the thread that runs `~Bridge` and + naming that instead; nothing in the morph#522 set does that. See + [core/backend.md](core/backend.md#the-structural-registration-surface--bindmodel-and-promotemodel) and morph#522. `switchBackend()` and `whenBound()` were audited for the same shape and do not diff --git a/docs/spec/core/backend.md b/docs/spec/core/backend.md index 0853aa4b9..33f1115c8 100644 --- a/docs/spec/core/backend.md +++ b/docs/spec/core/backend.md @@ -28,8 +28,10 @@ and react to backend changes. - [The dispatch struct — `ActionCall`](#the-dispatch-struct--actioncall) - [The abstract interface — `IBackend`](#the-abstract-interface--ibackend) - [Connect/disconnect notifications](#connectdisconnect-notifications) +- [Waiting for a bind — `bindWaitPolicy`](#waiting-for-a-bind--bindwaitpolicy) - [Asynchronous registration — `registerModelAsync`](#asynchronous-registration--registermodelasync) - [The structural registration surface — `bindModel` and `promoteModel`](#the-structural-registration-surface--bindmodel-and-promotemodel) + - [What a natively non-blocking backend does to `registerHandler`](#what-a-natively-non-blocking-backend-does-to-registerhandler) - [Error types](#error-types) - [`LocalBackend` — in-process execution](#localbackend--in-process-execution) - [`RemoteServer` — server-side message handler](#remoteserver--server-side-message-handler) @@ -75,10 +77,11 @@ holds a `unique_ptr` and delegates all model operations to it. | Method | Purpose | |---|---| | `registerModel(typeId, factory)` | Registers a new model instance, returns its opaque `ModelId`. | -| `registerModelWithContext(typeId, factory, contextKey)` | Same as `registerModel`, additionally passes a stable identity (e.g. account id). Default implementation drops `contextKey` and forwards to `registerModel` — correct for `LocalBackend` where the factory closure already captures identity. Every backend whose instances live behind a wire protocol overrides it to carry `contextKey` across: `SimulatedRemoteBackend` and `SocketBackend` both do. Not cosmetic — `RemoteServer::attachLogIfConfigured` skips the `LogProvider` lookup entirely on an empty `contextKey`, so a wire backend that drops the key leaves the instance with **no** action log rather than a log missing a field (morph#587). | -| `registerModelAsync(typeId, factory, contextKey, onRegistered, onError)` | Optional non-blocking counterpart to `registerModelWithContext`. Returns `false` by default (no async path); `Bridge::registerHandler()` prefers this when it returns `true` and falls back to the synchronous call otherwise. See [Asynchronous registration](#asynchronous-registration--registermodelasync). | +| `registerModelWithContext(typeId, factory, contextKey)` | Same as `registerModel`, additionally passes a stable identity (e.g. account id). Default implementation drops `contextKey` and forwards to `registerModel` — correct for `LocalBackend` where the factory closure already captures identity. Every backend whose instances live behind a wire protocol overrides it to carry `contextKey` across: `SimulatedRemoteBackend`, `SocketBackend` (morph#587) and `QtWebSocketBackend` (morph#594) all do. Not cosmetic — `RemoteServer::attachLogIfConfigured` skips the `LogProvider` lookup entirely on an empty `contextKey`, so a wire backend that drops the key leaves the instance with **no** action log rather than a log missing a field (morph#587). | +| `registerModelAsync(typeId, factory, contextKey, onRegistered, onError)` | Optional non-blocking counterpart to `registerModelWithContext`. Returns `false` by default, and since morph#568 no backend overrides it, so it always does; `Bridge::registerHandler()` then falls back to `bindModel`. Removed by morph#571. See [Asynchronous registration](#asynchronous-registration--registermodelasync). | | `bindModel(request, cbExec)` | Acquires a model instance and returns a `Completion` delivered on `cbExec`. One verb covering `registerModelWithContext`, `registerModelShared` and `attachModel`, selected by the request's shape. The preferred surface — see [The structural registration surface](#the-structural-registration-surface--bindmodel-and-promotemodel). | | `promoteModel(request, cbExec)` | Files an already-live instance under a key and returns a `Completion` delivered on `cbExec`. The structural counterpart of `assignPrimary`. | +| `bindWaitPolicy()` | Whether a caller may block its own thread until a `bindModel`/`promoteModel` completion settles. `BindWait::kCallerMayBlock` by default. The only framework caller is `Bridge::registerHandlerImpl`; see [Waiting for a bind — `bindWaitPolicy`](#waiting-for-a-bind--bindwaitpolicy). | | `deregisterModel(mid)` | Removes the model identified by `mid`. | | `execute(mid, call, cbExec)` | Dispatches `call` against the model identified by `mid`. Returns a `Completion>`. | | `notifyBackendChanged()` | Called by `Bridge::switchBackend()` after all handlers are re-registered. | @@ -155,6 +158,15 @@ envelope, so there is nothing to stamp. ## Asynchronous registration — `registerModelAsync` +> **Status.** Since morph#568 **no backend in the tree overrides any of the four +> `*Async` verbs.** They still exist on `IBackend`, still default to `false`, +> and `Bridge` still offers them first — but every implementor now reaches +> `Bridge` through [the structural registration +> surface](#the-structural-registration-surface--bindmodel-and-promotemodel) +> below, which is what `Bridge` falls back to when a verb answers `false`. This +> section describes the older shape, which morph#571 removes; read it for what +> the `*Async` contract *was*, not for how registration works today. + `registerModel`/`registerModelWithContext` are synchronous: a backend whose registration requires a round-trip can only implement that by blocking the calling thread until the reply arrives — `QtWebSocketBackend` does this via a @@ -175,9 +187,10 @@ either callback. the pre-built-binding overload) prefers this path: it adds the binding to `_handlers` and calls `registerModelAsync` *before* acquiring `Bridge::_mtx` for the callback (a synchronous callback invocation would otherwise -self-deadlock re-acquiring the lock). If it returns `false`, `registerHandler` -falls back to the synchronous `registerModelWithContext`, exactly as before -this feature existed. If it returns `true`, the binding is returned **unbound** +self-deadlock re-acquiring the lock). If it returns `false` — which, since +morph#568, it always does — `registerHandler` falls back to `bindModel` with an +empty `primary` and a zero `current`, the request shape that means +`registerModelWithContext`. If it returns `true`, the binding is returned **unbound** (`currentId == 0`) — `executeVia` fails fast with "handler not bound" for any call made before `onRegistered` fires, so a caller using the async path must wait for registration (e.g. gate its UI on it) rather than fire an action @@ -200,11 +213,14 @@ and the reconnect handler perform after a backend swap remains synchronous; giving that an async path too is a larger change to `Bridge`'s locking model, left for a future issue if it proves necessary. -`QtWebSocketBackend` is the one backend that currently overrides this, gated -by `QtWebSocketBackendConfig::asyncRegistrationEnabled` (default `false` — see -its own section below). +`QtWebSocketBackend` was the one backend that ever overrode this; morph#568 +moved it to `bindModel`, so nothing overrides it now. Everything the two +paragraphs below describe still happens — it happens inside +`QtWebSocketBackend::bindModel`, still gated by +`QtWebSocketBackendConfig::asyncRegistrationEnabled` (default `false` — see its +own section below). -**Queueing before the first connect.** A `registerModelAsync` call made before +**Queueing before the first connect.** A non-blocking private bind made before the socket has finished connecting is **queued**, not failed — this is exactly the ordering a single-threaded WASM client must use, since it can never block waiting for the connection to settle (a `BridgeHandler` constructed the moment @@ -213,8 +229,8 @@ request is sent, in FIFO order, the moment `connected` fires next (the first connect included, before the reconnect handler runs) — no protocol change: a call-id is assigned only at send time, same as the immediate path. If the socket is torn down (destroyed, or disconnects) before ever connecting, the -queue is drained by `cancelPending`, which still invokes each queued request's -`onError` exactly once, exactly like an in-flight (already-sent) registration +queue is drained by `cancelPending`, which still rejects each queued request's +`Completion` exactly once, exactly like an in-flight (already-sent) registration would. See `QtWebSocketBackend`'s own section below. ### Shared/keyed registration — `registerModelSharedAsync` / `attachModelAsync` @@ -232,12 +248,13 @@ in: | `registerModelSharedAsync(typeId, factory, identity, onRegistered, onError)` | `registerModelShared` | `Bridge::ensureBoundAsync` | | `attachModelAsync(typeId, factory, identity, current, onRegistered, onError)` | `attachModel` | `Bridge::attachHandlerAsync` | -`QtWebSocketBackend` implements both behind the same -`asyncRegistrationEnabled` flag, reusing the same `callId`-keyed pending map -(reply routing is verb-agnostic — a `register`, a shared `register`, and an -`attach` all reply the same way). An empty `identity.primary` degrades to -`registerModelAsync`, mirroring the synchronous methods' degrade-to-private -behaviour. +`QtWebSocketBackend` used to implement both behind the same +`asyncRegistrationEnabled` flag. Since morph#568 it implements neither: both +shapes are reached through `bindModel`, which keeps the same +`asyncRegistrationEnabled` gate, the same `callId`-keyed pending map (reply +routing is verb-agnostic — a `register`, a shared `register`, an `attach` and an +`assign` all reply the same way) and the same degrade-to-private behaviour for +an empty `primary`. Unlike the synchronous `attachModel`'s default implementation, `attachModelAsync` does **not** release `current` itself: an overriding backend @@ -295,19 +312,27 @@ The other three cannot take that same gate. It makes `~Bridge` *block* for the gated span, and each span acquires `_attachMtx` — which the synchronous `Bridge::attachHandler` holds across a full `attachModel` round trip, unbounded on a wire backend. What closes the window instead is the delivery thread. -`QtWebSocketBackend` — the only backend in the tree overriding any of the four — -satisfies the contract by construction rather than by care: it must itself be -used from the Qt event loop thread, and every *reply-driven* callback fires from -`onTextMessage` on that same thread, so the check and the use cannot straddle a -destructor. Its two non-reply paths do not weaken this — a disconnected or no-op -dispatch invokes the callback inline, inside the caller's own frame (which -`Bridge::detail::parkIfInFrame` exists to handle), and `cancelPending` fires the -remainder from `~Bridge` itself, which is not a *concurrent* destructor. - -A backend that replies on its own transport thread therefore reopens #486's -use-after-free. That is a **contract break**, diagnosable from this page and -from `IBackend::registerModelAsync`'s doc comment — not a latent race to be -rediscovered by a sanitizer. + +**Where that stands after morph#568.** No backend overrides the four verbs any +more, so nothing is left for this contract to bind. What the delivery thread +*is*, however, has not changed, and neither has the window: `Bridge` reaches the +structural surface naming `exec::detail::inlineExecutor()`, so a continuation +still runs on whichever thread the backend settled on. For +`QtWebSocketBackend` that is `onTextMessage`, on the Qt event loop thread it +must itself be used from — the same by-construction safety it had before, now +arrived at by the same route the prose used to describe. Its two non-reply paths +do not weaken this either: a disconnected or no-op bind settles inline, inside +the caller's own frame (which `Bridge::detail::parkIfInFrame` exists to handle), +and `cancelPending` settles the remainder from `~Bridge` itself, which is not a +*concurrent* destructor. + +So morph#568 did not close #486's window, and does not claim to. It moved the +decision: which thread a registration continuation runs on is now a value +`Bridge` produces at four call sites rather than an obligation on every backend +author, and a `Bridge` that grows an executor of its own can change it in one +place. Until it does, a backend that replies on its own transport thread would +still reopen #486 — see [How the threading contract becomes +structural](#how-the-threading-contract-becomes-structural). ## The structural registration surface — `bindModel` and `promoteModel` @@ -424,6 +449,14 @@ natively](#the-structural-registration-surface-natively). executor, so the caller returns immediately with an unresolved `Completion`. A single-threaded WASM main thread has no such executor to offer, which is why morph#568 puts `QtWebSocketBackend` on the surface natively instead. +- **It answers `bindWaitPolicy()` itself rather than forwarding it**, with + `kCallerMustNotBlock`. `bindModel`/`promoteModel` are the two verbs it + reshapes, so the policy describing them describes the adapter and not what it + wraps. A caller that waited would pay back exactly the blocking cost the + adapter was interposed to move, and would deadlock outright if it happened to + be running on `blockingExec`. This is why a `Bridge` over an adapter returns + an unbound handler while a `Bridge` over `SocketBackend` — also non-blocking + — does not. - **The executor is required.** An adapter that ran the call inline when handed nothing would be a `bindModel` that blocks on some configurations and not others — contract by configuration, which is what is being removed. @@ -452,19 +485,208 @@ the `Completion` when its reply arrives. There is no `bool`, no fallback verb and no inline-completion special case to declare: settling the `Completion` inside the dispatch call and settling it a second later from a transport thread are the same code at the call site, because delivery goes through `cbExec` -either way. That is the shape morph#568 moves `QtWebSocketBackend` onto, and -the shape morph#569 moves `SocketBackend` onto. +either way. + +Two backends in the tree take that route: `QtWebSocketBackend` (morph#568) and +`SocketBackend` (morph#569). `SocketBackend`'s case — why it is native rather +than wrapped, and what that does to its reconnect-handler hazard — is set out +under [The structural registration surface, +natively](#the-structural-registration-surface-natively). +`QtWebSocketBackend`'s follows here. + +Its `bindModel` reads: + +- `asyncRegistrationEnabled` unset → `IBackend::bindModel`, the blocking + default. Desktop behaviour, unchanged. +- set → build the envelope the request's shape names, assign a `callId`, send, + and return an unsettled `Completion` that `onTextMessage` settles. A private + bind made before the socket has connected is queued instead; a keyed one + rejects. + +The request-shape mapping is the one `SocketBackend` implements too — empty +`primary` with a zero `current` is a private `register`; empty `primary` with a +live `current` gives that instance up first and then binds privately; a +non-empty `primary` with a zero `current` is a shared `register`; with a live +one it is an `attach`. The two native implementations differ in transport and +in gating, not in what a `BindRequest` shape means. + +**They do differ in gating, and after morph#568 that difference is +observable.** `SocketBackend::bindModel` is unconditionally non-blocking; +`QtWebSocketBackend::bindModel` falls back to the blocking default unless +`asyncRegistrationEnabled` is set. Since morph#568 puts `Bridge` on this +surface, the gate now decides whether `Bridge::registerHandler` returns a +*bound* handler: a blocking `bindModel` settles inside the dispatch frame, so +the handler is bound on return, and a non-blocking one does not, so it is not. +See [What a natively non-blocking backend does to +`registerHandler`](#what-a-natively-non-blocking-backend-does-to-registerhandler) +below — that is a property of the surface, not of either transport. + +Two things that were four-way duplicated collapsed with the verbs. There is one +send path (`sendControl`), so the "encode before recording the pending entry" +invariant and the `env.session` stamp of morph#495 each exist once rather than +four times; and there is one pending map, because `register`, shared +`register`, `attach` and `assign` replies were always matched identically. +(`SocketBackend` reached the same conclusion independently and calls its own +`sendControlAsync`/`_pendingControl`; the Qt member keeps its older +`_pendingRegistrations` name.) + +The single-threaded WASM registration path — the case the four `*Async` verbs +were added for — therefore has **no special case left in the interface**. It is +the ordinary shape of `bindModel` on a backend whose transport is +non-blocking. `tests/qt/test_qt_websocket.cpp` pins it end to end against a real +`RemoteServer` (queue before connect, register-or-attach, re-point, +degrade-to-private, reject on a dead socket, promote), and +`examples/common/testkit/test_wasm_registration_path_native.cpp` pins the same +path through `Bridge`, where a fallback to the blocking verb would abort on the +nested `QEventLoop` `sendSync` needs. + +`QtWebSocketBackend` deliberately does **not** go through +`SynchronousBackendAdapter`: the adapter moves a blocking call to another +thread, and a WASM main thread has no other thread to move it to. + +### What a natively non-blocking backend does to `registerHandler` + +`Bridge::registerHandler` is synchronous and returns a `BridgeHandler`. What it +cannot do is make the *backend* synchronous. Before morph#568 the fallback under +`registerModelAsync` was the blocking `registerModelWithContext`, so on every +backend except a `QtWebSocketBackend` with `asyncRegistrationEnabled` set, the +handler was bound by the time the constructor returned. After morph#568 the +fallback is `bindModel`, so the rule is stated once, structurally: + +> **`registerHandler` returns a bound handler unless the backend says the +> caller must not wait.** A backend that has not overridden `bindModel` gets +> `IBackend`'s default, which runs the legacy blocking verb and settles before +> returning — bound, with no wait. A backend that overrode `bindModel` natively +> settles when its reply lands, and `registerHandlerImpl` **waits for it** — +> still bound — unless that backend answers +> `BindWait::kCallerMustNotBlock`, in which case the handler is returned +> **unbound** and the caller must gate on `Bridge::whenBound()` (or on the +> `onDone` of the async entry points) before issuing a call, exactly as the +> `*Async` path already required. + +The wait is morph#593's correction to the rule as morph#568 first wrote it. The +rule then read "bound exactly when `bindModel` settles inside the call", which +made *how the backend is implemented* decide what a synchronous, public entry +point returns — and `SocketBackend` (morph#569) changed its implementation +without any intent to change that. See +[Waiting for a bind — `bindWaitPolicy`](#waiting-for-a-bind--bindwaitpolicy). + +`executeVia` fails fast with `"handler not bound"` for a call issued before the +reply arrives; it does not queue. That is unchanged — it is the same failure the +`*Async` path produced, now reachable through one surface instead of two. + +Consequences, as of morph#568 and morph#593: + +| Backend | `bindModel` | `bindWaitPolicy()` | `registerHandler` returns | +|---|---|---|---| +| `LocalBackend`, `SimulatedRemoteBackend`, the eleven test doubles | default (blocking verb) | `kCallerMayBlock` (default) | bound; the wait finds the outcome already there | +| A backend wrapped in `SynchronousBackendAdapter` | non-blocking, on the adapter's executor | `kCallerMustNotBlock` | **unbound** | +| `QtWebSocketBackend`, `asyncRegistrationEnabled` unset | default (blocking verb) | `kCallerMayBlock` | bound | +| `QtWebSocketBackend`, `asyncRegistrationEnabled` set | native, non-blocking | `kCallerMustNotBlock` | **unbound** (the WASM case, which is the whole point) | +| `SocketBackend` (morph#569) | native, non-blocking | `kCallerMayBlock` (default) | bound, after waiting for the I/O thread's reply | + +### Waiting for a bind — `bindWaitPolicy` + +`Bridge::registerHandlerImpl` has exactly one call site for acquiring a model, +and after morph#568 and morph#569 two shipped backends needed opposite +behaviour from it: + +| Backend | What `bindModel` does | What the call site must do | Why | +|---|---|---|---| +| `SocketBackend` | returns an unsettled `Completion`; the I/O thread settles it | **wait** | its callers construct a `BridgeHandler` and use it on the next line; `executeVia` fails fast on `currentId == 0` | +| `QtWebSocketBackend`, `asyncRegistrationEnabled` set | returns an unsettled `Completion` | **not wait** | the reply is delivered by the Qt event loop of the calling thread, so a wait is a deadlock — on WASM, a page abort | + +From the `Completion` alone the two are indistinguishable, and the surface +morph#567 introduced had removed the only signal that told them apart (the +`*Async` verbs' `bool` return). Measured, not argued: on morph#568's head, six +`tests/net/` cases failed with `"handler not bound"`, and forcing the *other* +answer at that call site instead hung five `tests/qt/` cases on the nested +`QEventLoop`. Neither fixed setting of the call site is correct. + +`IBackend::bindWaitPolicy()` restores exactly one bit: + +- `BindWait::kCallerMayBlock` (the default) — the completion settles without + the calling thread's participation, either inside the call or on a thread the + caller does not own. A backend that answers this **must** settle every + `Completion` it hands out exactly once without any further call from the + caller, including on transport failure and on `cancelPending`/destruction. +- `BindWait::kCallerMustNotBlock` — waiting is either impossible (the reply + needs the caller's own event loop) or pointless (`SynchronousBackendAdapter`, + which exists to move the blocking elsewhere, and would deadlock if the caller + happened to be running on its executor). + +It is deliberately not the `bool` morph#567 removed. That `bool` chose *which +verb to call*, so every call site carried two paths and a backend could be +half-migrated; this one chooses nothing. There is still exactly one verb, called +unconditionally, and exactly one continuation — the only question is whether the +thread that registered that continuation is allowed to stop and wait for it. +Only a synchronous entry point that must return a bound instance asks: +`registerHandlerImpl` is the sole framework caller, and the asynchronous entry +points (`attachHandlerAsync`, `ensureBoundAsync`, `assignHandlerPrimary`) never +wait and never consult it. + +The wait is unbounded by design. A timeout would make "is this handler bound +when `registerHandler` returns" depend on how fast the network was, which is the +non-determinism the synchronous contract exists to exclude; a backend that +breaks its own settle-exactly-once contract therefore hangs here rather than +silently handing back an unbound handler. + +`SocketBackend`'s row above is the one morph#568 changed and morph#569 did not: +morph#569 landed while `Bridge` still called the legacy verbs, so its native +`bindModel` had no caller. morph#593 puts that row back to "bound" rather than +asking every existing non-Qt embedder to start gating on `whenBound()` — +`docs/spec/core/bridge.md`'s `whenBound()` section remains the right place for a +caller that wants to gate anyway, and is required for the two +`kCallerMustNotBlock` rows. ### Migration status -No *caller* uses this surface yet. `Bridge` still calls the five synchronous -verbs and prefers the four `*Async` twins, every existing implementor still -compiles unchanged, and the default `bindModel`/`promoteModel` implementations -route to exactly the legacy verb each request shape names — so a backend that -has overridden nothing behaves identically through either surface. On the -implementor side, `SocketBackend` overrides both natively (morph#569) without -changing any of its legacy verbs. The four twins are removed, and the prose -threading contract retired, in morph#571. +| Step | What it does | State | +|---|---|---| +| morph#567 | Adds `bindModel`/`promoteModel` and `SynchronousBackendAdapter`. Nothing else changes. | Landed | +| morph#568 | `QtWebSocketBackend` implements the surface natively and drops all four `*Async` overrides; `Bridge`'s four dispatch sites fall back to it instead of to a synchronous verb. | Landed | +| morph#569 | `SocketBackend` implements the surface natively, keeping every legacy verb on `sendSync`. | Landed | +| morph#593 | Adds `IBackend::bindWaitPolicy()`, the one signal morph#567's surface left the call site without. Fixes the `"handler not bound"` regression morph#568 caused in `SocketBackend`. | Landed | +| morph#570 | The example GUIs and the WASM spike; `Bridge::installReconnectHandler` onto `bindModel`. | Open | +| morph#571 | Removes the four `*Async` verbs; migrates `LocalBackend`, `SimulatedRemoteBackend` and the test doubles. | Open | + +Every existing implementor still compiles unchanged, and the default +`bindModel`/`promoteModel` implementations route to exactly the legacy verb each +request shape names — so a backend that has overridden nothing behaves +identically through either surface. What changed with morph#568 is the *caller*: +after it, every `Bridge` dispatch site has the shape + +```cpp +bool const started = backend->Async(..., onOk, onErr); // removed by morph#571 +if (!started) { + auto completion = backend->bindModel(request, exec::detail::inlineExecutor()); + completion.then(onOk).onError(onErr); +} +``` + +so the path count did not grow: the structural call replaced the synchronous +fallback that used to sit there, and morph#571 deletes the first branch to leave +one. No backend in the tree overrides a `*Async` verb any more, so the second +branch is the one that always runs; the eleven test doubles in +`tests/test_async_registration.cpp` that do override them still exercise the +first, which is what keeps that suite meaningful until morph#571 rewrites it. + +`Bridge::installReconnectHandler` is the one dispatch site morph#568 did **not** +move: its handler still calls the blocking `registerModelShared`/ +`registerModelWithContext`. That is deliberate — moving it changes `Bridge`'s +locking model, not just a call — and it is the reason `SocketBackend`'s +reconnect-handler deadlock hazard survives morph#568 exactly as +[its own section](#the-structural-registration-surface-natively) says it +survives morph#569. + +The executor those call sites name is **`exec::detail::inlineExecutor()`**, which +runs the continuation on the thread that settled it. That is deliberately the +*old* delivery thread, so morph#568 changes no observable *threading*: `Bridge` +owns no event loop and has no thread of its own to name. Making it name a real +one is the step that would turn the structural guarantee into a behaviour +change, and it belongs to whichever ticket gives `Bridge` such an executor — +see the note under [Threading contract](#threading-contract--the-callbacks-delivery-thread) +and morph#588. ## Error types @@ -1264,9 +1486,29 @@ by `_pendingMtx`, because `cancelPending` can be called from `Bridge` / `std::runtime_error("register failed: ")` (so a lost connection surfaces as `"register failed: disconnected"`) rather than propagating the raw error or hanging. The `factory` argument is ignored (model construction is delegated to - the server, as with all remote backends). `registerModelWithContext` is - **not** overridden — the default drops the `contextKey`, so this transport - does not carry a context key to the server's `LogProvider`. + the server, as with all remote backends). `registerModel` is a forward to + `registerModelWithContext` with an empty key, so there is one place that + builds this envelope rather than two. + +- `registerModelWithContext` — the same `register` round trip, carrying + `contextKey` on the envelope (`wire::makeRegister(typeId, contextKey)`). + Overridden since morph#594; before that it was `IBackend`'s default, which + drops the key. That is not a missing field: + `RemoteServer::attachLogIfConfigured` returns **before** consulting its + `LogProvider` when the envelope's `contextKey` is empty, so a privately + registered instance over this transport produced no action-log record at all + while `SimulatedRemoteBackend` and `SocketBackend` produced one. Three call + sites reached the dropping default: the blocking `bindModel`'s + empty-`primary`/zero-`current` branch (so, every private registration made + with `Config::asyncRegistrationEnabled` unset — its default), + `Bridge::switchBackend`'s re-registration after a reconnect (whatever that + flag was set to), and `registerModelShared`/`attachModel`'s own + empty-`primary` degradation. `bindModel`'s non-blocking path already carried + the key, which is what made the flag decide whether an instance was audited. + `tests/qt/test_qt_websocket.cpp`, "a private registration carries contextKey + to the server's log provider", pins all of it against a real + `QtWebSocketServer` and asserts on the provider rather than on the + registration succeeding — registration succeeded before the fix too. `sendSync` itself is hardened against a disconnect mid-call. Before parking the nested loop it checks `_connected` and throws `"disconnected"` up front if the @@ -1278,17 +1520,18 @@ by `_pendingMtx`, because `cancelPending` can be called from `Bridge` / the parked loop returns with an empty `_pendingReply`, `sendSync` throws `"disconnected"`. See concurrency_and_lifetimes.md. - **`registerModelAsync` is the non-blocking alternative**, opt-in via - `QtWebSocketBackendConfig::asyncRegistrationEnabled` (default `false`, so - every existing embedder keeps `registerModel`'s synchronous behavior - unchanged). A call made before the socket has finished connecting is queued + **`bindModel` is the non-blocking alternative**, opt-in via + `QtWebSocketBackendConfig::asyncRegistrationEnabled` (default `false`, which + makes `bindModel` run `IBackend`'s blocking default, so every existing + embedder keeps `registerModel`'s synchronous behavior unchanged). A private + bind made before the socket has finished connecting is queued (`_queuedRegistrations`) rather than failed, and flushed — each entry assigned a call-id and sent, in FIFO order — from the `connected` slot, the first connect included, before `_connectHandler`'s reconnect-handler counterpart runs. If the backend is destroyed (or the socket disconnects) before that queue is ever flushed, `cancelPending` drains it and still - invokes each entry's `onError` exactly once. See [Asynchronous - registration](#asynchronous-registration--registermodelasync). + rejects each entry's `Completion` exactly once. See [Backends with a + genuinely non-blocking path](#backends-with-a-genuinely-non-blocking-path). - `deregisterModel` — **fire-and-forget**, not synchronous: if `_connected`, it sends a `deregister` envelope and returns immediately without waiting for the ack; if disconnected, it does nothing. This deliberately avoids a nested @@ -1311,11 +1554,14 @@ frame and routes it by `callId`: `deserialize(body)` into the completion's value (deserialisation exceptions become the completion's error), any other kind → `std::runtime_error(message)` into the completion's error. If not found there, `_pendingRegistrations` is - checked next (an async `registerModelAsync` reply — same `callId` - counter/namespace as `execute`, separate map because the reply shape differs): - `ok` → `onRegistered(ModelId{modelId})`, any other kind → `onError(message)`. - A `callId` matching **neither** map (e.g. a late reply for an - already-cancelled call) is dropped silently. + checked next (a non-blocking `bindModel`/`promoteModel` reply — same `callId` + counter/namespace as `execute`, separate map because the reply shape differs; + one map for both verbs, because a `register`, a shared `register`, an `attach` + and an `assign` reply are all matched identically): `ok` → + `resolve(ModelId{modelId})`, any other kind → `reject(runtime_error(message))`, + both **outside** `_pendingMtx`, since settling runs the caller's continuation + and `Bridge`'s executor runs it inline. A `callId` matching **neither** map + (e.g. a late reply for an already-cancelled call) is dropped silently. - A **`callId == 0`** frame is a synchronous control reply (`register` when `asyncRegistrationEnabled` is `false`, or `attach`/`assign`/`instances`); it is stored in `_pendingReply` and quits the parked nested `QEventLoop`. @@ -1954,6 +2200,7 @@ inside the class calls `close()` — no thread it joins can be waiting on it. | `registerModelWithContext` | `virtual ModelId registerModelWithContext(const string&, function()>, string_view)` | Default: drops `contextKey`, calls `registerModel`. | | `bindModel` | `virtual Completion bindModel(BindRequest, IExecutor& cbExec)` | Default: runs `bindModelBlocking` inline and settles. See [The structural registration surface](#the-structural-registration-surface--bindmodel-and-promotemodel). | | `promoteModel` | `virtual Completion promoteModel(PromoteRequest, IExecutor& cbExec)` | Default: calls `assignPrimary` inline and settles with `request.mid`. | +| `bindWaitPolicy` | `virtual BindWait bindWaitPolicy() const noexcept` | Default: `BindWait::kCallerMayBlock`. Whether a caller may block until a `bindModel`/`promoteModel` completion settles. Read only by `Bridge::registerHandlerImpl`. | | `bindModelBlocking` | `ModelId bindModelBlocking(BindRequest)` | Non-virtual. Routes a `BindRequest` to `registerModelWithContext` / `registerModelShared` / `attachModel` by its shape; blocks. Shared by the default `bindModel` and by `SynchronousBackendAdapter`. | | `deregisterModel` | `virtual void deregisterModel(ModelId)` | Pure virtual. | | `execute` | `virtual Completion> execute(ModelId, ActionCall, IExecutor*)` | Pure virtual. | @@ -1977,6 +2224,13 @@ inside the class calls `close()` — no thread it joins can be waiting on it. | `PromoteRequest::typeId` | `std::string` | Model type id — the directory's first key component. | | `PromoteRequest::primary` | `std::string` | Key to file `mid` under. | +### `detail::BindWait` + +| Enumerator | Meaning | +|---|---| +| `kCallerMayBlock` | Default. The completion settles without the calling thread's participation; a caller may wait for it. Implies the backend settles every `Completion` exactly once, unprompted. | +| `kCallerMustNotBlock` | Waiting is impossible (the reply needs the caller's own event loop) or pointless (`SynchronousBackendAdapter`). The caller registers its continuation and returns. | + ### `SynchronousBackendAdapter` | Method | Notes | @@ -1984,6 +2238,7 @@ inside the class calls `close()` — no thread it joins can be waiting on it. | `SynchronousBackendAdapter(shared_ptr inner, IExecutor& blockingExec)` | Throws `std::invalid_argument` if `inner` is null. `blockingExec` is `MORPH_LIFETIMEBOUND` and must keep running tasks until the destructor's wait completes. | | `wrapped()` | The wrapped backend; never null. | | `bindModel(request, cbExec)` | Posts `inner->bindModelBlocking(request)` onto the control strand; settles the returned `Completion` on `cbExec`. Never blocks the caller. | +| `bindWaitPolicy()` | `BindWait::kCallerMustNotBlock`, always. Not forwarded: it describes the two verbs the adapter reshapes. | | `promoteModel(request, cbExec)` | Posts `inner->assignPrimary(...)` onto the control strand; resolves with `request.mid`. | | every other `IBackend` verb | Forwarded to `inner` unchanged, including the four `*Async` twins — wrapping a backend that has a non-blocking path must not take it away. | @@ -2049,7 +2304,7 @@ inside the class calls `close()` — no thread it joins can be waiting on it. | `initialReconnectDelay` | `std::chrono::milliseconds` | `500 ms` | | `maxReconnectDelay` | `std::chrono::milliseconds` | `30 s` | | `backoffMultiplier` | `double` | `2.0` | -| `asyncRegistrationEnabled` | `bool` | `false` — opts in to `registerModelAsync` (see [Asynchronous registration](#asynchronous-registration--registermodelasync)); `false` keeps every embedder on `registerModel`'s synchronous behavior. | +| `asyncRegistrationEnabled` | `bool` | `false` — whether `bindModel` may return before the reply. `false` defers to `IBackend::bindModel`, which blocks the Qt thread in a nested `QEventLoop`, keeping every existing embedder's behaviour. `true` is the WASM setting. Not an opt-in to a second set of verbs any more: the continuation exists either way. | ### `QtWebSocketBackend` (namespace `morph::qt`) @@ -2058,14 +2313,17 @@ inside the class calls `close()` — no thread it joins can be waiting on it. | `QtWebSocketBackend(serverUrl, dispatcher = defaultDispatcher(), registry = defaultRegistry(), tls = nullopt, cfg = Config{})` | Opens the socket to `serverUrl` in the constructor. `dispatcher`/`registry` params are accepted but unused (models live on the server). `tls` non-null → `wss://`. `tls` is not declared at all when Qt is built with `QT_NO_SSL` (see above). | | `QtWebSocketBackend(serverUrl, tls, cfg = Config{})` | Overload that skips the unused `dispatcher`/`registry` pair (issue #55): a caller who only needs `tls`/`cfg` reaches them directly, without naming `morph::model::detail::defaultDispatcher()`/`defaultRegistry()` explicitly. Delegates to the main constructor with both defaulted. Not declared on a `QT_NO_SSL` build (no `tls` parameter to distinguish it from the `(serverUrl, cfg)` overload below). | | `QtWebSocketBackend(serverUrl, cfg)` | Overload that skips `dispatcher`/`registry` and `tls` together — the common case for a caller that only wants to set a `Config` field (e.g. `asyncRegistrationEnabled`) over a plaintext `ws://` connection. Delegates to the main constructor with `dispatcher`/`registry` defaulted and (on an SSL-enabled build) `tls = std::nullopt`. | -| `registerModelAsync(typeId, factory, contextKey, onRegistered, onError)` | Returns `false` immediately unless `cfg.asyncRegistrationEnabled` is `true`. Otherwise: assigns a fresh `callId` (the same counter `execute` uses), records the callbacks in `_pendingRegistrations[callId]`, sends `register` with that `callId`, and returns `true`. `onRegistered`/`onError` fire later from `onTextMessage` (or from `cancelPending` on a disconnect) — never synchronously from this call. | +| `bindModel(request, cbExec)` | Defers to `IBackend::bindModel` (blocking) unless `cfg.asyncRegistrationEnabled` is `true`. Otherwise builds the envelope `request`'s shape names — `register`, shared `register`, or `attach` — assigns a fresh `callId` (the same counter `execute` uses), records the promise in `_pendingRegistrations[callId]` and sends. The `Completion` settles later from `onTextMessage` (or from `cancelPending` on a disconnect). A private bind on an unconnected socket is queued in `_queuedRegistrations` instead; a keyed one rejects with `"disconnected"`. | +| `bindWaitPolicy()` | `kCallerMustNotBlock` exactly when `cfg.asyncRegistrationEnabled` is set — that is when completions are settled by `onTextMessage`, a Qt slot delivered by the calling thread's own event loop, so a caller that blocked waiting for one would never see it arrive. `kCallerMayBlock` otherwise, where `bindModel` settles inside the call. | +| `promoteModel(request, cbExec)` | Always non-blocking, with no `Config` gate — `assignPrimary`'s caller is inside a `Completion` chain, so there is no synchronous guarantee to preserve. Sends `assign` through the same path. An empty `primary` or zero `mid` resolves with `request.mid` without sending. | | `waitForConnected(timeoutMs = 5000)` | Pumps the Qt loop until connected or timeout; returns `_connected`. | | `negotiateProtocolVersion()` | Opt-in: sends `hello` synchronously (same nested-`QEventLoop` path as `registerModel`), classifies the reply via `wire::interpretHelloReply`. Throws on an explicit version rejection or a `sendSync` failure. | -| `registerModel(typeId, factory)` | Synchronous via nested `QEventLoop`; `factory` ignored. Throws on `err` reply. | +| `registerModel(typeId, factory)` | Forwards to `registerModelWithContext` with an empty key. | +| `registerModelWithContext(typeId, factory, contextKey)` | Synchronous via nested `QEventLoop`; `factory` ignored. Sends `register` carrying `contextKey`, so a privately registered instance is journalled (morph#594). Throws on `err` reply, and wraps a `sendSync` failure as `"register failed: "`. | | `deregisterModel(mid)` | **Fire-and-forget** — sends only if connected, does not wait for the ack. Carries a non-zero `callId` from the same counter `execute` uses, recorded in `_pendingDeregisters` so `onTextMessage` recognises the unwanted reply and drops it rather than handing it to a parked `sendSync` (issue #65). | | `execute(mid, call, cbExec)` | Assigns a `callId`, sends `execute`, returns a `Completion`. Immediate `DisconnectedError` if not connected. | | `notifyBackendChanged()` | No-op. | -| `cancelPending(exc)` | Drains `_pending` under `_pendingMtx`, delivers `exc` to each state. | +| `cancelPending(exc)` | Drains `_pending`, `_pendingRegistrations` and `_queuedRegistrations` under `_pendingMtx`, then delivers `exc` to each — the exception itself, so a control call rejected by a dropped socket carries the same `DisconnectedError` an `execute` does. | | `setReconnectHandler(handler)` | Stores the handler; invoked on the Qt thread after every *subsequent* connect. `nullptr` clears. | | `setConnectHandler(handler)` | Stores the handler; invoked on the Qt thread after every successful connect, first included. `nullptr` clears. | | `setDisconnectHandler(handler)` | Stores the handler; invoked on the Qt thread whenever the socket drops, before reconnect scheduling. `nullptr` clears. | diff --git a/include/morph/core/backend.hpp b/include/morph/core/backend.hpp index 5eaf4a176..d2d3a485a 100644 --- a/include/morph/core/backend.hpp +++ b/include/morph/core/backend.hpp @@ -6,6 +6,7 @@ #include #include #include +#include #include #include #include @@ -133,6 +134,55 @@ struct PromoteRequest { std::string primary; }; +/// @brief Whether a caller holding an unsettled `bindModel`/`promoteModel` +/// `Completion` may block its own thread until that `Completion` +/// settles. +/// +/// This is the one thing `bindModel`'s signature cannot say, and morph#593 is +/// what happens when it is not said: two shipped backends both return an +/// unsettled `Completion` from `bindModel`, and `Bridge::registerHandlerImpl` +/// — a synchronous entry point that hands its caller a `BridgeHandler` usable +/// on the next line — must wait for one of them and must not wait for the +/// other. From the `Completion` alone the two are indistinguishable. +/// +/// It is deliberately **not** the `bool` the surface removed (see "The +/// structural registration surface" below, point 1). That `bool` chose +/// *which verb to call*, so every call site carried two paths and a backend +/// could be half-migrated. This one chooses nothing: there is still exactly +/// one verb, called unconditionally, and exactly one continuation. It says +/// only whether the thread that issued the call is allowed to stop and wait +/// for the continuation it already registered. +enum class BindWait : std::uint8_t { + /// @brief The completion settles without the calling thread's + /// participation — inside the call, or on a thread the caller does + /// not own. A caller may block until it settles. + /// + /// The default, and correct for every backend that has not overridden + /// `bindModel` (the default settles before it returns) as well as for + /// `SocketBackend`, whose I/O thread settles the completion. A backend + /// that returns this **must** settle every `Completion` it hands out + /// exactly once without further calls from the caller — including on + /// transport failure and on `cancelPending`/destruction — or a caller that + /// waits will wait forever. + kCallerMayBlock, + + /// @brief The completion cannot settle while the calling thread is blocked + /// in a wait, or blocking it would defeat the point of this + /// backend. A caller must register its continuation and return. + /// + /// Two backends say this, for two different reasons: + /// + /// - `QtWebSocketBackend` with `Config::asyncRegistrationEnabled` set: the + /// reply arrives through the Qt event loop of the thread that issued the + /// call, so waiting is a deadlock. On a WASM main thread it aborts the + /// page (morph#568), which is the case that surface exists for. + /// - `SynchronousBackendAdapter`: it exists precisely to move a blocking + /// call off the caller's thread, so a caller that then waits for it has + /// bought nothing — and if the caller happens to be running on the + /// adapter's own executor, has deadlocked. + kCallerMustNotBlock, +}; + /// @brief Abstract interface for execution backends (local, remote, …). /// /// A backend owns model instances and dispatches actions against them. @@ -494,6 +544,18 @@ struct IBackend { // implementations below, or without blocking the caller at all // through `SynchronousBackendAdapter`. // + // What morph#593 established is that removing *that* bool also + // removed something else the call site needed and that is not the + // same question: whether the thread that called `bindModel` is + // allowed to wait for the continuation it just registered. Two + // shipped backends return an unsettled `Completion` and give opposite + // answers (`SocketBackend`: yes, its I/O thread settles it; + // `QtWebSocketBackend` under `asyncRegistrationEnabled`: no, waiting + // deadlocks the event loop the reply arrives on). `bindWaitPolicy()` + // below restores exactly that one bit and nothing else — it never + // selects a verb, so the "second path" the removed bool created does + // not come back with it. + // // 2. **The delivery thread is a parameter.** `Completion` posts its // handlers to the executor it was built with, so the continuation runs // where @p cbExec says and nowhere else — the backend does not choose. @@ -579,6 +641,24 @@ struct IBackend { } // NOLINTEND(performance-unnecessary-value-param) + /// @brief Whether a caller may block until this backend's + /// `bindModel`/`promoteModel` completions settle. + /// + /// Answers the one question `Completion` cannot: an unsettled `Completion` + /// looks the same whether the reply is coming from a thread the caller does + /// not own or from the caller's own event loop. Only a synchronous entry + /// point that must hand back a *bound* instance asks it — + /// `Bridge::registerHandlerImpl` is the single caller in the framework; the + /// asynchronous entry points never wait and never consult it. + /// + /// A backend that returns `kCallerMayBlock` (the default) commits to + /// settling every `Completion` it returns exactly once without any further + /// call from the caller. Every backend that does not override `bindModel` + /// satisfies that trivially: the default settles before it returns. + /// + /// @return `BindWait::kCallerMayBlock` unless overridden. + [[nodiscard]] virtual BindWait bindWaitPolicy() const noexcept { return BindWait::kCallerMayBlock; } + /// @brief Runs @p request against the legacy synchronous verbs, blocking. /// /// Factored out of `bindModel`'s default implementation so that @@ -885,6 +965,20 @@ class SynchronousBackendAdapter : public detail::IBackend { }); } + /// @brief This adapter's whole purpose is that the caller does not block. + /// + /// Not forwarded to the wrapped backend, unlike everything below: + /// `bindModel`/`promoteModel` are the two verbs this adapter *reshapes*, so + /// the policy describing them describes this adapter, not what it wraps. A + /// caller that waited would pay exactly the blocking cost the adapter was + /// interposed to move elsewhere, and — if it happens to be running on + /// `blockingExec` — would deadlock against the strand it is waiting on. + /// + /// @return `BindWait::kCallerMustNotBlock`, always. + [[nodiscard]] detail::BindWait bindWaitPolicy() const noexcept override { + return detail::BindWait::kCallerMustNotBlock; + } + // ── Everything else is forwarded unchanged ─────────────────────────── // // A decorator has to forward every verb it does not reshape, including the diff --git a/include/morph/core/bridge.hpp b/include/morph/core/bridge.hpp index 7eac63cf3..518dade79 100644 --- a/include/morph/core/bridge.hpp +++ b/include/morph/core/bridge.hpp @@ -6,6 +6,7 @@ #include #include #include +#include #include #include #include @@ -207,12 +208,12 @@ struct HandlerBinding { /// @brief Registration-settled seam (see `Bridge::whenBound`). /// /// `registrationInFlight` is `true` from just *before* `registerHandlerImpl` - /// calls `IBackend::registerModelAsync` until that call's - /// `onRegistered`/`onError` callback resolves. It is set unconditionally on - /// every path, the synchronous fallback included — see the comment at the - /// assignment for why it must be set before the backend call rather than - /// after. The fallback does not *leave* it set: it resolves the waiters - /// (and clears the flag) before returning. `whenBound()` + /// calls the backend until that call's continuation resolves. It is set + /// unconditionally on every path, the blocking one included — see the + /// comment at the assignment for why it must be set before the backend call + /// rather than after. A backend that settles inside the dispatch frame does + /// not *leave* it set: the continuation runs before `registerHandlerImpl` + /// returns and resolves the waiters (clearing the flag). `whenBound()` /// checks it to distinguish "an async reply is coming, queue a waiter" /// from "nothing is in flight, resolve false now". `registrationWaiters` /// holds callbacks queued by `whenBound()` while `registrationInFlight` is @@ -227,6 +228,28 @@ struct HandlerBinding { std::vector, std::function>> registrationWaiters; }; +/// @brief Renders @p failure as the diagnostic string a log line wants. +/// +/// The `Completion` surface carries an `exception_ptr`; the one remaining place +/// that needs text rather than a rethrowable failure is a log message. Kept +/// here rather than inline so "what does a rejected control call read like" +/// has one answer. +/// +/// @param failure Rejection to describe; may be null. +/// @return `what()` for a `std::exception`, or a generic stand-in otherwise. +inline std::string describeFailure(const std::exception_ptr& failure) { + if (!failure) { + return "unknown error"; + } + try { + std::rethrow_exception(failure); + } catch (const std::exception& exc) { + return exc.what(); + } catch (...) { + return "unknown error"; + } +} + /// @brief Outcome a backend's inline completion parked for its dispatcher. struct ParkedOutcome { /// @brief `true` when the parked outcome is a success. @@ -256,6 +279,12 @@ struct ParkedOutcome { struct AsyncDispatchHandoff { /// @brief Guards every other field; never held across `onDone` or `_attachMtx`. std::mutex mtx; + /// @brief Signalled once `fired` is set, for `awaitHandoff`'s benefit. + /// + /// Only a dispatcher that deliberately waits (`awaitHandoff`) ever blocks on + /// this; the three asynchronous dispatch sites never do, so for them the + /// `notify_all` in `parkIfInFrame` is an uncontended no-op. + std::condition_variable settled; /// @brief `true` while the backend's dispatch call is still on the caller's stack. bool inFrame = true; /// @brief Set once either callback has claimed the outcome. @@ -283,17 +312,25 @@ struct AsyncDispatchHandoff { /// `false` if the caller owns the outcome and should deliver it itself. inline bool parkIfInFrame(AsyncDispatchHandoff& handoff, bool succeeded, ::morph::exec::detail::ModelId modelId, std::exception_ptr failure) { - std::scoped_lock const guard{handoff.mtx}; - if (handoff.fired) { - // A backend is contractually allowed exactly one callback per dispatch; - // swallow a second one rather than reporting twice. - return true; + bool inFrame = false; + { + std::scoped_lock const guard{handoff.mtx}; + if (handoff.fired) { + // A backend is contractually allowed exactly one callback per dispatch; + // swallow a second one rather than reporting twice. + return true; + } + handoff.fired = true; + handoff.succeeded = succeeded; + handoff.modelId = modelId; + handoff.failure = std::move(failure); + inFrame = handoff.inFrame; } - handoff.fired = true; - handoff.succeeded = succeeded; - handoff.modelId = modelId; - handoff.failure = std::move(failure); - return handoff.inFrame; + // Outside the lock: a waiter in `awaitHandoff` re-acquires `mtx` the moment + // it wakes, and notifying while still holding it makes it wake only to block + // again. + handoff.settled.notify_all(); + return inFrame; } /// @brief Closes the inline window and takes whatever a callback parked. @@ -314,6 +351,35 @@ inline std::optional claimHandoff(AsyncDispatchHandoff& handoff) return ParkedOutcome{.succeeded = handoff.succeeded, .modelId = handoff.modelId, .failure = handoff.failure}; } +/// @brief Holds the inline window open until a callback parks an outcome in it, +/// then closes it and takes that outcome. +/// +/// `claimHandoff`'s blocking twin, and the whole of what +/// `BindWait::kCallerMayBlock` buys: the dispatching frame stops and waits, so a +/// reply that lands on the backend's own thread is still delivered *by this +/// frame*, on this thread, before the dispatching call returns — which is +/// exactly what the frame does for a backend that settled inline. The outcome +/// therefore takes the same path in both cases (published here, rethrown here), +/// instead of one path for "settled in frame" and another for "settled later". +/// +/// Only ever called on a backend that returned `BindWait::kCallerMayBlock`, +/// whose contract is that the completion settles without this thread doing +/// anything. There is deliberately no timeout: a bounded wait would make +/// "is the handler bound when `registerHandler` returns" depend on how fast the +/// network was, which is the non-determinism the synchronous contract exists to +/// exclude. A backend that violates its own settle-exactly-once contract hangs +/// here rather than silently returning an unbound handler, and a hang is the +/// diagnosable failure of those two. +/// +/// @param handoff Handoff slot created by the dispatching frame. +/// @return The outcome a callback parked; never `std::nullopt`. +inline std::optional awaitHandoff(AsyncDispatchHandoff& handoff) { + std::unique_lock guard{handoff.mtx}; + handoff.settled.wait(guard, [&handoff] { return handoff.fired; }); + handoff.inFrame = false; + return ParkedOutcome{.succeeded = handoff.succeeded, .modelId = handoff.modelId, .failure = handoff.failure}; +} + /// @brief The gate that makes "the `Bridge` is still there" and "call into it" /// a single, indivisible step. /// @@ -510,36 +576,39 @@ class Bridge { binding->currentId.store(newId.v); } - /// @brief Async counterpart to `attachHandler`: prefers the backend's - /// `attachModelAsync` when available, invoking @p onDone once - /// attached (or failed) instead of blocking. + /// @brief Async counterpart to `attachHandler`: dispatches the attach and + /// invokes @p onDone once attached (or failed), instead of blocking. /// - /// Falls back to the synchronous `attachHandler` body (and calls @p onDone - /// immediately, from this thread) when the backend offers no async - /// path — so a caller that always goes through this method behaves - /// identically to calling `attachHandler` directly, on every backend - /// that has not opted in to `attachModelAsync`. + /// Reaches the backend through `IBackend::bindModel` — the structural + /// registration surface — unless the backend still overrides the legacy + /// `attachModelAsync`, which is offered first and which morph#571 removes. + /// Either way there is exactly one dispatch and exactly one continuation: a + /// backend with no non-blocking attach settles inside the `bindModel` call, + /// having blocked for the same round trip the synchronous `attachHandler` + /// would have, and @p onDone is then invoked from this thread before this + /// method returns — so a caller that always goes through this method + /// behaves identically to calling `attachHandler` directly on such a + /// backend. /// /// @par Locking - /// `_attachMtx` is held around the guard check, the async branch's - /// *dispatch*, and the synchronous branch's own state mutation — matching + /// `_attachMtx` is held around the guard check and the *dispatch* — matching /// `attachHandler`'s existing lock scope — but is **released before /// @p onDone is ever invoked**, on every path, unconditionally. That is not /// a nicety: what `execute()` does from inside @p onDone is dispatch the /// action, and a result-keyed dispatch promotes its binding through /// `assignHandlerPrimary`, which takes `_attachMtx` itself. Invoking /// @p onDone under the lock therefore self-deadlocks the moment the - /// completion is delivered on the calling thread — which is exactly what - /// the synchronous fallback below does, and what an inline executor does - /// for every callback. This is `registerHandlerImpl`'s existing rule ("the - /// backend call must not run under `_mtx`") applied to `_attachMtx`. - /// - /// The guarantee holds even for a backend that completes its callback - /// *inline*, from inside `attachModelAsync` itself, while this frame still - /// holds the lock: such a callback parks its outcome in a - /// `detail::AsyncDispatchHandoff` and returns without acting, and this frame - /// applies it after the dispatch call has returned and the lock is gone. - /// See that struct's doc comment. + /// completion is delivered on the calling thread — which is exactly what a + /// blocking backend's `bindModel` does, since the executor this call site + /// names runs the continuation inline. This is `registerHandlerImpl`'s + /// existing rule ("the backend call must not run under `_mtx`") applied to + /// `_attachMtx`. + /// + /// The guarantee holds even for a backend that settles *inline*, from inside + /// the dispatch call itself, while this frame still holds the lock: such a + /// continuation parks its outcome in a `detail::AsyncDispatchHandoff` and + /// returns without acting, and this frame applies it after the dispatch call + /// has returned and the lock is gone. See that struct's doc comment. /// /// An out-of-frame success callback re-acquires `_attachMtx` for the two /// `std::string` fields it publishes (`contextKey`/`primary`, which @@ -581,83 +650,109 @@ class Bridge { } auto const previous = ::morph::exec::detail::ModelId{binding->currentId.load()}; auto backend = loadBackend(); - auto primaryCopy = primary; + auto primaryCopy = std::move(primary); std::weak_ptr<::morph::backend::detail::IBackend> const weakBackend{backend}; auto const weakLiveness = _callbacks.token(); std::weak_ptr const weakBinding{binding}; auto handoff = std::make_shared(); - bool started = false; + auto onAttached = [this, weakBackend, weakLiveness, weakBinding, primaryCopy, onDone, + handoff](::morph::exec::detail::ModelId newId) { + if (detail::parkIfInFrame(*handoff, true, newId, nullptr)) { + return; // Completed inline: the dispatching frame will finish this. + } + // This check and the `this` touch below it are two steps -- the + // morph#486 shape. Closed not by a gate but by + // `IBackend::registerModelAsync`'s threading contract (see its + // doc comment): an overriding backend must deliver `*Async` + // replies on a thread that cannot run `~Bridge` concurrently. + // Gating instead would block `~Bridge` behind `_attachMtx`, + // which `attachHandler` holds across a full `attachModel` round + // trip. See morph#489. + if (!weakLiveness.active()) { + return; // The Bridge is gone; publishing this id would be pointless. + } + auto strongBinding = weakBinding.lock(); + if (!strongBinding) { + return; // The BridgeHandler (and its binding) is gone. + } + std::exception_ptr failure; + { + // contextKey/primary are plain std::strings that every + // other site reads under `_attachMtx`; publishing them + // without it would be a data race, not just a stale + // read. (`registerHandlerImpl`'s read during + // registration is the one documented carve-out -- see + // its own comment, and morph#505.) + std::scoped_lock const guard{_attachMtx}; + auto pinned = weakBackend.lock(); + if (!pinned || pinned != loadBackend()) { + // A switchBackend() already moved past this attach + // (see registerHandlerImpl's identical guard) and + // its own re-registration loop already handled + // `binding` on the *new* backend -- applying this + // stale reply now would overwrite that with a + // dangling id from a backend nothing uses any + // more. Unlike registerHandlerImpl's fire-and- + // forget re-registration, a real execute() call is + // synchronously waiting on `onDone` here, so the + // stale reply must still be reported -- silently + // dropping it would hang that caller forever. + failure = std::make_exception_ptr( + std::runtime_error("attach reply arrived from a backend switchBackend() already replaced")); + } else { + try { + strongBinding->contextKey = primaryCopy; + strongBinding->primary = primaryCopy; + strongBinding->currentId.store(newId.v); + } catch (...) { + failure = std::current_exception(); + } + } + } + onDone(failure); // Outside the lock -- see @par Locking. + }; + auto onFailed = [onDone, handoff](const std::exception_ptr& failure) { + if (detail::parkIfInFrame(*handoff, false, {}, failure)) { + return; + } + onDone(failure); + }; try { - started = backend->attachModelAsync( + bool const started = backend->attachModelAsync( binding->typeId, binding->modelFactory, {.contextKey = primaryCopy, .primary = primaryCopy}, previous, - [this, weakBackend, weakLiveness, weakBinding, primaryCopy, onDone, - handoff](::morph::exec::detail::ModelId newId) { - if (detail::parkIfInFrame(*handoff, true, newId, nullptr)) { - return; // Completed inline: the dispatching frame will finish this. - } - // This check and the `this` touch below it are two steps -- the - // morph#486 shape. Closed not by a gate but by - // `IBackend::registerModelAsync`'s threading contract (see its - // doc comment): an overriding backend must deliver `*Async` - // replies on a thread that cannot run `~Bridge` concurrently. - // Gating instead would block `~Bridge` behind `_attachMtx`, - // which `attachHandler` holds across a full `attachModel` round - // trip. See morph#489. - if (!weakLiveness.active()) { - return; // The Bridge is gone; publishing this id would be pointless. - } - auto strongBinding = weakBinding.lock(); - if (!strongBinding) { - return; // The BridgeHandler (and its binding) is gone. - } - std::exception_ptr failure; - { - // contextKey/primary are plain std::strings that every - // other site reads under `_attachMtx`; publishing them - // without it would be a data race, not just a stale - // read. (`registerHandlerImpl`'s read during - // registration is the one documented carve-out -- see - // its own comment, and morph#505.) - std::scoped_lock const guard{_attachMtx}; - auto pinned = weakBackend.lock(); - if (!pinned || pinned != loadBackend()) { - // A switchBackend() already moved past this attach - // (see registerHandlerImpl's identical guard) and - // its own re-registration loop already handled - // `binding` on the *new* backend -- applying this - // stale reply now would overwrite that with a - // dangling id from a backend nothing uses any - // more. Unlike registerHandlerImpl's fire-and- - // forget re-registration, a real execute() call is - // synchronously waiting on `onDone` here, so the - // stale reply must still be reported -- silently - // dropping it would hang that caller forever. - failure = std::make_exception_ptr(std::runtime_error( - "attach reply arrived from a backend switchBackend() already replaced")); - } else { - try { - strongBinding->contextKey = primaryCopy; - strongBinding->primary = primaryCopy; - strongBinding->currentId.store(newId.v); - } catch (...) { - failure = std::current_exception(); - } - } - } - onDone(failure); // Outside the lock -- see @par Locking. - }, - [onDone, handoff](const std::string& message) { - auto failure = std::make_exception_ptr(std::runtime_error(message)); - if (detail::parkIfInFrame(*handoff, false, {}, failure)) { - return; - } - onDone(failure); + onAttached, [onFailed](const std::string& message) { + onFailed(std::make_exception_ptr(std::runtime_error(message))); }); + if (!started) { + // The structural surface (`IBackend::bindModel`). A backend + // with a genuinely non-blocking attach settles the returned + // `Completion` when its reply lands; one without settles it + // from inside this call, having blocked exactly as the + // synchronous `attachModel` this replaces did. Either way the + // continuation exists, so there is no third path below. + // + // `inlineExecutor()` because that is where the continuation ran + // before: on whichever thread the backend settled the reply on + // (`IBackend::registerModelAsync`'s prose contract). What is + // new is that this call site *names* it; see + // `exec::detail::InlineExecutor`. An inline settle therefore + // reaches `onAttached`/`onFailed` while `_attachMtx` is still + // held, which is precisely the case `handoff` exists for. + auto completion = + backend->bindModel(::morph::backend::detail::BindRequest{.typeId = binding->typeId, + .factory = binding->modelFactory, + .contextKey = primaryCopy, + .primary = primaryCopy, + .current = previous}, + ::morph::exec::detail::inlineExecutor()); + completion.then(onAttached).onError(onFailed); + } } catch (...) { - // The backend's own dispatch call can throw synchronously (e.g. - // QtWebSocketBackend::attachModelAsync's wire::encode() failing - // before send) -- report it like any other failure instead of - // letting it escape execute()'s documented never-throws contract. + // A backend's own dispatch call can throw synchronously (e.g. a + // `wire::encode()` failing before send) -- report it like any other + // failure instead of letting it escape execute()'s documented + // never-throws contract. `bindModel` rejects rather than throws, so + // this now guards only the legacy `attachModelAsync` branch. lock.unlock(); onDone(std::current_exception()); return; @@ -682,24 +777,8 @@ class Bridge { onDone(failure); return; } - if (started) { - return; - } - // No async path on this backend: run the identical synchronous attach - // `attachHandler` would have run, under the same lock, then report the - // outcome only once the lock is gone (see @par Locking above). - std::exception_ptr failure; - try { - auto newId = backend->attachModel(binding->typeId, binding->modelFactory, - {.contextKey = primary, .primary = primary}, previous); - binding->contextKey = primary; - binding->primary = std::move(primary); - binding->currentId.store(newId.v); - } catch (...) { - failure = std::current_exception(); - } - lock.unlock(); - onDone(failure); + // Nothing parked: the reply is still to come, and `onAttached`/ + // `onFailed` will deliver it themselves once it does. } /// @brief Gives @p binding an anonymous instance if it does not have one yet. @@ -746,60 +825,77 @@ class Bridge { auto const weakLiveness = _callbacks.token(); std::weak_ptr const weakBinding{binding}; auto handoff = std::make_shared(); - bool started = false; + auto onBound = [this, weakBackend, weakLiveness, weakBinding, onDone, + handoff](::morph::exec::detail::ModelId newId) { + if (detail::parkIfInFrame(*handoff, true, newId, nullptr)) { + return; // Completed inline: the dispatching frame will finish this. + } + // This check and the `this` touch below it are two steps -- the + // morph#486 shape. Closed not by a gate but by delivering on a + // thread that cannot run `~Bridge` concurrently: the legacy + // `*Async` branch below leaves that to the backend (see + // `IBackend::registerModelAsync`'s doc comment), the `bindModel` + // branch names it as an executor. Gating instead would block + // `~Bridge` behind `_attachMtx`, which `attachHandler` holds + // across a full `attachModel` round trip. See morph#489. + if (!weakLiveness.active()) { + return; // The Bridge is gone; publishing this id would be pointless. + } + auto strongBinding = weakBinding.lock(); + if (!strongBinding) { + return; // The BridgeHandler (and its binding) is gone. + } + std::exception_ptr failure; + { + // Brief `_attachMtx` window purely to serialise this + // check against a concurrent switchBackend() (which + // takes the same lock) -- `currentId` itself is an + // atomic and needs no lock to store. + std::scoped_lock const guard{_attachMtx}; + auto pinned = weakBackend.lock(); + if (!pinned || pinned != loadBackend()) { + // See attachHandlerAsync's identical guard: a + // stale reply from a backend switchBackend() + // already replaced must still resolve `onDone` + // (a real execute() call is waiting), not be + // silently dropped. + failure = std::make_exception_ptr( + std::runtime_error("attach reply arrived from a backend switchBackend() already replaced")); + } else { + strongBinding->currentId.store(newId.v); + } + } + onDone(failure); + }; + auto onFailed = [onDone, handoff](const std::exception_ptr& failure) { + if (detail::parkIfInFrame(*handoff, false, {}, failure)) { + return; + } + onDone(failure); + }; try { - started = backend->registerModelSharedAsync( - binding->typeId, binding->modelFactory, {.contextKey = binding->contextKey, .primary = {}}, - [this, weakBackend, weakLiveness, weakBinding, onDone, handoff](::morph::exec::detail::ModelId newId) { - if (detail::parkIfInFrame(*handoff, true, newId, nullptr)) { - return; // Completed inline: the dispatching frame will finish this. - } - // This check and the `this` touch below it are two steps -- the - // morph#486 shape. Closed not by a gate but by - // `IBackend::registerModelAsync`'s threading contract (see its - // doc comment): an overriding backend must deliver `*Async` - // replies on a thread that cannot run `~Bridge` concurrently. - // Gating instead would block `~Bridge` behind `_attachMtx`, - // which `attachHandler` holds across a full `attachModel` round - // trip. See morph#489. - if (!weakLiveness.active()) { - return; // The Bridge is gone; publishing this id would be pointless. - } - auto strongBinding = weakBinding.lock(); - if (!strongBinding) { - return; // The BridgeHandler (and its binding) is gone. - } - std::exception_ptr failure; - { - // Brief `_attachMtx` window purely to serialise this - // check against a concurrent switchBackend() (which - // takes the same lock) -- `currentId` itself is an - // atomic and needs no lock to store. - std::scoped_lock const guard{_attachMtx}; - auto pinned = weakBackend.lock(); - if (!pinned || pinned != loadBackend()) { - // See attachHandlerAsync's identical guard: a - // stale reply from a backend switchBackend() - // already replaced must still resolve `onDone` - // (a real execute() call is waiting), not be - // silently dropped. - failure = std::make_exception_ptr(std::runtime_error( - "attach reply arrived from a backend switchBackend() already replaced")); - } else { - strongBinding->currentId.store(newId.v); - } - } - onDone(failure); - }, - [onDone, handoff](const std::string& message) { - auto failure = std::make_exception_ptr(std::runtime_error(message)); - if (detail::parkIfInFrame(*handoff, false, {}, failure)) { - return; - } - onDone(failure); + bool const started = backend->registerModelSharedAsync( + binding->typeId, binding->modelFactory, {.contextKey = binding->contextKey, .primary = {}}, onBound, + [onFailed](const std::string& message) { + onFailed(std::make_exception_ptr(std::runtime_error(message))); }); + if (!started) { + // The structural surface; see `attachHandlerAsync`'s identical + // branch for why `inlineExecutor()` is the executor this call + // site names. An empty `primary` with a zero `current` is the + // request shape that means `registerModelShared` -- the very + // verb the synchronous `ensureBound` calls. + auto completion = + backend->bindModel(::morph::backend::detail::BindRequest{.typeId = binding->typeId, + .factory = binding->modelFactory, + .contextKey = binding->contextKey, + .primary = {}, + .current = {}}, + ::morph::exec::detail::inlineExecutor()); + completion.then(onBound).onError(onFailed); + } } catch (...) { - // See attachHandlerAsync's identical guard: the backend's own + // See attachHandlerAsync's identical guard: a backend's own legacy // dispatch call can throw synchronously before send. lock.unlock(); onDone(std::current_exception()); @@ -815,23 +911,8 @@ class Bridge { onDone(parked->failure); return; } - if (started) { - return; - } - // No async path on this backend: run the identical synchronous - // registration `ensureBound` would have run, under the same lock, then - // report the outcome only once the lock is gone (see - // `attachHandlerAsync`'s "@par Locking"). - std::exception_ptr failure; - try { - auto newId = backend->registerModelShared(binding->typeId, binding->modelFactory, - {.contextKey = binding->contextKey, .primary = {}}); - binding->currentId.store(newId.v); - } catch (...) { - failure = std::current_exception(); - } - lock.unlock(); - onDone(failure); + // Nothing parked: the reply is still to come, and `onBound`/`onFailed` + // will deliver it themselves once it does. } /// @brief Files @p binding's current instance under @p primary, in place. @@ -855,24 +936,26 @@ class Bridge { /// `bool` return threaded across every `IBackend` implementation and, for /// wire backends, a reply field) — tracked as a follow-up, not fixed /// here. - /// Prefers the backend's `IBackend::assignPrimaryAsync` when it offers - /// one — the same "avoid a nested-event-loop block that aborts a WASM + /// Reaches the backend through `IBackend::promoteModel` — the structural + /// registration surface — unless it still overrides the legacy + /// `assignPrimaryAsync`, which is offered first and which morph#571 + /// removes. The same "avoid a nested-event-loop block that aborts a WASM /// main thread" rationale `Bridge::registerHandler()` follows for the - /// initial bind step (`backend.md`, "Asynchronous registration") applies - /// identically here: this method is invoked from inside the result - /// `Completion`'s callback chain (`BridgeHandler::execute`'s `onResult`), - /// not from the original call stack, so there is no caller left blocked - /// waiting on it either way — the async path simply avoids parking the - /// Qt event loop for the round trip. `binding->contextKey`/`primary` are - /// only published once the (possibly async) reply confirms the call, so - /// a caller reading `binding->primary()` never sees a promotion that the - /// backend has not actually completed. Falls back to the synchronous - /// `assignPrimary` when the backend offers no async path, publishing - /// immediately in that case. - /// - /// `_attachMtx` is released *before* calling `assignPrimaryAsync` — not - /// held across it — mirroring `registerHandlerImpl`'s discipline for - /// `registerModelAsync`: the success/error callback below re-acquires + /// initial bind step applies here: this method is invoked from inside the + /// result `Completion`'s callback chain (`BridgeHandler::execute`'s + /// `onResult`), not from the original call stack, so there is no caller + /// left blocked waiting on it either way — a non-blocking promote simply + /// avoids parking the Qt event loop for the round trip. + /// `binding->contextKey`/`primary` are only published once the reply + /// confirms the call, so a caller reading `binding->primary()` never sees a + /// promotion the backend has not actually completed. A backend with no + /// non-blocking promote settles inside the `promoteModel` call, publishing + /// before this method returns; a failure is logged rather than thrown, + /// since `promoteModel` reports through its `Completion` by contract. + /// + /// `_attachMtx` is released *before* the dispatch — not held across it — + /// mirroring `registerHandlerImpl`'s discipline: the success callback + /// below re-acquires /// `_attachMtx`, so a backend that ever invoked it synchronously (none /// documented here do, but nothing prevents one from doing so) would /// otherwise self-deadlock re-acquiring a mutex this same call stack @@ -895,57 +978,63 @@ class Bridge { auto const weakLiveness = _callbacks.token(); std::weak_ptr<::morph::backend::detail::IBackend> const weakBackend{backend}; std::weak_ptr const weakBinding{binding}; - bool const started = backend->assignPrimaryAsync( - ::morph::exec::detail::ModelId{raw}, binding->typeId, primary, - [this, weakLiveness, weakBackend, weakBinding, primary](::morph::exec::detail::ModelId) { - // This check and the `this` touch below it are two steps -- the - // morph#486 shape. Closed not by a gate but by - // `IBackend::registerModelAsync`'s threading contract (see its - // doc comment): an overriding backend must deliver `*Async` - // replies on a thread that cannot run `~Bridge` concurrently. - // Gating instead would block `~Bridge` behind `_attachMtx`, - // which `attachHandler` holds across a full `attachModel` round - // trip. See morph#489. - if (!weakLiveness.active()) { - return; // The Bridge is gone; do not touch `this`. - } - auto strongBinding = weakBinding.lock(); - if (!strongBinding) { - return; // The BridgeHandler (and its binding) is gone. - } - std::scoped_lock const attachLock{_attachMtx}; - auto pinned = weakBackend.lock(); - if (!pinned || pinned != loadBackend()) { - // A switchBackend() already moved past this promotion; - // applying a stale reply now would overwrite whatever - // state the new backend's re-registration already - // established -- same reasoning as registerHandlerImpl's - // async callback. - return; - } - // A binding whose primary is already set (by a concurrent - // attach/assign that raced ahead of this async reply, or - // simply already promoted) must not be overwritten here. - if (!strongBinding->primary.empty()) { - return; - } - strongBinding->contextKey = primary; - strongBinding->primary = primary; - }, - [typeId = binding->typeId](const std::string& message) { - ::morph::log::logError("[assignHandlerPrimary] async promotion of '" + typeId + - "' failed: " + message); - }); - if (!started) { - backend->assignPrimary(::morph::exec::detail::ModelId{raw}, binding->typeId, primary); - std::scoped_lock const lock{_attachMtx}; - // Re-check under the lock: a concurrent attach/assign could have - // raced ahead while the (now-established-synchronous) backend - // call above ran without _attachMtx held. - if (binding->primary.empty()) { - binding->contextKey = primary; - binding->primary = std::move(primary); + auto onPromoted = [this, weakLiveness, weakBackend, weakBinding, primary](::morph::exec::detail::ModelId) { + // This check and the `this` touch below it are two steps -- the + // morph#486 shape. Closed not by a gate but by delivering on a + // thread that cannot run `~Bridge` concurrently: the legacy + // `assignPrimaryAsync` branch below leaves that to the backend (see + // `IBackend::registerModelAsync`'s doc comment), the `promoteModel` + // branch names it as an executor. Gating instead would block + // `~Bridge` behind `_attachMtx`, which `attachHandler` holds across + // a full `attachModel` round trip. See morph#489. + if (!weakLiveness.active()) { + return; // The Bridge is gone; do not touch `this`. } + auto strongBinding = weakBinding.lock(); + if (!strongBinding) { + return; // The BridgeHandler (and its binding) is gone. + } + std::scoped_lock const attachLock{_attachMtx}; + auto pinned = weakBackend.lock(); + if (!pinned || pinned != loadBackend()) { + // A switchBackend() already moved past this promotion; + // applying a stale reply now would overwrite whatever + // state the new backend's re-registration already + // established -- same reasoning as registerHandlerImpl's + // async callback. + return; + } + // A binding whose primary is already set (by a concurrent + // attach/assign that raced ahead of this async reply, or + // simply already promoted) must not be overwritten here. + if (!strongBinding->primary.empty()) { + return; + } + strongBinding->contextKey = primary; + strongBinding->primary = primary; + }; + auto onFailed = [typeId = binding->typeId](const std::string& message) { + ::morph::log::logError("[assignHandlerPrimary] async promotion of '" + typeId + "' failed: " + message); + }; + bool const started = backend->assignPrimaryAsync(::morph::exec::detail::ModelId{raw}, binding->typeId, primary, + onPromoted, onFailed); + if (!started) { + // The structural surface (`IBackend::promoteModel`). A backend with + // no non-blocking promote settles the returned `Completion` from + // inside this call, having run the same synchronous `assignPrimary` + // this replaces; `inlineExecutor()` then delivers `onPromoted` on + // this thread, exactly where the synchronous branch used to publish. + // Unlike that branch, a failure is logged rather than thrown: this + // runs inside the result `Completion`'s callback chain, where an + // escaping exception is swallowed by `CompletionState` anyway, and + // `promoteModel` reports through the `Completion` by contract. + auto completion = backend->promoteModel( + ::morph::backend::detail::PromoteRequest{ + .mid = ::morph::exec::detail::ModelId{raw}, .typeId = binding->typeId, .primary = primary}, + ::morph::exec::detail::inlineExecutor()); + completion.then(onPromoted).onError([onFailed](const std::exception_ptr& failure) { + onFailed(detail::describeFailure(failure)); + }); } } @@ -1740,21 +1829,27 @@ class Bridge { return _backend; } - /// @brief Shared body of both `registerHandler()` overloads: prefers the - /// backend's `registerModelAsync` path (see `IBackend::registerModelAsync`'s - /// doc comment for why — avoiding a nested-event-loop block that - /// aborts a WASM main thread) and falls back to the synchronous - /// `registerModelWithContext` when the backend offers no async path. - /// - /// @p binding is added to `_handlers` *before* the backend call — not - /// after, as the synchronous fallback below does internally — so a + /// @brief Shared body of both `registerHandler()` overloads: binds through + /// `IBackend::bindModel` — the structural registration surface — + /// unless the backend still overrides the legacy + /// `registerModelAsync`, which is offered first and which morph#571 + /// removes. + /// + /// A backend with a non-blocking bind returns an unsettled `Completion` and + /// the binding is returned unbound (see `IBackend::bindModel`'s doc comment + /// for why that matters — a nested-event-loop block aborts a WASM main + /// thread). One with only the blocking default settles inside the call, + /// having run exactly the `registerModelWithContext` this used to call + /// directly, so the binding is bound before this returns and a failure is + /// **rethrown** to the caller, as that call used to throw. + /// + /// @p binding is added to `_handlers` *before* the backend call, so a /// concurrently-running `switchBackend()`/reconnect can already see and /// re-register it even while this registration is still in flight (see - /// the async branch's comment for why that race is harmless). This also - /// means the backend call must not run under `_mtx`: a backend that (unlike - /// every backend documented here) invoked `onRegistered`/`onError` - /// synchronously from inside `registerModelAsync` would otherwise - /// self-deadlock re-acquiring `_mtx` in the callback below. + /// the continuation's comment for why that race is harmless). This also + /// means the backend call must not run under `_mtx`: a backend that settles + /// from inside the dispatch call would otherwise self-deadlock re-acquiring + /// `_mtx` in the continuation below. /// @param binding Binding to register; its `typeId`/`modelFactory`/`contextKey` must be set. void registerHandlerImpl(const std::shared_ptr& binding) { auto backend = loadBackend(); @@ -1796,73 +1891,130 @@ class Bridge { // **set `contextKey` before calling `registerHandler()`, and do not // mutate it concurrently with that call.** After registration returns, // every access goes under `_attachMtx` as documented. morph#505. - bool const started = backend->registerModelAsync( - binding->typeId, binding->modelFactory, binding->contextKey, - [this, weakBackend, weakBinding, lifetime = _lifetime](::morph::exec::detail::ModelId newId) { - auto strongBinding = weakBinding.lock(); - bool applied = false; - if (strongBinding) { - // `lifetime`'s gate held across the whole touch of `this` - // below (`_mtx`, `loadBackend()`), not just at entry -- - // `CallbackToken::active()` is advisory and cannot carry - // this weight (see `detail::BridgeLifetime`'s own doc - // comment). Safe to hold across this span, unlike - // `installReconnectHandler`'s reconnect callback - // (morph#489, site 4): nothing inside is a call into - // unbounded/consumer-supplied code, only a mutex and a - // backend-pointer comparison. - std::shared_lock const gate{lifetime->mtx}; - if (lifetime->alive) { - std::scoped_lock const lock{_mtx}; - auto pinned = weakBackend.lock(); - if (pinned && pinned == loadBackend()) { - // A switchBackend() already moved past this registration - // (see this backend's own doc comment on the class) and - // its own re-registration loop already gave `binding` a - // fresh id on the *new* backend -- applying this stale - // one now would overwrite that with a dangling id from a - // backend nothing uses any more. - strongBinding->currentId.store(newId.v); - applied = true; - } + auto onRegistered = [this, weakBackend, weakBinding, + lifetime = _lifetime](::morph::exec::detail::ModelId newId) { + auto strongBinding = weakBinding.lock(); + bool applied = false; + if (strongBinding) { + // `lifetime`'s gate held across the whole touch of `this` + // below (`_mtx`, `loadBackend()`), not just at entry -- + // `CallbackToken::active()` is advisory and cannot carry + // this weight (see `detail::BridgeLifetime`'s own doc + // comment). Safe to hold across this span, unlike + // `installReconnectHandler`'s reconnect callback + // (morph#489, site 4): nothing inside is a call into + // unbounded/consumer-supplied code, only a mutex and a + // backend-pointer comparison. + std::shared_lock const gate{lifetime->mtx}; + if (lifetime->alive) { + std::scoped_lock const lock{_mtx}; + auto pinned = weakBackend.lock(); + if (pinned && pinned == loadBackend()) { + // A switchBackend() already moved past this registration + // (see this backend's own doc comment on the class) and + // its own re-registration loop already gave `binding` a + // fresh id on the *new* backend -- applying this stale + // one now would overwrite that with a dangling id from a + // backend nothing uses any more. + strongBinding->currentId.store(newId.v); + applied = true; } } - // Resolve whenBound() waiters regardless of whether the id was - // actually applied above: either way this binding's initial - // registration attempt has settled (a stale reply ignored here - // means switchBackend's own synchronous re-registration already - // bound it), so nothing should still be described as "in - // flight". Runs even when the Bridge/binding is gone -- both - // weak locks above are only guards on touching `this`/the - // binding's other fields, not on this bookkeeping, which reads - // no Bridge state. - if (strongBinding) { - resolveRegistrationWaiters(*strongBinding, /*ok=*/applied || isBound(strongBinding), nullptr); + } + // Resolve whenBound() waiters regardless of whether the id was + // actually applied above: either way this binding's initial + // registration attempt has settled (a stale reply ignored here + // means switchBackend's own synchronous re-registration already + // bound it), so nothing should still be described as "in + // flight". Runs even when the Bridge/binding is gone -- both + // weak locks above are only guards on touching `this`/the + // binding's other fields, not on this bookkeeping, which reads + // no Bridge state. + if (strongBinding) { + resolveRegistrationWaiters(*strongBinding, /*ok=*/applied || isBound(strongBinding), nullptr); + } + }; + auto onFailed = [weakBinding, typeId = binding->typeId](const std::string& message) { + ::morph::log::logError("[registerHandler] async registration of '" + typeId + "' failed: " + message); + if (auto strongBinding = weakBinding.lock()) { + resolveRegistrationWaiters( + *strongBinding, /*ok=*/false, + std::make_exception_ptr(std::runtime_error("registration failed: " + message))); + } + }; + + bool const started = backend->registerModelAsync(binding->typeId, binding->modelFactory, binding->contextKey, + onRegistered, onFailed); + if (started) { + return; + } + + // The structural surface (`IBackend::bindModel`). An empty `primary` + // with a zero `current` is the request shape that means + // `registerModelWithContext` -- the verb this branch used to call + // directly -- so a backend with no non-blocking bind runs exactly that, + // blocks exactly as long, and settles before `bindModel` returns. + // + // `inlineExecutor()` because that is where this continuation ran + // before: on whichever thread the backend settled on, which for the + // blocking default is this one. See `exec::detail::InlineExecutor`. + // + // A synchronous backend that *fails* must still fail the way it used to + // -- `registerModelWithContext` threw out of `registerHandler()`, and a + // `BridgeHandler` constructor that cannot bind must not return as if it + // had. So a rejection arriving inside this frame is parked and + // rethrown, while one arriving later (a genuinely non-blocking backend, + // where no caller is left to throw at) goes to `onFailed`, which + // settles `whenBound()` waiters instead. + auto handoff = std::make_shared(); + auto completion = backend->bindModel(::morph::backend::detail::BindRequest{.typeId = binding->typeId, + .factory = binding->modelFactory, + .contextKey = binding->contextKey, + .primary = {}, + .current = {}}, + ::morph::exec::detail::inlineExecutor()); + completion + .then([onRegistered, handoff](::morph::exec::detail::ModelId newId) { + if (detail::parkIfInFrame(*handoff, true, newId, nullptr)) { + return; } - }, - [weakBinding, typeId = binding->typeId](const std::string& message) { - ::morph::log::logError("[registerHandler] async registration of '" + typeId + "' failed: " + message); - if (auto strongBinding = weakBinding.lock()) { - resolveRegistrationWaiters( - *strongBinding, /*ok=*/false, - std::make_exception_ptr(std::runtime_error("registration failed: " + message))); + onRegistered(newId); + }) + .onError([onFailed, handoff](const std::exception_ptr& failure) { + if (detail::parkIfInFrame(*handoff, false, {}, failure)) { + return; } + onFailed(detail::describeFailure(failure)); }); - - if (!started) { - binding->currentId.store( - backend->registerModelWithContext(binding->typeId, binding->modelFactory, binding->contextKey).v); - // Route through resolveRegistrationWaiters (not a bare flag - // clear): a whenBound() call from another thread that already - // holds this same binding (the pre-built-binding registerHandler() - // overload hands the caller the shared_ptr before this function - // is even called) could have raced in during the window above and - // queued a waiter while registrationInFlight was still true. That - // waiter's Completion must still be settled here, or it hangs - // forever -- registrationInFlight was never in flight for a - // backend with no async path, but whenBound() cannot tell that - // apart from "the reply just hasn't arrived yet" without this. - resolveRegistrationWaiters(*binding, /*ok=*/true, nullptr); + // `registerHandler` is a synchronous entry point: its caller + // constructs a `BridgeHandler` and uses it on the next line, and + // `executeVia` fails fast on an unbound `currentId`. So unless the + // backend says its completion cannot settle while this thread waits, + // this frame waits for it -- and the outcome is then published and + // rethrown by exactly the code below that already handles a backend + // that settled inline. The two cases differ in how long this frame + // sits still, not in what the caller observes. + // + // `kCallerMustNotBlock` is the exception, and it is the *reason* it is + // an exception that matters: a `QtWebSocketBackend` with + // `asyncRegistrationEnabled` set delivers its reply through the Qt + // event loop of this very thread, so waiting here is a deadlock, not a + // delay -- on a WASM main thread it aborts the page (morph#568). Such a + // backend's caller gets an unbound handler and must gate on + // `whenBound()`, exactly as the `*Async` path already required of it. + // See `IBackend::bindWaitPolicy` and morph#593. + auto parked = backend->bindWaitPolicy() == ::morph::backend::detail::BindWait::kCallerMayBlock + ? detail::awaitHandoff(*handoff) + : detail::claimHandoff(*handoff); + if (parked) { + if (!parked->succeeded) { + // Settles the waiters queued during the window above before + // unwinding, so a concurrent whenBound() is rejected rather + // than left hanging on a registration that will never complete. + resolveRegistrationWaiters(*binding, /*ok=*/false, parked->failure); + std::rethrow_exception(parked->failure); + } + onRegistered(parked->modelId); } } diff --git a/include/morph/core/executor.hpp b/include/morph/core/executor.hpp index f505a7be9..b324efe11 100644 --- a/include/morph/core/executor.hpp +++ b/include/morph/core/executor.hpp @@ -229,4 +229,55 @@ class MainThreadExecutor : public IExecutor { std::queue> _q; }; +namespace detail { + +/// @brief Executor that runs each posted task on the posting thread, at once. +/// +/// "Deliver wherever the producer settled", expressed as an executor rather +/// than as a rule nobody can check. `Bridge` names it at the structural +/// registration surface (`IBackend::bindModel`/`promoteModel`, see +/// `docs/spec/core/backend.md`) because `Bridge` owns no thread of its own: it +/// has no event loop to post a registration continuation to, and the four +/// legacy `*Async` verbs it is replacing delivered their callbacks on exactly +/// this thread — whichever one the backend settled the reply on. Naming that +/// choice at the call site is the point of the change: the *caller* now decides +/// where a registration continuation runs, and can be changed to decide +/// differently without touching a single backend. +/// +/// @warning Not a general-purpose executor. Posting to it re-enters the caller, +/// so a handler that takes a lock the posting frame already holds +/// self-deadlocks. Every `Bridge` site that names it either releases +/// its locks first or parks the outcome through +/// `detail::AsyncDispatchHandoff` (see `Bridge::attachHandlerAsync`'s +/// `@par Locking`). Application code that wants "run it now" should +/// call the function instead of posting it. +// NOLINTNEXTLINE(cppcoreguidelines-special-member-functions) +class InlineExecutor : public IExecutor { +public: + /// @brief Runs @p task immediately, on the calling thread. + /// + /// An empty @p task is ignored rather than invoked, since `std::function`'s + /// empty state is a legal value to move around and calling it is undefined. + /// @param task Callable to execute. + void post(std::function task) override { + if (task) { + task(); + } + } +}; + +/// @brief The process-wide `InlineExecutor`. +/// +/// A function-local static, so it is constructed on first use and outlives every +/// `Completion` built against it — which is exactly what `Completion`'s "the +/// executor must outlive the completion" requirement asks of a caller that has +/// no executor of its own to name. +/// @return Reference to the shared inline executor. +inline IExecutor& inlineExecutor() { + static InlineExecutor executor; + return executor; +} + +} // namespace detail + } // namespace morph::exec diff --git a/include/morph/qt/qt_websocket_backend.hpp b/include/morph/qt/qt_websocket_backend.hpp index 780ce42d6..8454745b1 100644 --- a/include/morph/qt/qt_websocket_backend.hpp +++ b/include/morph/qt/qt_websocket_backend.hpp @@ -39,34 +39,51 @@ struct QtWebSocketBackendConfig { /// @brief Multiplier applied to the delay after each failed attempt. double backoffMultiplier = 2.0; - /// @brief Opt in to `registerModelAsync` (see its doc comment on `IBackend`). - /// - /// Defaults to `false`: `Bridge::registerHandler()` then falls back to the - /// synchronous `registerModel`, exactly as before this feature existed — - /// every existing embedder (a desktop Qt client, this backend's own test - /// suite) keeps registering synchronously, immediately usable the line - /// after `BridgeHandler`'s constructor returns. - /// - /// Set `true` only for a build where that synchronous guarantee cannot - /// hold at all — a WASM main thread, where the nested `QEventLoop` - /// `registerModel` relies on aborts the page outright. Doing so is a - /// deliberate trade: the caller must then wait for registration to - /// complete (e.g. gate the UI on it) before firing an action through that - /// handler, since `executeVia` fails fast with "handler not bound" for an - /// unbound binding rather than queuing or blocking. + /// @brief Whether `bindModel`/`promoteModel` may return before the reply. + /// + /// Defaults to `false`: both settle their `Completion` from inside the call, + /// having blocked the Qt thread in a nested `QEventLoop` for the round trip + /// — `IBackend`'s own default behaviour, and what every existing embedder + /// (a desktop Qt client, this backend's own test suite) already relies on, + /// since it makes a handler usable on the line after `BridgeHandler`'s + /// constructor returns. + /// + /// Set `true` for a build where that blocking call cannot happen at all — + /// a WASM main thread, where Qt refuses to spin a nested loop and the + /// attempt aborts the page outright. `bindModel` then sends the request and + /// returns an unsettled `Completion`, which the reply settles later. That is + /// a deliberate trade, not a free improvement: the caller must wait for the + /// continuation (e.g. gate the UI on `BridgeHandler::whenBound()`) before + /// firing an action through that handler, since `executeVia` fails fast + /// with "handler not bound" for an unbound binding rather than queuing or + /// blocking. + /// + /// This flag chooses *whether the transport blocks*. It is no longer an + /// opt-in to a second set of interface verbs: the continuation exists on + /// both paths, because `bindModel` returns a `Completion` either way. bool asyncRegistrationEnabled = false; }; /// @brief `IBackend` implementation that communicates with a `RemoteServer` over WebSocket. /// -/// `registerModel()` is synchronous (blocks the calling thread via a nested -/// `QEventLoop` until the server replies) -- unusable on a WASM main thread, -/// which Qt refuses to spin a nested loop on at all. `registerModelAsync()` -/// is the non-blocking alternative `Bridge::registerHandler()` prefers when -/// available (see `IBackend::registerModelAsync`'s doc comment): it assigns a -/// call-id, sends the message, returns immediately, and invokes exactly one -/// of its `onRegistered`/`onError` callbacks once the matching reply arrives -/// -- the same call-id-matching mechanism `execute()` already uses. +/// Registration goes through the structural surface: `bindModel()` and +/// `promoteModel()` (`IBackend`, and `docs/spec/core/backend.md`'s "The +/// structural registration surface"). This is the one backend in the tree that +/// implements them *natively* rather than through the blocking defaults or +/// `SynchronousBackendAdapter` — with `Config::asyncRegistrationEnabled` set it +/// assigns a call-id, sends the message, and returns an unsettled `Completion` +/// that the matching reply settles, using the same call-id-matching mechanism +/// `execute()` already uses. Which thread the continuation then runs on is the +/// caller's choice, not this class's: `Completion` posts to the `IExecutor` +/// passed to `bindModel`/`promoteModel`, so a caller that needs the +/// continuation on its own thread names its own executor and gets it. +/// +/// The synchronous verbs (`registerModel()`, `registerModelShared()`, +/// `attachModel()`, `assignPrimary()`) remain, and still block the calling +/// thread in a nested `QEventLoop` until the server replies — unusable on a +/// WASM main thread, which Qt refuses to spin a nested loop on at all. They are +/// what the default (`asyncRegistrationEnabled == false`) `bindModel` runs, and +/// what `Bridge::switchBackend()` re-registers through. /// `deregisterModel()` is fire-and-forget (it sends the message without /// waiting, avoiding a nested event loop during destruction). `execute()` is /// asynchronous: it assigns a call-id, sends the message, and resolves the @@ -198,52 +215,120 @@ class QtWebSocketBackend : public ::morph::backend::detail::IBackend { const std::string& typeId, std::function()> factory) override; - /// @brief Sends a `register` message and returns without blocking; the - /// reply is matched later, asynchronously, by `callId`. - /// - /// The non-blocking counterpart to `registerModel`/`registerModelWithContext` - /// (see `IBackend::registerModelAsync`'s doc comment for why this exists): - /// `registerModel` blocks the calling thread in a nested `QEventLoop` via - /// `sendSync`, which a WASM main thread cannot do at all. This instead - /// assigns a fresh `callId` (the same counter `execute()` uses), sends the - /// `register` envelope, and returns `true` immediately; the reply is - /// matched via `_pendingRegistrations` when `onTextMessage` sees it (no - /// protocol change needed — the server already echoes `callId` on every - /// reply, `register` included). Exactly one of @p onRegistered / @p onError - /// fires, on the Qt event loop thread, once the reply arrives — or never, - /// if the socket disconnects first without ever reconnecting and - /// `cancelPending` is never called again for this id (a disconnect - /// *before* a reconnect calls `cancelPending`, which does invoke @p onError - /// — see `cancelPending`'s doc comment). - /// - /// If the socket has not finished connecting yet, the request is **queued** - /// rather than failed: this is exactly the ordering a single-threaded WASM - /// client must use, since it can never block waiting for the connection to - /// settle (a `BridgeHandler` constructed the moment the backend is wired - /// up, before the first `connected` signal). The queued request is sent — - /// with a call-id assigned at that point, not now — the moment `connected` - /// fires next (first connect included), in FIFO order. If the socket never - /// connects at all and the backend is torn down first, the queued entry is - /// still resolved: `~QtWebSocketBackend` calls `cancelPending`, which drains - /// `_queuedRegistrations` too and invokes @p onError exactly once for each — - /// no call-id was ever assigned, but the callback still fires, the same - /// guarantee an already-sent (call-id-bearing) registration gets. - /// - /// @param typeId String type-id of the model to instantiate. - /// @param factory Unused — this backend holds no local model to construct; - /// the server instantiates the model from `typeId`. - /// @param contextKey Stable identity of the new instance; travels in the wire envelope. - /// @param onRegistered Invoked with the server-assigned `ModelId` on success. - /// @param onError Invoked with a diagnostic message on failure or disconnect. - /// @return `true` when `Config::asyncRegistrationEnabled` is set (the - /// backend then owns the reply); `false` when it is not — which is - /// the default, so the caller falls back to the synchronous path - /// unless the embedder opted in. - bool registerModelAsync(const std::string& typeId, - std::function()> factory, - std::string_view contextKey, - std::function onRegistered, - std::function onError) override; + /// @brief Sends a `register` carrying @p contextKey and blocks until the reply arrives. + /// + /// `IBackend::registerModelWithContext`'s default drops @p contextKey, which + /// is right for `LocalBackend` — the caller's own factory closure already + /// captures the identity — but wrong for a backend whose instances live on + /// the far side of a wire protocol: the server constructs the holder itself, + /// so `contextKey` is the *only* channel by which the instance's identity + /// reaches it. `RemoteServer::attachLogIfConfigured` returns without + /// consulting its `LogProvider` at all when the envelope's `contextKey` is + /// empty, so dropping it here did not merely lose an entity key — it left + /// the instance **unjournalled** (morph#594). `SimulatedRemoteBackend` and + /// `morph::net::SocketBackend` (morph#587) override this for the same + /// reason; backends documented as interchangeable must not disagree about + /// whether a private registration is audited. + /// + /// This is also the verb the *blocking* `bindModel` path reaches for an + /// empty-`primary`, zero-`current` request, and the one + /// `Bridge::switchBackend` calls directly when it re-registers a handler + /// after a reconnect — so before morph#594 the key was dropped whatever + /// `Config::asyncRegistrationEnabled` was set to on a backend swap, and + /// dropped on every private registration when it was unset. `bindModel`'s + /// own non-blocking path already carried it. + /// + /// `registerModel` forwards here with an empty key, so there is one place + /// that builds this envelope rather than two that can drift apart. + /// + /// @param typeId String type-id of the model to register. + /// @param factory Ignored — model construction is delegated to the server. + /// @param contextKey Stable identity of the new instance; empty if none. + /// @return `ModelId` assigned by the server. + /// @throws std::runtime_error if the server replies with an error or the socket is not connected. + ::morph::exec::detail::ModelId registerModelWithContext( + const std::string& typeId, std::function()> factory, + std::string_view contextKey) override; + + /// @brief Acquires a model instance over the wire, natively non-blocking + /// when `Config::asyncRegistrationEnabled` is set. + /// + /// The structural registration surface (`IBackend::bindModel`), implemented + /// here rather than inherited: this is the one backend in the tree with a + /// genuinely non-blocking acquire path, so it settles the returned + /// `Completion` when the server's reply arrives instead of blocking a + /// thread until then. + /// + /// With `Config::asyncRegistrationEnabled` unset (the default) this defers + /// to `IBackend::bindModel`, which runs the synchronous verb the request's + /// shape names and **blocks the Qt thread** in a nested `QEventLoop` for the + /// round trip. That is the desktop behaviour every existing embedder + /// relies on; see `QtWebSocketBackendConfig::asyncRegistrationEnabled`. + /// + /// With it set, the request's shape selects the envelope, mirroring + /// `IBackend::bindModelBlocking`'s routing of the same three shapes: + /// + /// | `primary` | `current` | Envelope sent | + /// |---|---|---| + /// | empty | `0` | `register` (private) | + /// | empty | non-zero | `deregister` of `current`, then `register` | + /// | non-empty | `0` | `register` with `shared` (register-or-attach) | + /// | non-empty | non-zero | `attach`, naming `current` | + /// + /// Each carries a fresh `callId` from the counter `execute()` uses, and the + /// reply is matched via `_pendingRegistrations` when `onTextMessage` sees + /// it — no protocol change, since the server already echoes `callId` on + /// every reply. + /// + /// A **private** registration issued before the socket has finished + /// connecting is queued rather than failed: exactly the ordering a + /// single-threaded WASM client must use, since it can never block waiting + /// for the connection to settle (a `BridgeHandler` constructed the moment + /// the backend is wired up, before the first `connected` signal). The queued + /// request is sent — with a call-id assigned at that point, not now — the + /// moment `connected` fires next, first connect included, in FIFO order. If + /// the socket never connects and the backend is torn down first, the queued + /// entry is still settled: `~QtWebSocketBackend` calls `cancelPending`, + /// which drains `_queuedRegistrations` too and rejects each exactly once. + /// A keyed bind on a disconnected socket rejects immediately instead. + /// + /// @param request Owning bind request; moved from. `request.factory` is + /// unused — this backend holds no local model to construct; + /// the server instantiates the model from `request.typeId`. + /// @param cbExec Executor the continuation is delivered on. Borrowed: it + /// must outlive the returned `Completion`. + /// @return A `Completion` resolved with the bound `ModelId`, or rejected + /// with the failure. Already settled on the blocking path. + ::morph::async::Completion<::morph::exec::detail::ModelId> bindModel(::morph::backend::detail::BindRequest request, + ::morph::exec::IExecutor& cbExec) override; + + /// @brief Whether a caller may block waiting for this backend's + /// `bindModel`/`promoteModel` completions. + /// + /// `kCallerMustNotBlock` exactly when `Config::asyncRegistrationEnabled` is + /// set, because that is exactly when a completion is settled by + /// `onTextMessage` — a Qt slot, delivered by the event loop of the thread + /// that called `bindModel`. A caller blocked in a wait is not running that + /// event loop, so the reply it is waiting for can never arrive: the + /// deadlock morph#568 exists to remove, which on a WASM main thread aborts + /// the page outright. + /// + /// With the flag unset this backend's `bindModel` is `IBackend`'s default, + /// which settles inside the call, so `kCallerMayBlock` is both true and + /// free: the caller's wait finds the outcome already parked and returns + /// without sleeping. + /// + /// Note which way round this reads. It does not say "registration is + /// asynchronous" — `SocketBackend`'s is too, and it answers + /// `kCallerMayBlock` because a separate I/O thread settles its completions. + /// It says only that *this* thread must not stop and wait. See morph#593. + /// + /// @return `kCallerMustNotBlock` when `Config::asyncRegistrationEnabled` is + /// set, `kCallerMayBlock` otherwise. + [[nodiscard]] ::morph::backend::detail::BindWait bindWaitPolicy() const noexcept override { + return _cfg.asyncRegistrationEnabled ? ::morph::backend::detail::BindWait::kCallerMustNotBlock + : ::morph::backend::detail::BindWait::kCallerMayBlock; + } /// @brief Sends a shared (register-or-attach) `register` and blocks for the reply. /// @@ -257,31 +342,6 @@ class QtWebSocketBackend : public ::morph::backend::detail::IBackend { const std::string& typeId, std::function()> factory, ::morph::backend::detail::InstanceIdentity identity) override; - /// @brief Sends a shared (register-or-attach) `register` and, if async - /// registration is enabled, returns without blocking. - /// - /// The non-blocking counterpart to `registerModelShared`, matching - /// `registerModelAsync`'s shape exactly (same `callId` counter, same - /// `_pendingRegistrations` map, same verb-agnostic reply routing in - /// `onTextMessage`). An empty `identity.primary` degrades to the private - /// path, i.e. to `registerModelAsync`, mirroring the synchronous - /// `registerModelShared`'s own degrade-to-private behaviour. - /// - /// @param typeId String type-id of the model. - /// @param factory Ignored — model construction is delegated to the server. - /// @param identity Entity key for the action log plus the directory primary key. - /// @param onRegistered Invoked with the assigned `ModelId` on success. - /// @param onError Invoked with a diagnostic message on failure. - /// @return `true` if `asyncRegistrationEnabled` is set (see - /// `QtWebSocketBackendConfig`) and the request was sent; - /// `false` otherwise, falling back to the synchronous - /// `registerModelShared`. - bool registerModelSharedAsync(const std::string& typeId, - std::function()> factory, - ::morph::backend::detail::InstanceIdentity identity, - std::function onRegistered, - std::function onError) override; - /// @brief Sends an `attach` and blocks for the reply, re-pointing from @p current. /// @param typeId String type-id of the model. /// @param factory Ignored — model construction is delegated to the server. @@ -293,30 +353,6 @@ class QtWebSocketBackend : public ::morph::backend::detail::IBackend { const std::string& typeId, std::function()> factory, ::morph::backend::detail::InstanceIdentity identity, ::morph::exec::detail::ModelId current) override; - /// @brief Sends an `attach` and, if async registration is enabled, - /// returns without blocking. - /// - /// The non-blocking counterpart to `attachModel`; see - /// `registerModelSharedAsync` immediately above for the shared shape. An - /// empty `identity.primary` releases @p current and degrades to a private - /// async registration, mirroring the synchronous `attachModel`'s own - /// empty-primary branch. - /// - /// @param typeId String type-id of the model. - /// @param factory Ignored — model construction is delegated to the server. - /// @param identity Entity key for the action log plus the directory primary key. - /// @param current Instance currently held, or `ModelId{0}` if none. - /// @param onRegistered Invoked with the `ModelId` now attached to, on success. - /// @param onError Invoked with a diagnostic message on failure. - /// @return `true` if `asyncRegistrationEnabled` is set and the request - /// was sent; `false` otherwise, falling back to the synchronous - /// `attachModel`. - bool attachModelAsync(const std::string& typeId, - std::function()> factory, - ::morph::backend::detail::InstanceIdentity identity, ::morph::exec::detail::ModelId current, - std::function onRegistered, - std::function onError) override; - /// @brief Files a live server-side instance under @p primary. /// @param mid Live instance to promote. /// @param typeId Model type id. @@ -324,30 +360,34 @@ class QtWebSocketBackend : public ::morph::backend::detail::IBackend { void assignPrimary(::morph::exec::detail::ModelId mid, const std::string& typeId, std::string_view primary) override; - /// @brief Sends an `assign` message and returns without blocking; the - /// reply is matched later, asynchronously, by `callId`. - /// - /// The non-blocking counterpart to `assignPrimary` (see - /// `IBackend::assignPrimaryAsync`'s doc comment): `assignPrimary` blocks - /// the calling thread in a nested `QEventLoop` via `sendSync`, the same - /// shape `registerModelAsync` exists to let a caller avoid for the bind - /// step. This instead assigns a fresh `callId` (the same counter - /// `execute()`/`registerModelAsync()` use), sends the `assign` envelope, - /// and returns `true` immediately; the reply is matched via - /// `_pendingAssigns` when `onTextMessage` sees it. Exactly one of - /// @p onRegistered / @p onError fires, on the Qt event loop thread, once - /// the reply arrives. - /// - /// @param mid Live instance to promote. - /// @param typeId Model type id. - /// @param primary Canonical string encoding of the key to file it under. - /// @param onRegistered Invoked with @p mid on success (including the - /// documented no-op cases -- see `IBackend::assignPrimaryAsync`). - /// @param onError Invoked with a diagnostic message on failure or disconnect. - /// @return `true` always -- this backend has an async path (`false` is never returned). - bool assignPrimaryAsync(::morph::exec::detail::ModelId mid, const std::string& typeId, std::string_view primary, - std::function onRegistered, - std::function onError) override; + /// @brief Files a live server-side instance under a key, without blocking. + /// + /// The structural counterpart of `assignPrimary` (`IBackend::promoteModel`), + /// implemented natively for the same reason `bindModel` above is: + /// `assignPrimary` blocks the Qt thread in a nested `QEventLoop` via + /// `sendSync`, which a WASM main thread cannot do. This assigns a fresh + /// `callId` from the counter `execute()`/`bindModel()` use, sends the + /// `assign` envelope, and returns an unsettled `Completion`; the reply is + /// matched via `_pendingAssigns` when `onTextMessage` sees it. + /// + /// Unlike `bindModel`, this does **not** consult + /// `Config::asyncRegistrationEnabled`: promotion happens from inside the + /// result `Completion`'s callback chain, where no caller is left blocked + /// waiting for it either way, so there is no synchronous guarantee to + /// preserve — which is why `assignPrimaryAsync`, the verb this replaces, + /// had no opt-in gate either. + /// + /// The documented no-op cases (empty `primary`, zero `mid`) resolve with + /// @p request's `mid` without sending anything, matching + /// `IBackend::promoteModel`. A disconnected socket rejects. + /// + /// @param request Owning promote request; moved from. + /// @param cbExec Executor the continuation is delivered on. Borrowed: it + /// must outlive the returned `Completion`. + /// @return A `Completion` resolved with the promoted `ModelId`, or rejected + /// with the failure. + ::morph::async::Completion<::morph::exec::detail::ModelId> promoteModel( + ::morph::backend::detail::PromoteRequest request, ::morph::exec::IExecutor& cbExec) override; /// @brief Asks the server for the live shared primary keys of @p typeId. /// @param typeId String type-id to enumerate. @@ -363,7 +403,7 @@ class QtWebSocketBackend : public ::morph::backend::detail::IBackend { /// model registered on the server indefinitely. /// /// Assigned a real, non-zero `callId` from the same counter/namespace - /// `execute()`/`registerModelAsync()` use (see issue #65): `callId == 0` + /// `execute()`/`bindModel()` use (see issue #65): `callId == 0` /// is reserved for a parked synchronous control call's reply, and a /// fire-and-forget `deregister` sharing that sentinel could otherwise have /// its own stray "ok" reply handed to an unrelated `registerModel`'s @@ -390,16 +430,14 @@ class QtWebSocketBackend : public ::morph::backend::detail::IBackend { /// @brief No-op — this backend holds no local model objects. void notifyBackendChanged() override {} - /// @brief Resolves every pending execute call's `Completion` with @p exc, - /// and fails every pending async registration's `onError`. + /// @brief Rejects every in-flight call's `Completion` with @p exc — + /// executes, binds (queued ones included) and promotes alike. /// /// Called by `Bridge::switchBackend()` on the outgoing backend, by `~Bridge`, /// and internally when the socket disconnects. Late replies arriving for - /// already-cancelled call ids (execute or register) are dropped silently. + /// already-cancelled call ids (execute, bind or promote) are dropped silently. /// - /// @param exc Exception delivered to every pending completion's error sink; - /// `exc.what()`-equivalent text is delivered to every pending - /// `registerModelAsync` call's `onError`. + /// @param exc Exception delivered to every pending completion's error sink. void cancelPending(const std::exception_ptr& exc) override; /// @brief Installs the handler `Bridge` uses to re-register handlers after a reconnect. @@ -451,15 +489,10 @@ class QtWebSocketBackend : public ::morph::backend::detail::IBackend { /// @return `true` if an execute was found and settled. bool tryRouteExecuteReply(const ::morph::wire::Envelope& env); - /// @brief Completes the async model registration filed under `env.callId`. - /// @param env Decoded reply envelope. - /// @return `true` if a pending registration was found and completed. - bool tryRouteRegistrationReply(const ::morph::wire::Envelope& env); - - /// @brief Completes the async primary-assignment filed under `env.callId`. + /// @brief Settles the bind/promote filed under `env.callId`, if one is pending. /// @param env Decoded reply envelope. - /// @return `true` if a pending assignment was found and completed. - bool tryRouteAssignReply(const ::morph::wire::Envelope& env); + /// @return `true` if a pending control call was found and settled. + bool tryRouteControlReply(const ::morph::wire::Envelope& env); /// @brief Drops the reply to a fire-and-forget deregister (issue #65). /// @param env Decoded reply envelope. @@ -472,16 +505,24 @@ class QtWebSocketBackend : public ::morph::backend::detail::IBackend { /// @brief Attempts to reopen the socket using the saved URL/TLS config. void attemptReconnect(); - /// @brief Assigns a call-id, records the pending registration, and sends - /// the `register` envelope. Shared by `registerModelAsync`'s - /// immediate path and the queued-request flush on `connected`. - void sendRegisterAsync(const std::string& typeId, std::string_view contextKey, - std::function onRegistered, - std::function onError); + /// @brief Assigns a call-id, records @p promise, and sends @p env. + /// + /// The one send path every non-blocking control call takes, so the ordering + /// invariant it enforces — encode before the map insertion, because a + /// throwing `wire::encode()` after inserting would park a promise for a + /// reply to a message that was never sent — is stated and tested once + /// rather than four times. + /// + /// @param env Control envelope to send; its `callId`/`session` are + /// stamped here. + /// @param promise Settled when the matching reply arrives, or by + /// `cancelPending`. + void sendControl(::morph::wire::Envelope env, + ::morph::async::Completion<::morph::exec::detail::ModelId>::Promise promise); - /// @brief Sends every request queued by `registerModelAsync` while the - /// socket was not yet connected, in FIFO order. Called from the - /// `connected` slot, before the reconnect handler fires. + /// @brief Sends every private bind queued while the socket was not yet + /// connected, in FIFO order. Called from the `connected` slot, + /// before the reconnect handler fires. void flushQueuedRegistrations(); QUrl _serverUrl; @@ -512,29 +553,29 @@ class QtWebSocketBackend : public ::morph::backend::detail::IBackend { std::unordered_map _pending; std::mutex _pendingMtx; - /// @brief One in-flight `registerModelAsync` call, keyed by `callId`. + /// @brief In-flight `bindModel`/`promoteModel` calls, keyed by `callId`. /// /// Kept separate from `PendingExecute`/`_pending` (a different `callId` /// namespace would be a protocol change; this shares the same namespace - /// and counter, just a different local map) because a register reply's - /// shape (`modelId`, no `deserialize` step) differs from an execute - /// reply's. - struct PendingRegistration { - std::function onRegistered; - std::function onError; - }; - std::unordered_map _pendingRegistrations; - - /// @brief One `registerModelAsync` call made before the socket had - /// finished connecting. No call-id is assigned until the request - /// is actually sent (from `flushQueuedRegistrations`), so a queued - /// entry that never gets to fire (backend destroyed first) needs no - /// cancellation bookkeeping. + /// and counter, just a different local map) because a control reply's shape + /// (`modelId`, no `deserialize` step) differs from an execute reply's. + /// + /// One map for both verbs, not two: `register`, `registerShared`, `attach` + /// and `assign` replies are all matched identically — a bare `modelId` + /// echoed against the `callId` — so the split the four `*Async` verbs used + /// to justify has nothing left to represent. + std::unordered_map::Promise> + _pendingRegistrations; + + /// @brief One private `bindModel` issued before the socket had finished + /// connecting. No call-id is assigned until the request is actually + /// sent (from `flushQueuedRegistrations`), so a queued entry that + /// never gets to fire (backend destroyed first) is drained by + /// `cancelPending` from here rather than by call-id. struct QueuedRegistration { std::string typeId; std::string contextKey; - std::function onRegistered; - std::function onError; + ::morph::async::Completion<::morph::exec::detail::ModelId>::Promise promise; }; std::vector _queuedRegistrations; @@ -549,18 +590,6 @@ class QtWebSocketBackend : public ::morph::backend::detail::IBackend { /// backend was cancelled/destroyed) is simply ignored — nothing to clean /// up either way. std::unordered_set _pendingDeregisters; - - /// @brief One in-flight `assignPrimaryAsync` call, keyed by `callId`. - /// - /// Mirrors `PendingRegistration`/`_pendingRegistrations`: same `callId` - /// namespace and counter, separate map because an `assign` reply is - /// matched the same way a `register`/`registerShared` reply is (a bare - /// `modelId`, echoing back the instance that was promoted). - struct PendingAssign { - std::function onRegistered; - std::function onError; - }; - std::unordered_map _pendingAssigns; }; } // namespace morph::qt diff --git a/scripts/branch_partial_allowlist.json b/scripts/branch_partial_allowlist.json index 38b0257e5..db4d16423 100644 --- a/scripts/branch_partial_allowlist.json +++ b/scripts/branch_partial_allowlist.json @@ -80,7 +80,7 @@ }, { "file": "include/morph/core/backend.hpp", - "line": 1230, + "line": 1324, "source": "if (const auto* inst = _instances.find(modelId)) {", "reason": "Unreachable by construction given the `_changeAware`/`_instances` invariant (core audit finding BK2). `_changeAware` is an index over the instance directory: an id enters it in `createHolder` (this file, when the holder answers `isBackendChangeAware()`) in the same `_regMtx`-held critical section that files the instance, and leaves it in `deregisterModel` only when `InstanceDirectory::release` reports the instance actually destroyed. `notifyBackendChanged()` (this function) holds the same `_regMtx` while walking `_changeAware` and looking each id up at this line, so every id it walks is still live -- the null arm cannot occur without a code change that breaks that subset invariant. Formerly keyed on `_models`, the map morph#523 replaced with the directory; the invariant and its reason are unchanged." }, @@ -98,15 +98,15 @@ }, { "file": "include/morph/core/bridge.hpp", - "line": 1453, + "line": 1542, "source": "if (_executeDeadline.count() > 0 && _timeoutScheduler) {", "reason": "Unreachable by construction (core audit finding B6). `setExecuteDeadline` (this file) is the only writer of both `_executeDeadline` and `_timeoutScheduler`, and always creates `_timeoutScheduler` in the same call that sets `_executeDeadline` positive (`_executeDeadline = deadline; if (_executeDeadline.count() > 0 && !_timeoutScheduler) { _timeoutScheduler = std::make_shared<...>(); }`, both under `_executeDeadlineMtx`); nothing anywhere resets `_timeoutScheduler` back to null -- the class's own doc comment on `setExecuteDeadline` says so explicitly (\"setting the deadline back to 0 stops new calls from arming it but does not tear the thread down\"). So `_executeDeadline > 0 && !_timeoutScheduler` cannot happen at this line once any positive deadline has ever been set." }, { "file": "include/morph/core/bridge.hpp", - "line": 1589, + "line": 1654, "source": "if (deadlineHandle && schedulerRef) {", - "reason": "Unreachable by construction, same joint-assignment shape as B6 above (core audit finding B11, reclassified (a)->(b) on review). `deadlineHandle` and `schedulerRef` are assigned together, a few lines above this one in `executeVia`, only inside `if (_executeDeadline.count() > 0 && _timeoutScheduler) { schedulerRef = _timeoutScheduler; ... }` (see the bridge.hpp:1453 entry above) -- there is no path that sets `deadlineHandle` without also having set `schedulerRef` from the same non-null `_timeoutScheduler` in the same conditional. So `schedulerRef` null while `deadlineHandle` is non-null cannot occur; the only theoretically-open arm this compound condition has is structurally impossible." + "reason": "Unreachable by construction, same joint-assignment shape as B6 above (core audit finding B11, reclassified (a)->(b) on review). `deadlineHandle` and `schedulerRef` are assigned together, a few lines above this one in `executeVia`, only inside `if (_executeDeadline.count() > 0 && _timeoutScheduler) { schedulerRef = _timeoutScheduler; ... }` (see the bridge.hpp:1542 entry above) -- there is no path that sets `deadlineHandle` without also having set `schedulerRef` from the same non-null `_timeoutScheduler` in the same conditional. So `schedulerRef` null while `deadlineHandle` is non-null cannot occur; the only theoretically-open arm this compound condition has is structurally impossible. This entry is specifically the exception-path use of the guard (the `catch` block that undoes `_pendingCalls` and cancels the deadline before rethrowing). Two more textually-identical `if (deadlineHandle && schedulerRef)` guards exist further down, in the `.then()`/`.onError()` continuations (lines 1678, 1767) -- present on master too, this PR only shifted their line numbers -- which are a different guard on a different, reachable arm (schedulerRef going null between capture and a callback that can run after `~Bridge()`) and are not covered by this disposition." }, { "file": "include/morph/core/remote.hpp", diff --git a/scripts/mutation_survivors.json b/scripts/mutation_survivors.json index 0122e40ea..9d1a6a7cf 100644 --- a/scripts/mutation_survivors.json +++ b/scripts/mutation_survivors.json @@ -56,7 +56,7 @@ }, { "file": "include/morph/core/bridge.hpp", - "line": 121, + "line": 122, "mutants": 3, "mutator": "cxx_add_to_sub", "source": "return modelHash ^ (key.sharing.hash_code() + 0x9e3779b9U + (modelHash << 6) + (modelHash >> 2));", @@ -64,7 +64,7 @@ }, { "file": "include/morph/core/backend.hpp", - "line": 1228, + "line": 1322, "mutants": 1, "mutator": "cxx_replace_scalar_call", "source": "aware.reserve(_changeAware.size());", diff --git a/src/qt/qt_websocket_backend.cpp b/src/qt/qt_websocket_backend.cpp index d24bad8bd..f213c587f 100644 --- a/src/qt/qt_websocket_backend.cpp +++ b/src/qt/qt_websocket_backend.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include #include @@ -45,10 +46,10 @@ QtWebSocketBackend::QtWebSocketBackend(QUrl serverUrl, ::morph::model::detail::A if (_connectHandler) { _connectHandler(); } - // Send every registerModelAsync request that arrived before this - // connect (the first connect included) -- see issue #54. Runs before - // the reconnect handler below so a caller that gates UI on - // onRegistered sees it fire promptly on first connect too. + // Send every bind request that arrived before this connect (the first + // connect included) -- see issue #54. Runs before the reconnect handler + // below so a caller that gates UI on the bind's continuation sees it + // fire promptly on first connect too. flushQueuedRegistrations(); // Fire the reconnect handler only on subsequent connects, never on the // first one — initial registration is handled by the BridgeHandler ctors. @@ -138,8 +139,14 @@ std::string QtWebSocketBackend::sendSync(const std::string& msg) { } ::morph::exec::detail::ModelId QtWebSocketBackend::registerModel( - const std::string& typeId, std::function()> /*factory*/) { - auto env = ::morph::wire::makeRegister(typeId); + const std::string& typeId, std::function()> factory) { + return registerModelWithContext(typeId, std::move(factory), {}); +} + +::morph::exec::detail::ModelId QtWebSocketBackend::registerModelWithContext( + const std::string& typeId, std::function()> /*factory*/, + std::string_view contextKey) { + auto env = ::morph::wire::makeRegister(typeId, std::string{contextKey}); env.session = _session; std::string replyJson; try { @@ -157,57 +164,97 @@ ::morph::exec::detail::ModelId QtWebSocketBackend::registerModel( throw std::runtime_error("register failed: " + reply.message); } -bool QtWebSocketBackend::registerModelAsync( - const std::string& typeId, std::function()> /*factory*/, - std::string_view contextKey, std::function onRegistered, - std::function onError) { +::morph::async::Completion<::morph::exec::detail::ModelId> QtWebSocketBackend::bindModel( + ::morph::backend::detail::BindRequest request, ::morph::exec::IExecutor& cbExec) { if (!_cfg.asyncRegistrationEnabled) { - // Opt-in only (see QtWebSocketBackendConfig::asyncRegistrationEnabled): - // returning false here makes Bridge::registerHandler() fall back to - // the synchronous registerModel(), preserving every existing - // embedder's behavior unless it explicitly asks for the async path. - return false; + // Blocking by request (see QtWebSocketBackendConfig::asyncRegistrationEnabled): + // IBackend's default runs the synchronous verb this request's shape + // names and settles from this thread, preserving every existing + // embedder's behaviour unless it explicitly asks for the non-blocking + // path. + return ::morph::backend::detail::IBackend::bindModel(std::move(request), cbExec); } - if (!_connected) { - // Queue rather than fail: this is exactly the ordering a - // single-threaded WASM client must use, since it can never block - // waiting for the connection to settle (see issue #54). The queued - // request is sent -- with a call-id assigned then, not now -- the - // moment `connected` fires next (first connect included), from - // flushQueuedRegistrations(). No call-id is assigned yet; if the - // backend is torn down (or the socket disconnects) before that - // happens, cancelPending() drains this queue too and still invokes - // onError exactly once. - std::scoped_lock const lock{_pendingMtx}; - _queuedRegistrations.push_back(QueuedRegistration{.typeId = typeId, - .contextKey = std::string{contextKey}, - .onRegistered = std::move(onRegistered), - .onError = std::move(onError)}); - return true; + + auto [completion, promise] = ::morph::async::Completion<::morph::exec::detail::ModelId>::makeSettleable(&cbExec); + + if (request.primary.empty()) { + // Mirrors the synchronous attachModel's empty-primary branch: release + // the instance currently held (fire-and-forget, as deregisterModel + // already is) and degrade to a private registration. + if (request.current.v != 0U) { + deregisterModel(request.current); + } + if (!_connected) { + // Queue rather than fail: this is exactly the ordering a + // single-threaded WASM client must use, since it can never block + // waiting for the connection to settle (see issue #54). The queued + // request is sent -- with a call-id assigned then, not now -- the + // moment `connected` fires next (first connect included), from + // flushQueuedRegistrations(). No call-id is assigned yet; if the + // backend is torn down (or the socket disconnects) before that + // happens, cancelPending() drains this queue too and still settles + // the promise exactly once. + std::scoped_lock const lock{_pendingMtx}; + _queuedRegistrations.push_back(QueuedRegistration{.typeId = std::move(request.typeId), + .contextKey = std::move(request.contextKey), + .promise = std::move(promise)}); + return std::move(completion); + } + sendControl(::morph::wire::makeRegister(request.typeId, request.contextKey), std::move(promise)); + return std::move(completion); } - sendRegisterAsync(typeId, contextKey, std::move(onRegistered), std::move(onError)); - return true; + + if (!_connected) { + // A keyed bind carries no queue: unlike a private registration it may + // be a re-point of a live instance, and replaying that against a + // connection that has since been re-established would attach from a + // `current` the new connection never issued. + promise.reject(std::make_exception_ptr(std::runtime_error{"disconnected"})); + return std::move(completion); + } + + // `registerShared` for a first bind, `attach` when re-pointing from a live + // instance -- the same two envelopes registerModelShared/attachModel build, + // selected by the same field of the request that IBackend::bindModelBlocking + // selects the synchronous verb by. + auto env = request.current.v == 0U + ? ::morph::wire::makeRegisterShared(request.typeId, request.primary, request.contextKey) + : ::morph::wire::makeAttach(request.typeId, request.primary, request.current.v, request.contextKey); + sendControl(std::move(env), std::move(promise)); + return std::move(completion); } -void QtWebSocketBackend::sendRegisterAsync(const std::string& typeId, std::string_view contextKey, - std::function onRegistered, - std::function onError) { +void QtWebSocketBackend::sendControl(::morph::wire::Envelope env, + ::morph::async::Completion<::morph::exec::detail::ModelId>::Promise promise) { uint64_t const callId = ++_nextCallId; - auto env = ::morph::wire::makeRegister(typeId, std::string{contextKey}); env.callId = callId; + // Stamped exactly as every synchronous control verb stamps it: RemoteServer + // authenticates and authorizes from env.session, so omitting it reached the + // server as an unauthenticated principal on the non-blocking (WASM) path + // only. morph#495 -- and now unmissable, because this is the only place a + // control envelope is sent from. env.session = _session; - // Encoded before the map insertion below: wire::encode() can throw on - // serialization failure, and a throw after inserting would leave this - // callId's onRegistered/onError parked in _pendingRegistrations forever, - // waiting for a reply to a message that was never sent -- nothing erases - // an entry whose send never happened. Encoding first means a throw here - // propagates to the caller (Bridge::registerHandlerImpl et al. already - // handle it) with nothing to clean up. - auto const encoded = QString::fromStdString(::morph::wire::encode(env)); + QString encoded; + try { + // Encoded before the map insertion below: wire::encode() can throw on + // serialization failure, and a throw after inserting would leave this + // callId's promise parked in _pendingRegistrations forever, waiting for + // a reply to a message that was never sent -- nothing erases an entry + // whose send never happened. + encoded = QString::fromStdString(::morph::wire::encode(env)); + } catch (...) { + // Rejected rather than rethrown: `bindModel`/`promoteModel` promise + // their caller exactly one failure channel, the returned `Completion` + // (`IBackend::bindModel`). Rethrowing would give a dispatch-time + // failure a second one, which is what the legacy `*Async` verbs did + // and what every call site then had to carry a `catch` for. Nothing + // has been recorded at this point, so there is nothing to unwind. + promise.reject(std::current_exception()); + return; + } { std::scoped_lock const lock{_pendingMtx}; - _pendingRegistrations[callId] = - PendingRegistration{.onRegistered = std::move(onRegistered), .onError = std::move(onError)}; + _pendingRegistrations.insert_or_assign(callId, std::move(promise)); } _socket.sendTextMessage(encoded); } @@ -219,86 +266,8 @@ void QtWebSocketBackend::flushQueuedRegistrations() { queued.swap(_queuedRegistrations); } for (auto& entry : queued) { - sendRegisterAsync(entry.typeId, entry.contextKey, std::move(entry.onRegistered), std::move(entry.onError)); - } -} - -bool QtWebSocketBackend::registerModelSharedAsync( - const std::string& typeId, std::function()> /*factory*/, - ::morph::backend::detail::InstanceIdentity identity, - std::function onRegistered, - std::function onError) { - if (!_cfg.asyncRegistrationEnabled) { - return false; - } - if (identity.primary.empty()) { - // Degrades to the private (non-shared) path, exactly like the - // synchronous registerModelShared below -- and that path already - // has an async form: this class's own registerModelAsync. - return registerModelAsync(typeId, nullptr, identity.contextKey, std::move(onRegistered), std::move(onError)); - } - if (!_connected) { - onError("disconnected"); - return true; - } - uint64_t const callId = ++_nextCallId; - auto env = - ::morph::wire::makeRegisterShared(typeId, std::string{identity.primary}, std::string{identity.contextKey}); - env.callId = callId; - // Same stamp the synchronous registerModelShared applies: RemoteServer - // authenticates and authorizes from env.session, so omitting it here reached - // the server as an unauthenticated principal on the async (WASM) path only. - // morph#495. - env.session = _session; - // See registerModelAsync's identical comment: encoded before the map - // insertion, so a throwing encode() cannot orphan a pending entry. - auto const encoded = QString::fromStdString(::morph::wire::encode(env)); - { - std::scoped_lock const lock{_pendingMtx}; - _pendingRegistrations[callId] = - PendingRegistration{.onRegistered = std::move(onRegistered), .onError = std::move(onError)}; - } - _socket.sendTextMessage(encoded); - return true; -} - -bool QtWebSocketBackend::attachModelAsync( - const std::string& typeId, std::function()> /*factory*/, - ::morph::backend::detail::InstanceIdentity identity, ::morph::exec::detail::ModelId current, - std::function onRegistered, - std::function onError) { - if (!_cfg.asyncRegistrationEnabled) { - return false; - } - if (identity.primary.empty()) { - // Mirrors the synchronous attachModel's empty-primary branch: release - // the current instance (fire-and-forget, as deregisterModel already - // is) and degrade to a private async registration. - if (current.v != 0U) { - deregisterModel(current); - } - return registerModelAsync(typeId, nullptr, identity.contextKey, std::move(onRegistered), std::move(onError)); - } - if (!_connected) { - onError("disconnected"); - return true; - } - uint64_t const callId = ++_nextCallId; - auto env = - ::morph::wire::makeAttach(typeId, std::string{identity.primary}, current.v, std::string{identity.contextKey}); - env.callId = callId; - // See registerModelSharedAsync above: stamped for the same reason (morph#495). - env.session = _session; - // See registerModelAsync's identical comment: encoded before the map - // insertion, so a throwing encode() cannot orphan a pending entry. - auto const encoded = QString::fromStdString(::morph::wire::encode(env)); - { - std::scoped_lock const lock{_pendingMtx}; - _pendingRegistrations[callId] = - PendingRegistration{.onRegistered = std::move(onRegistered), .onError = std::move(onError)}; + sendControl(::morph::wire::makeRegister(entry.typeId, entry.contextKey), std::move(entry.promise)); } - _socket.sendTextMessage(encoded); - return true; } ::morph::wire::ProtocolNegotiationResult QtWebSocketBackend::negotiateProtocolVersion() { @@ -365,40 +334,23 @@ void QtWebSocketBackend::assignPrimary(::morph::exec::detail::ModelId mid, const (void)modelIdFromReply(sendSync(::morph::wire::encode(env)), "assign"); } -bool QtWebSocketBackend::assignPrimaryAsync(::morph::exec::detail::ModelId mid, const std::string& typeId, - std::string_view primary, - std::function onRegistered, - std::function onError) { - if (primary.empty() || mid.v == 0U) { +::morph::async::Completion<::morph::exec::detail::ModelId> QtWebSocketBackend::promoteModel( + ::morph::backend::detail::PromoteRequest request, ::morph::exec::IExecutor& cbExec) { + auto [completion, promise] = ::morph::async::Completion<::morph::exec::detail::ModelId>::makeSettleable(&cbExec); + + if (request.primary.empty() || request.mid.v == 0U) { // Same no-op contract as the synchronous assignPrimary: nothing to - // promote. Resolve onRegistered as a no-op success, echoing mid back, - // rather than treating it as a failure. - onRegistered(mid); - return true; + // promote. Resolve, echoing mid back, rather than treating it as a + // failure. + promise.resolve(request.mid); + return std::move(completion); } if (!_connected) { - onError("disconnected"); - return true; + promise.reject(std::make_exception_ptr(std::runtime_error{"disconnected"})); + return std::move(completion); } - uint64_t const callId = ++_nextCallId; - auto env = ::morph::wire::makeAssign(typeId, std::string{primary}, mid.v); - env.callId = callId; - // Same stamp the synchronous assignPrimary applies -- RemoteServer authorizes - // from env.session, so an unstamped assign reached the server as an - // unauthenticated principal on the async (WASM) path only (morph#495). - env.session = _session; - // Encoded before the map insertion below, the invariant registerModelAsync - // states and the other two async hooks already follow: wire::encode() can - // throw, and a throw after inserting would park this callId's - // onRegistered/onError in _pendingAssigns forever with no message sent. - auto const encoded = QString::fromStdString(::morph::wire::encode(env)); - { - std::scoped_lock const lock{_pendingMtx}; - _pendingAssigns[callId] = - PendingAssign{.onRegistered = std::move(onRegistered), .onError = std::move(onError)}; - } - _socket.sendTextMessage(encoded); - return true; + sendControl(::morph::wire::makeAssign(request.typeId, request.primary, request.mid.v), std::move(promise)); + return std::move(completion); } std::vector QtWebSocketBackend::listInstances(const std::string& typeId) { @@ -472,15 +424,14 @@ ::morph::async::Completion> QtWebSocketBackend::execute( void QtWebSocketBackend::cancelPending(const std::exception_ptr& exc) { std::unordered_map drainedExecutes; - std::unordered_map drainedRegistrations; + std::unordered_map::Promise> + drainedRegistrations; std::vector drainedQueue; - std::unordered_map drainedAssigns; { std::scoped_lock const lock{_pendingMtx}; drainedExecutes.swap(_pending); drainedRegistrations.swap(_pendingRegistrations); drainedQueue.swap(_queuedRegistrations); - drainedAssigns.swap(_pendingAssigns); // _pendingDeregisters tracks fire-and-forget requests nobody awaits -- // just drop the bookkeeping, there is no callback to invoke. _pendingDeregisters.clear(); @@ -490,41 +441,22 @@ void QtWebSocketBackend::cancelPending(const std::exception_ptr& exc) { pending.state->setException(exc); } } - // Every caller passes a `make_exception_ptr`, so `rethrow_exception` always - // throws and one of the two handlers always assigns `message`. - std::string message; - try { - std::rethrow_exception(exc); - } catch (const std::exception& concrete) { - message = concrete.what(); - } catch (...) { - // A non-std::exception carries no portable message, so report the - // disconnect itself -- which is what a caller can act on anyway. Doing - // it here rather than as an initializer above keeps the handler from - // being lexically empty, which `bugprone-empty-catch` rejects however - // well the intent is commented (morph#514). - message = "disconnected"; - } - for (auto& [ignoredCallId, pending] : drainedRegistrations) { - if (pending.onError) { - pending.onError(message); - } - } - // A registerModelAsync request queued while the socket had never yet - // connected (issue #54) never got a call-id, so it cannot be found in - // _pendingRegistrations above -- drain it here instead, on the same - // cancelPending path that already handles a connection that goes away - // (or never comes up) before a queued reply, so its onError still fires - // exactly once rather than leaving the caller waiting forever. + for (auto& [ignoredCallId, promise] : drainedRegistrations) { + // The exception itself, not a message rebuilt from it: a control call + // rejected by a dropped socket now delivers the very + // `backend::DisconnectedError` an execute() call delivers, instead of + // the `runtime_error` the `*Async` verbs' string channel flattened it + // into. + promise.reject(exc); + } + // A private bind queued while the socket had never yet connected (issue + // #54) never got a call-id, so it cannot be found in _pendingRegistrations + // above -- drain it here instead, on the same cancelPending path that + // already handles a connection that goes away (or never comes up) before a + // queued reply, so its continuation still fires exactly once rather than + // leaving the caller waiting forever. for (auto& entry : drainedQueue) { - if (entry.onError) { - entry.onError(message); - } - } - for (auto& [ignoredCallId, pending] : drainedAssigns) { - if (pending.onError) { - pending.onError(message); - } + entry.promise.reject(exc); } } @@ -619,42 +551,26 @@ bool QtWebSocketBackend::tryRouteExecuteReply(const ::morph::wire::Envelope& env return true; } -bool QtWebSocketBackend::tryRouteRegistrationReply(const ::morph::wire::Envelope& env) { - PendingRegistration regPending; +bool QtWebSocketBackend::tryRouteControlReply(const ::morph::wire::Envelope& env) { + std::optional<::morph::async::Completion<::morph::exec::detail::ModelId>::Promise> promise; { std::scoped_lock const lock{_pendingMtx}; auto iter = _pendingRegistrations.find(env.callId); if (iter == _pendingRegistrations.end()) { return false; } - regPending = std::move(iter->second); + promise.emplace(std::move(iter->second)); _pendingRegistrations.erase(iter); } + // Settled outside `_pendingMtx`: settling runs the caller's continuation on + // whatever executor it named, and an executor that runs inline (which is + // what `Bridge` names) would then re-enter this backend under a lock this + // frame still holds. if (env.kind == "ok") { - regPending.onRegistered(::morph::exec::detail::ModelId{env.modelId}); + promise->resolve(::morph::exec::detail::ModelId{env.modelId}); } else { - regPending.onError(env.message); - } - return true; -} - -bool QtWebSocketBackend::tryRouteAssignReply(const ::morph::wire::Envelope& env) { - PendingAssign assignPending; - { - std::scoped_lock const lock{_pendingMtx}; - auto iter = _pendingAssigns.find(env.callId); - if (iter == _pendingAssigns.end()) { - return false; - } - assignPending = std::move(iter->second); - _pendingAssigns.erase(iter); - } - - if (env.kind == "ok") { - assignPending.onRegistered(::morph::exec::detail::ModelId{env.modelId}); - } else { - assignPending.onError(env.message); + promise->reject(std::make_exception_ptr(std::runtime_error{env.message})); } return true; } @@ -678,10 +594,7 @@ void QtWebSocketBackend::routeKeyedReply(const ::morph::wire::Envelope& env) { if (tryRouteExecuteReply(env)) { return; } - if (tryRouteRegistrationReply(env)) { - return; - } - if (tryRouteAssignReply(env)) { + if (tryRouteControlReply(env)) { return; } // No map matched (the deregister drop included): an already-resolved or @@ -700,7 +613,7 @@ void QtWebSocketBackend::onTextMessage(const QString& message) { return; } - // Async execute replies and async registerModelAsync replies both carry a + // Execute replies and non-blocking bind/promote replies both carry a // non-zero callId (the two share one counter/namespace, but land in // separate maps below since their reply shapes differ); sync replies // (registerModel/deregister/etc's sendSync calls) carry callId == 0 and diff --git a/tests/qt/test_qt_websocket.cpp b/tests/qt/test_qt_websocket.cpp index f5b6169b0..0812bb478 100644 --- a/tests/qt/test_qt_websocket.cpp +++ b/tests/qt/test_qt_websocket.cpp @@ -23,6 +23,7 @@ #include #include #include +#include #include #include #include @@ -105,6 +106,56 @@ static void pumpUntil(const std::function& done, int maxIterations = 50) QCoreApplication::sendPostedEvents(nullptr, QEvent::DeferredDelete); } +// ── Structural-registration-surface helpers ────────────────────────────────── +// +// The tests below drive `bindModel`/`promoteModel` directly. They need to +// distinguish "the continuation was handed to my executor" from "the +// continuation ran", which is precisely the distinction the surface exists to +// make: a backend that settled inline would collapse the two. +namespace { + +/// @brief `MainThreadExecutor` that counts what has been handed to it. +/// +/// `posted` rises when the backend settles; the queued task only runs on +/// `drain()`. A backend that ignored `cbExec` and invoked the continuation +/// itself would leave `posted` at zero. +struct CountingExecutor : morph::exec::MainThreadExecutor { + std::atomic posted{0}; + + void post(std::function task) override { + posted.fetch_add(1); + morph::exec::MainThreadExecutor::post(std::move(task)); + } +}; + +/// @brief Outcome of one `bindModel`/`promoteModel` call, as the test sees it. +struct ControlOutcome { + std::atomic modelId{0}; + std::string failure; + std::atomic settled{false}; +}; + +/// @brief Attaches @p outcome's handlers to @p completion. +void observe(morph::async::Completion& completion, ControlOutcome& outcome) { + completion + .then([&outcome](morph::exec::detail::ModelId mid) { + outcome.modelId.store(mid.v); + outcome.settled.store(true); + }) + .onError([&outcome](const std::exception_ptr& err) { + try { + std::rethrow_exception(err); + } catch (const std::exception& exc) { + outcome.failure = exc.what(); + } catch (...) { + outcome.failure = "unknown"; + } + outcome.settled.store(true); + }); +} + +} // namespace + // ── TLS config helpers ─────────────────────────────────────────────────────── static QSslConfiguration makeServerTlsConfig() { QFile certFile{QStringLiteral(TESTS_CERTS_DIR "/server.crt")}; @@ -233,7 +284,7 @@ TEST_CASE("morph::qt::QtWebSocketBackend::notifyBackendChanged is a documented n } TEST_CASE( - "morph::qt::QtWebSocketBackend: registerModelAsync (opt-in via Config::asyncRegistrationEnabled) registers " + "morph::qt::QtWebSocketBackend: bindModel (non-blocking via Config::asyncRegistrationEnabled) registers " "without blocking", "[qt][ws][issue26]") { ensureApp(); @@ -258,7 +309,7 @@ TEST_CASE( // Registration does not block: registerHandler() already returned above, // yet the binding is still unbound -- this is the whole point of the - // async path (see IBackend::registerModelAsync's doc comment). A real + // non-blocking path (see IBackend::bindModel's doc comment). A real // WASM caller would gate its UI on this instead of firing an action // immediately, since executeVia fails fast on an unbound binding. CHECK(binding->currentId.load() == 0U); @@ -358,7 +409,7 @@ TEST_CASE( #endif TEST_CASE( - "morph::qt::QtWebSocketBackend: registerModelAsync's pending registration is cancelled when the connection " + "morph::qt::QtWebSocketBackend: bindModel's pending registration is cancelled when the connection " "drops before a reply arrives", "[qt][ws][issue26]") { ensureApp(); @@ -391,13 +442,14 @@ TEST_CASE( CHECK(binding->currentId.load() == 0U); // onRegistered never fired; still safely unbound } -// ── The shared/keyed async wire methods ────────────────────────────────────── -// Same opt-in gate and same callId-keyed reply routing as registerModelAsync -// above; these drive them against a real RemoteServer, end to end. +// ── The keyed shapes of the structural surface ─────────────────────────────── +// `bindModel` with a non-empty `primary` is the register-or-attach the +// `registerModelShared`/`attachModel` pair used to spell as two verbs; these +// drive it against a real RemoteServer, end to end. // NOLINTNEXTLINE(readability-function-cognitive-complexity) -TEST_CASE("morph::qt::QtWebSocketBackend: registerModelSharedAsync registers-or-attaches without blocking", - "[qt][ws][issue26][shared-instances]") { +TEST_CASE("morph::qt::QtWebSocketBackend: bindModel with a primary registers-or-attaches without blocking", + "[qt][ws][issue26][shared-instances][morph568]") { ensureApp(); morph::exec::ThreadPoolExecutor serverPool{2}; auto server = std::make_shared(serverPool); @@ -410,19 +462,22 @@ TEST_CASE("morph::qt::QtWebSocketBackend: registerModelSharedAsync registers-or- morph::qt::QtWebSocketBackend::Config{.asyncRegistrationEnabled = true}}; REQUIRE(backend.waitForConnected()); - std::atomic registered{0}; - std::string failure; - REQUIRE(backend.registerModelSharedAsync( - "WsEchoModel", nullptr, {.contextKey = "acct-1", .primary = "acct-1"}, - [&](morph::exec::detail::ModelId mid) { registered.store(mid.v); }, - [&](const std::string& message) { failure = message; })); + morph::exec::MainThreadExecutor cbExec; + ControlOutcome first; + auto firstBind = backend.bindModel( + {.typeId = "WsEchoModel", .factory = nullptr, .contextKey = "acct-1", .primary = "acct-1", .current = {}}, + cbExec); + observe(firstBind, first); - // Returned true without waiting for the reply: nothing has arrived yet. - CHECK(registered.load() == 0U); + // Returned without waiting for the reply: nothing has arrived yet. + CHECK(first.modelId.load() == 0U); - pumpUntil([&] { return registered.load() != 0U || !failure.empty(); }); - CHECK(failure.empty()); - REQUIRE(registered.load() != 0U); + pumpUntil([&] { + cbExec.drain(); + return first.settled.load(); + }); + CHECK(first.failure.empty()); + REQUIRE(first.modelId.load() != 0U); // It really went out as a *shared* register, not a private one: the key is // now in the server's instance directory. @@ -430,21 +485,24 @@ TEST_CASE("morph::qt::QtWebSocketBackend: registerModelSharedAsync registers-or- REQUIRE(keys.size() == 1); CHECK(keys.front() == "acct-1"); - // A second shared register for the same key joins the same instance rather - // than creating a second one -- the register-or-attach half of the name. - std::atomic second{0}; - REQUIRE(backend.registerModelSharedAsync( - "WsEchoModel", nullptr, {.contextKey = "acct-1", .primary = "acct-1"}, - [&](morph::exec::detail::ModelId mid) { second.store(mid.v); }, - [&](const std::string& message) { failure = message; })); - pumpUntil([&] { return second.load() != 0U || !failure.empty(); }); - CHECK(failure.empty()); - CHECK(second.load() == registered.load()); + // A second bind for the same key joins the same instance rather than + // creating a second one -- the register-or-attach half of the shape. + ControlOutcome second; + auto secondBind = backend.bindModel( + {.typeId = "WsEchoModel", .factory = nullptr, .contextKey = "acct-1", .primary = "acct-1", .current = {}}, + cbExec); + observe(secondBind, second); + pumpUntil([&] { + cbExec.drain(); + return second.settled.load(); + }); + CHECK(second.failure.empty()); + CHECK(second.modelId.load() == first.modelId.load()); } // NOLINTNEXTLINE(readability-function-cognitive-complexity) -TEST_CASE("morph::qt::QtWebSocketBackend: attachModelAsync joins the existing shared instance without blocking", - "[qt][ws][issue26][shared-instances]") { +TEST_CASE("morph::qt::QtWebSocketBackend: bindModel re-points from a live instance without blocking", + "[qt][ws][issue26][shared-instances][morph568]") { ensureApp(); morph::exec::ThreadPoolExecutor serverPool{2}; auto server = std::make_shared(serverPool); @@ -457,40 +515,49 @@ TEST_CASE("morph::qt::QtWebSocketBackend: attachModelAsync joins the existing sh morph::qt::QtWebSocketBackend::Config{.asyncRegistrationEnabled = true}}; REQUIRE(backend.waitForConnected()); - // Seed the directory synchronously, so the async attach below has something - // to join and its reply can be compared against a known id. + // Seed the directory synchronously, so the bind below has something to join + // and its reply can be compared against a known id. auto const seeded = backend.registerModelShared("WsEchoModel", nullptr, {.contextKey = "acct-7", .primary = "acct-7"}); REQUIRE(seeded.v != 0U); - std::atomic attached{0}; - std::string failure; - REQUIRE(backend.attachModelAsync( - "WsEchoModel", nullptr, {.contextKey = "acct-7", .primary = "acct-7"}, morph::exec::detail::ModelId{0}, - [&](morph::exec::detail::ModelId mid) { attached.store(mid.v); }, - [&](const std::string& message) { failure = message; })); - CHECK(attached.load() == 0U); // the reply has not arrived yet - - pumpUntil([&] { return attached.load() != 0U || !failure.empty(); }); - CHECK(failure.empty()); - CHECK(attached.load() == seeded.v); - - // Re-pointing to a different key gets a different instance, still async. - std::atomic repointed{0}; - REQUIRE(backend.attachModelAsync( - "WsEchoModel", nullptr, {.contextKey = "acct-8", .primary = "acct-8"}, - morph::exec::detail::ModelId{attached.load()}, - [&](morph::exec::detail::ModelId mid) { repointed.store(mid.v); }, - [&](const std::string& message) { failure = message; })); - pumpUntil([&] { return repointed.load() != 0U || !failure.empty(); }); - CHECK(failure.empty()); - REQUIRE(repointed.load() != 0U); - CHECK(repointed.load() != seeded.v); + morph::exec::MainThreadExecutor cbExec; + ControlOutcome joined; + auto join = backend.bindModel( + {.typeId = "WsEchoModel", .factory = nullptr, .contextKey = "acct-7", .primary = "acct-7", .current = {}}, + cbExec); + observe(join, joined); + CHECK(joined.modelId.load() == 0U); // the reply has not arrived yet + + pumpUntil([&] { + cbExec.drain(); + return joined.settled.load(); + }); + CHECK(joined.failure.empty()); + CHECK(joined.modelId.load() == seeded.v); + + // Re-pointing to a different key gets a different instance -- the `attach` + // envelope, selected by a non-zero `current`. + ControlOutcome repointed; + auto repoint = backend.bindModel({.typeId = "WsEchoModel", + .factory = nullptr, + .contextKey = "acct-8", + .primary = "acct-8", + .current = morph::exec::detail::ModelId{joined.modelId.load()}}, + cbExec); + observe(repoint, repointed); + pumpUntil([&] { + cbExec.drain(); + return repointed.settled.load(); + }); + CHECK(repointed.failure.empty()); + REQUIRE(repointed.modelId.load() != 0U); + CHECK(repointed.modelId.load() != seeded.v); } // NOLINTNEXTLINE(readability-function-cognitive-complexity) -TEST_CASE("morph::qt::QtWebSocketBackend: attachModelAsync with an empty primary degrades to a private registration", - "[qt][ws][issue26][shared-instances]") { +TEST_CASE("morph::qt::QtWebSocketBackend: bindModel with an empty primary is a private registration", + "[qt][ws][issue26][shared-instances][morph568]") { ensureApp(); morph::exec::ThreadPoolExecutor serverPool{2}; auto server = std::make_shared(serverPool); @@ -503,23 +570,25 @@ TEST_CASE("morph::qt::QtWebSocketBackend: attachModelAsync with an empty primary morph::qt::QtWebSocketBackend::Config{.asyncRegistrationEnabled = true}}; REQUIRE(backend.waitForConnected()); - std::atomic registered{0}; - std::string failure; - REQUIRE(backend.attachModelAsync( - "WsEchoModel", nullptr, {.contextKey = "ctx", .primary = ""}, morph::exec::detail::ModelId{0}, - [&](morph::exec::detail::ModelId mid) { registered.store(mid.v); }, - [&](const std::string& message) { failure = message; })); - - pumpUntil([&] { return registered.load() != 0U || !failure.empty(); }); - CHECK(failure.empty()); - REQUIRE(registered.load() != 0U); + morph::exec::MainThreadExecutor cbExec; + ControlOutcome outcome; + auto bind = backend.bindModel( + {.typeId = "WsEchoModel", .factory = nullptr, .contextKey = "ctx", .primary = "", .current = {}}, cbExec); + observe(bind, outcome); + + pumpUntil([&] { + cbExec.drain(); + return outcome.settled.load(); + }); + CHECK(outcome.failure.empty()); + REQUIRE(outcome.modelId.load() != 0U); // Private, exactly like the synchronous attachModel's own empty-primary // branch: nothing was filed in the shared directory. CHECK(backend.listInstances("WsEchoModel").empty()); } -TEST_CASE("morph::qt::QtWebSocketBackend: registerModelSharedAsync on a never-connected socket reports onError", - "[qt][ws][issue26][shared-instances][disconnect]") { +TEST_CASE("morph::qt::QtWebSocketBackend: a keyed bindModel on a never-connected socket rejects", + "[qt][ws][issue26][shared-instances][disconnect][morph568]") { ensureApp(); // Port 1 is reserved and never listening — the socket never reaches Connected. QUrl url{QString("ws://127.0.0.1:1")}; @@ -528,21 +597,116 @@ TEST_CASE("morph::qt::QtWebSocketBackend: registerModelSharedAsync on a never-co morph::qt::QtWebSocketBackend::Config{.asyncRegistrationEnabled = true}}; REQUIRE_FALSE(backend.waitForConnected(200)); - std::string failure; - std::atomic registered{0}; - // Accepts the request (returns true) and reports the failure through - // onError rather than blocking or throwing. Bridge::ensureBoundAsync - // tolerates this firing inline, from inside the call itself. - REQUIRE(backend.registerModelSharedAsync( - "WsEchoModel", nullptr, {.contextKey = "acct-1", .primary = "acct-1"}, - [&](morph::exec::detail::ModelId mid) { registered.store(mid.v); }, - [&](const std::string& message) { failure = message; })); - CHECK(registered.load() == 0U); - CHECK(failure == "disconnected"); + morph::exec::MainThreadExecutor cbExec; + ControlOutcome outcome; + // Reports the failure through the Completion rather than blocking or + // throwing: one failure channel, which is what the structural surface + // promises its caller. + auto bind = backend.bindModel( + {.typeId = "WsEchoModel", .factory = nullptr, .contextKey = "acct-1", .primary = "acct-1", .current = {}}, + cbExec); + observe(bind, outcome); + cbExec.drain(); + CHECK(outcome.modelId.load() == 0U); + CHECK(outcome.failure == "disconnected"); +} + +// ── The delivery thread, which is what morph#567 made structural ───────────── +// +// This is the first *production* backend on the surface, so this is the first +// test that the guarantee survives a real transport rather than a test double +// settling its own promise. `QtWebSocketBackend` settles from `onTextMessage`, +// on the Qt event-loop thread; the caller here names a different executor and +// must be the one that decides when the continuation runs. +// +// Written so it fails if the backend delivered inline: `posted` rises when the +// reply lands, and the continuation must still not have run at that point. +TEST_CASE( + "morph::qt::QtWebSocketBackend: bindModel delivers its continuation on the caller's executor, not the " + "thread it settles on", + "[qt][ws][morph568][threading]") { + ensureApp(); + morph::exec::ThreadPoolExecutor serverPool{2}; + auto server = std::make_shared(serverPool); + morph::qt::QtWebSocketServer wsServer{*server, 0}; + REQUIRE(wsServer.listen()); + + const QUrl url{QString("ws://127.0.0.1:%1").arg(wsServer.port())}; + morph::qt::QtWebSocketBackend backend{url, morph::model::detail::defaultDispatcher(), + morph::model::detail::defaultRegistry(), std::nullopt, + morph::qt::QtWebSocketBackend::Config{.asyncRegistrationEnabled = true}}; + REQUIRE(backend.waitForConnected()); + + CountingExecutor cbExec; + std::atomic ran{false}; + std::atomic seen{0}; + auto bind = backend.bindModel( + {.typeId = "WsEchoModel", .factory = nullptr, .contextKey = "ctx", .primary = "", .current = {}}, cbExec); + bind.then([&](morph::exec::detail::ModelId mid) { + seen.store(mid.v); + ran.store(true); + }) + .onError([&](const std::exception_ptr&) { ran.store(true); }); + + // Pump only the Qt loop: the reply arrives and the backend settles, which + // posts the continuation. Nothing drains `cbExec` here. + pumpUntil([&] { return cbExec.posted.load() != 0; }); + REQUIRE(cbExec.posted.load() != 0); // the reply really did land + CHECK_FALSE(ran.load()); // ... and the backend did not run it + + cbExec.drain(); + CHECK(ran.load()); + CHECK(seen.load() != 0U); +} + +// The promote half of the surface, direct rather than through Bridge. +TEST_CASE("morph::qt::QtWebSocketBackend: promoteModel files a live instance under a key without blocking", + "[qt][ws][shared-instances][morph568]") { + ensureApp(); + morph::exec::ThreadPoolExecutor serverPool{2}; + auto server = std::make_shared(serverPool); + morph::qt::QtWebSocketServer wsServer{*server, 0}; + REQUIRE(wsServer.listen()); + + const QUrl url{QString("ws://127.0.0.1:%1").arg(wsServer.port())}; + morph::qt::QtWebSocketBackend backend{url, morph::model::detail::defaultDispatcher(), + morph::model::detail::defaultRegistry(), std::nullopt, + morph::qt::QtWebSocketBackend::Config{.asyncRegistrationEnabled = true}}; + REQUIRE(backend.waitForConnected()); + + auto const anonymous = backend.registerModel("WsEchoModel", nullptr); + REQUIRE(anonymous.v != 0U); + REQUIRE(backend.listInstances("WsEchoModel").empty()); + + morph::exec::MainThreadExecutor cbExec; + ControlOutcome outcome; + auto promote = backend.promoteModel({.mid = anonymous, .typeId = "WsEchoModel", .primary = "acct-9"}, cbExec); + observe(promote, outcome); + CHECK_FALSE(outcome.settled.load()); // not blocked on the round trip + + pumpUntil([&] { + cbExec.drain(); + return outcome.settled.load(); + }); + CHECK(outcome.failure.empty()); + CHECK(outcome.modelId.load() == anonymous.v); + + auto const keys = backend.listInstances("WsEchoModel"); + REQUIRE(keys.size() == 1); + CHECK(keys.front() == "acct-9"); + + // The documented no-op cases resolve rather than reject, and send nothing. + ControlOutcome noop; + auto emptyKey = backend.promoteModel({.mid = anonymous, .typeId = "WsEchoModel", .primary = ""}, cbExec); + observe(emptyKey, noop); + cbExec.drain(); + CHECK(noop.settled.load()); + CHECK(noop.failure.empty()); + CHECK(noop.modelId.load() == anonymous.v); } TEST_CASE( - "morph::qt::QtWebSocketBackend: registerModelAsync called before the socket connects queues and retries once " + "morph::qt::QtWebSocketBackend: bindModel called before the socket connects queues and retries once " "connected fires", "[qt][ws][issue54]") { ensureApp(); @@ -565,7 +729,7 @@ TEST_CASE( auto binding = std::make_shared(); binding->typeId = "WsEchoModel"; binding->modelFactory = [] { return morph::model::detail::ModelFactory::create(); }; - // registerModelAsync is called here, before the socket has finished its + // bindModel is called here, before the socket has finished its // handshake. Previously this called onError("disconnected") immediately // and never retried -- currentId would stay 0 forever even once the // socket connects moments later. It must instead queue the attempt and @@ -2298,6 +2462,74 @@ TEST_CASE("Process separation: TLS handshake works across processes", "[qt][wss] REQUIRE(runClient(url, {QStringLiteral("--tls")}) == 0); } +// Coverage for morph#594: a *private* registration over this transport must +// carry `contextKey` to the server, exactly as `SimulatedRemoteBackend` and +// `morph::net::SocketBackend` (morph#587) do. +// +// The assertion is deliberately on the provider, not on the registration: +// registration succeeded before this was fixed too. `RemoteServer:: +// attachLogIfConfigured` returns *before* consulting the `LogProvider` when the +// envelope's `contextKey` is empty, so a dropped key does not produce an audit +// record missing a field — it produces no audit record at all, silently. +TEST_CASE("morph::qt::QtWebSocketBackend: a private registration carries contextKey to the server's log provider", + "[qt][ws][action_log]") { + ensureApp(); + morph::exec::ThreadPoolExecutor serverPool{2}; + auto server = std::make_shared(serverPool); + + // The provider runs on the server's own strand and the assertions on this + // thread; the mutex is what makes that handoff an ordinary handoff rather + // than a data race TSan will flag. + std::mutex providerMtx; + std::vector requestedFor; + auto log = std::make_shared(); + server->setLogProvider([&](std::string_view modelType, std::string_view contextKey) { + std::scoped_lock const lock{providerMtx}; + requestedFor.emplace_back(std::string{modelType} + ":" + std::string{contextKey}); + return log; + }); + + morph::qt::QtWebSocketServer wsServer{*server, 0}; + REQUIRE(wsServer.listen()); + + QUrl const url{QString("ws://127.0.0.1:%1").arg(wsServer.port())}; + morph::qt::QtWebSocketBackend backend{url}; + REQUIRE(backend.waitForConnected()); + + SECTION("through the blocking registerModelWithContext path") { + auto const mid = backend.registerModelWithContext("WsEchoModel", nullptr, "acct-594"); + REQUIRE(mid.v != 0U); + std::scoped_lock const lock{providerMtx}; + CHECK(requestedFor == std::vector{"WsEchoModel:acct-594"}); + } + + SECTION("and through the default bindModel, which is what Bridge::registerHandler reaches here") { + // `asyncRegistrationEnabled` is unset, so this is `IBackend::bindModel`'s + // default dispatching the empty-`primary`/zero-`current` shape to + // `registerModelWithContext` — the path a `Bridge` over this backend + // takes, and the one morph#594 reported as dropping the key. + morph::exec::detail::ModelId bound{}; + backend + .bindModel(morph::backend::detail::BindRequest{.typeId = "WsEchoModel", + .factory = nullptr, + .contextKey = "acct-bind", + .primary = {}, + .current = {}}, + morph::exec::detail::inlineExecutor()) + .thenDetached([&](morph::exec::detail::ModelId mid) { bound = mid; }); + REQUIRE(bound.v != 0U); + std::scoped_lock const lock{providerMtx}; + CHECK(requestedFor == std::vector{"WsEchoModel:acct-bind"}); + } + + SECTION("while plain registerModel still sends no key, so the provider is not consulted") { + auto const mid = backend.registerModel("WsEchoModel", nullptr); + REQUIRE(mid.v != 0U); + std::scoped_lock const lock{providerMtx}; + CHECK(requestedFor.empty()); + } +} + // ── Custom main: own the QCoreApplication explicitly ───────────────────────── // // Without this, `QCoreApplication` was a static local in `ensureApp()` and so @@ -2321,7 +2553,7 @@ int main(int argc, char* argv[]) { return result; } -// ── morph#495: the async control paths must stamp the session too ── +// ── morph#495: the non-blocking control paths must stamp the session too ── // // registerModelSharedAsync, attachModelAsync and assignPrimaryAsync each built // their envelope and encoded it with no `env.session = _session`, while all @@ -2329,7 +2561,10 @@ int main(int argc, char* argv[]) { // authorizes from env.session (remote.hpp: stampVerifiedPrincipal, and the // register/attach/assign authorization sites), so a client using the async path // -- which is the WASM path, and the only one a WASM main thread can use -- -// reached an authorizing server as an unauthenticated principal. +// reached an authorizing server as an unauthenticated principal. Those three +// verbs are gone (morph#568); `sendControl` is now the single place a control +// envelope is built, so the gap has one place left to reappear in -- and this +// test still guards it. // // A test asserting only "the async call succeeds" would have passed before the // fix, so this records what the *server* saw. @@ -2355,7 +2590,7 @@ struct RecordingAuthorizer : morph::session::IAuthorizer { }; } // namespace -TEST_CASE("morph::qt::QtWebSocketBackend: the async control envelopes carry the session", +TEST_CASE("morph::qt::QtWebSocketBackend: the non-blocking control envelopes carry the session", "[qt][ws][morph495][security]") { ensureApp(); morph::exec::ThreadPoolExecutor serverPool{2}; @@ -2376,20 +2611,23 @@ TEST_CASE("morph::qt::QtWebSocketBackend: the async control envelopes carry the session.token = "tok-495"; backend.setSession(session); - std::atomic registered{0}; - std::string failure; - REQUIRE(backend.registerModelSharedAsync( - "WsEchoModel", nullptr, {.contextKey = "acct-495", .primary = "acct-495"}, - [&](morph::exec::detail::ModelId mid) { registered.store(mid.v); }, - [&](const std::string& message) { failure = message; })); - pumpUntil([&] { return registered.load() != 0U || !failure.empty(); }); - CHECK(failure.empty()); - REQUIRE(registered.load() != 0U); + morph::exec::MainThreadExecutor cbExec; + ControlOutcome outcome; + auto bind = backend.bindModel( + {.typeId = "WsEchoModel", .factory = nullptr, .contextKey = "acct-495", .primary = "acct-495", .current = {}}, + cbExec); + observe(bind, outcome); + pumpUntil([&] { + cbExec.drain(); + return outcome.settled.load(); + }); + CHECK(outcome.failure.empty()); + REQUIRE(outcome.modelId.load() != 0U); std::scoped_lock const lock{authorizer->mtx}; REQUIRE_FALSE(authorizer->registerTokens.empty()); - // Before the fix this was "" -- registerModelSharedAsync built its envelope - // with no `env.session = _session`, so the server received a - // default-constructed session and could not authenticate the caller at all. + // Before the fix this was "" -- the shared register built its envelope with + // no `env.session = _session`, so the server received a default-constructed + // session and could not authenticate the caller at all. CHECK(authorizer->registerTokens.back() == "tok-495"); } diff --git a/tests/test_async_registration.cpp b/tests/test_async_registration.cpp index ee9f10300..1ee1b69f6 100644 --- a/tests/test_async_registration.cpp +++ b/tests/test_async_registration.cpp @@ -534,10 +534,20 @@ class AsyncAssignPrimaryBackend : public morph::backend::LocalBackend { // A backend with no async registration path at all (registerModelSharedAsync // defaults to `return false`, so ensureBoundAsync always falls back to the -// synchronous path), whose synchronous registerModelShared throws -- -// exercises ensureBoundAsync's own synchronous-fallback `catch (...)` -// (Task 15a finding B2), the ensureBoundAsync counterpart of -// attachHandlerAsync's identical-shaped fallback catch. +// blocking path), whose synchronous register throws -- exercises +// ensureBoundAsync's own fallback failure path (Task 15a finding B2), the +// ensureBoundAsync counterpart of attachHandlerAsync's identical-shaped one. +// +// Both verbs throw, not just registerModelShared. `ensureBoundAsync` asks for an +// *anonymous* instance -- `registerModelShared` with an empty `primary` -- and +// `IBackend::registerModelShared` documents that case as degrading to +// `registerModelWithContext`; every backend in the tree implements the degrade +// as its first statement. Since morph#568 the fallback goes through +// `IBackend::bindModel`, whose `BindRequest` names that shape directly +// (`primary` empty, `current` zero) and therefore reaches +// `registerModelWithContext`, so a double that threw only from +// `registerModelShared` would quietly stop failing and this test would pass by +// registering successfully instead of by surfacing a throw. class ThrowingSyncRegisterSharedBackend : public morph::backend::detail::IBackend { public: morph::exec::detail::ModelId registerModel( @@ -553,6 +563,12 @@ class ThrowingSyncRegisterSharedBackend : public morph::backend::detail::IBacken void notifyBackendChanged() override {} void cancelPending(const std::exception_ptr&) override {} + morph::exec::detail::ModelId registerModelWithContext( + const std::string&, std::function()>, + std::string_view) override { + throw std::runtime_error("registerModelShared failed synchronously"); + } + morph::exec::detail::ModelId registerModelShared( const std::string&, std::function()>, morph::backend::detail::InstanceIdentity) override { @@ -1862,13 +1878,15 @@ TEST_CASE( "(Task 15a finding B2)", "[bridge][registration][issue26]") { // Distinct from the test above: ThrowingDispatchBackend's throw comes from - // the ASYNC dispatch entry point itself (registerModelSharedAsync), before - // any fallback is even considered. ThrowingSyncRegisterSharedBackend + // the legacy ASYNC dispatch entry point itself (registerModelSharedAsync), + // before any fallback is even considered, and is caught by + // ensureBoundAsync's `catch (...)`. ThrowingSyncRegisterSharedBackend // instead offers no async path at all (registerModelSharedAsync's default - // `return false`), so ensureBoundAsync falls through to running the - // synchronous registerModelShared body under the same lock -- and that - // synchronous call is what throws here, exercising the fallback's own - // `catch (...)` (bridge.hpp's ensureBoundAsync, ~line 751). + // `return false`), so ensureBoundAsync falls through to `bindModel`, whose + // default runs the blocking register from inside the call. That throw is + // turned into a rejection by `IBackend::bindModel` rather than propagating + // -- one failure channel, the returned `Completion` -- and must still reach + // @p onDone, with the original exception rather than a stringified one. auto binding = std::make_shared(); binding->typeId = "AR_Model"; binding->modelFactory = [] { return morph::model::detail::ModelFactory::create(); }; diff --git a/tests/test_backend_registration_surface.cpp b/tests/test_backend_registration_surface.cpp index 244d32d23..eba4df775 100644 --- a/tests/test_backend_registration_surface.cpp +++ b/tests/test_backend_registration_surface.cpp @@ -27,6 +27,7 @@ #include #include #include +#include #include #include #include @@ -569,3 +570,112 @@ TEST_CASE( "cancelPending", "setReconnectHandler", "setConnectHandler", "setDisconnectHandler", "setSession:pal"}); } + +// ── `bindWaitPolicy`: the one bit `Completion` cannot carry (morph#593) ────── +// +// Two backends both return an unsettled `Completion` from `bindModel`, and +// `Bridge::registerHandler` — a synchronous entry point whose caller uses the +// handler on the next line — must wait for one and must not wait for the other. +// The pair of cases below pins both answers at the one call site that asks. +// +// The 200 ms reply delay is deliberate and is what makes each case *fail* under +// the opposite policy: it is far longer than the microseconds a non-waiting +// `registerHandler` takes to return, so "bound on return" cannot happen by luck +// and "unbound on return" cannot be a lost race. Neither assertion's passing +// direction depends on the exact value — the waiting case waits however long the +// reply takes, and the non-waiting case polls until it arrives. + +namespace { + +constexpr auto kBindReplyDelay = std::chrono::milliseconds{200}; + +/// @brief A backend whose bind reply arrives, later, from a thread the caller +/// does not own — `morph::net::SocketBackend`'s shape. +/// +/// Derives from `RecordingBackend` for the verbs `Bridge` needs it to have, but +/// answers `false` to `registerModelAsync` (`RecordingBackend` answers `true`) +/// so that `Bridge::registerHandlerImpl` reaches the structural surface rather +/// than the legacy async verb morph#571 removes. +struct TransportThreadBackend : RecordingBackend { + /// @brief Policy this double reports; the thing under test. + morph::backend::detail::BindWait policy = morph::backend::detail::BindWait::kCallerMayBlock; + /// @brief The "transport" that settles the bind, asserted to be another thread. + std::thread transport; + /// @brief Thread `bindModel` was called on. + std::thread::id callerThread; + /// @brief `true` if the completion settled somewhere other than `callerThread`. + std::atomic settledOffCallerThread{false}; + + TransportThreadBackend() = default; + TransportThreadBackend(const TransportThreadBackend&) = delete; + TransportThreadBackend& operator=(const TransportThreadBackend&) = delete; + TransportThreadBackend(TransportThreadBackend&&) = delete; + TransportThreadBackend& operator=(TransportThreadBackend&&) = delete; + + ~TransportThreadBackend() override { + if (transport.joinable()) { + transport.join(); + } + } + + bool registerModelAsync(const std::string& /*typeId*/, + std::function()> /*factory*/, + std::string_view /*contextKey*/, std::function /*onRegistered*/, + std::function /*onError*/) override { + return false; + } + + ModelCompletion bindModel(BindRequest /*request*/, morph::exec::IExecutor& cbExec) override { + auto [completion, promise] = ModelCompletion::makeSettleable(&cbExec); + callerThread = std::this_thread::get_id(); + transport = std::thread{[this, kept = std::make_shared(std::move(promise))] { + std::this_thread::sleep_for(kBindReplyDelay); + settledOffCallerThread.store(std::this_thread::get_id() != callerThread); + kept->resolve(ModelId{99}); + }}; + return std::move(completion); + } + + [[nodiscard]] morph::backend::detail::BindWait bindWaitPolicy() const noexcept override { return policy; } +}; + +} // namespace + +TEST_CASE("morph::bridge::Bridge: registerHandler waits out a kCallerMayBlock backend's bind and returns bound", + "[backend][registration-surface][bridge]") { + auto owned = std::make_unique(); + auto* backend = owned.get(); + backend->policy = morph::backend::detail::BindWait::kCallerMayBlock; + + morph::bridge::Bridge bridge{std::move(owned)}; + auto binding = bridge.registerHandler(); + + // No polling, no drain: the constructor did not return until the reply + // landed. This is the contract every non-Qt embedder had before morph#568 + // and that morph#586 took away from `SocketBackend`. + REQUIRE(morph::bridge::Bridge::isBound(binding)); + REQUIRE(binding->currentId.load() == 99U); + // ...and it was a *wait*, not a synchronous backend: the value was produced + // on a thread `registerHandler` does not own. + REQUIRE(backend->settledOffCallerThread.load()); +} + +TEST_CASE("morph::bridge::Bridge: registerHandler does not wait for a kCallerMustNotBlock backend", + "[backend][registration-surface][bridge]") { + auto owned = std::make_unique(); + auto* backend = owned.get(); + backend->policy = morph::backend::detail::BindWait::kCallerMustNotBlock; + + morph::bridge::Bridge bridge{std::move(owned)}; + auto binding = bridge.registerHandler(); + + // Returned while the reply is still 200 ms away. For `QtWebSocketBackend` + // under `asyncRegistrationEnabled` this is not a preference: the reply is + // delivered by the Qt event loop of this very thread, so a `registerHandler` + // that waited here would never return (morph#568's WASM page abort). + REQUIRE_FALSE(morph::bridge::Bridge::isBound(binding)); + + REQUIRE(morph::testing::waitUntil([&] { return morph::bridge::Bridge::isBound(binding); })); + REQUIRE(binding->currentId.load() == 99U); + REQUIRE(backend->settledOffCallerThread.load()); +}