Skip to content

core, net, build: executeWhenBound, a shared pollUntil, and find core-cpp before fetching (#841, #831, #840) - #844

Open
Yaraslaut wants to merge 3 commits into
masterfrom
lane/core-net-build
Open

Yaraslaut wants to merge 3 commits into
masterfrom
lane/core-net-build

Conversation

@Yaraslaut

Copy link
Copy Markdown
Member

Closes #841
Closes #831
Closes #840

Three tickets on one branch, one commit each. Everything below was measured on this branch (base 9d001052) on macOS arm64, AppleClang 17, Debug, unless it says otherwise.

#841: BridgeHandler::executeWhenBound(action) (commit bfe9963)

What changed

  • New BridgeHandler::executeWhenBound(action), shaped the way the triage comment recommends: a separately named method rather than a constructor policy. execute() still fails fast.
    • Bound: dispatches exactly as execute() does.
    • Registration in flight: chains on whenBound(), then dispatches on the GUI executor. A failed registration rejects with the registration's error. If whenBound() resolves false, it rejects with "handler not bound".
    • Handler destroyed first: the held action is dropped. It is never dispatched, and the returned Completion never settles.
    • Bridge retired before a queued dispatch runs: rejects with "bridge destroyed".
  • static_assert(!kShared): the method is NoSharing only, because on a shared handler whenBound() never waits. A second static_assert requires a copyable result, since the deferred path copies the value from one Completion to another, the same way the keyed branches of execute() do.
  • The continuation captures weak_ptr<HandlerBinding>, &_bridge, the BridgeLifetime gate, the executor and the state. It never captures this.
  • Specs: bridge.md gains an executeWhenBound() subsection under "Registration readiness", with a table and the reasoning for each choice, plus a row in the API table. backend.md's "does not queue" sentence now names the opt-in. The doc comment on qt_websocket_backend.hpp's asyncRegistrationEnabled mentions it too.

Verified

  • morph_tests "[executeWhenBound]": All tests passed (31 assertions in 5 test cases). The five cases sit next to the whenBound cases:
    • dispatch before the bind resolves after completeNext();
    • a failed registration reaches onError with its message;
    • a handler destroyed before the bind leaves nothing dispatched and neither callback fired;
    • a handler destroyed after the bind but before the queued dispatch runs (DeterministicExecutor) leaves nothing dispatched;
    • an already-bound handler dispatches straight away.
  • AsyncRegisterBackend gained an executeCount() so "nothing dispatched" is measured at the backend.
  • morph_tests "[registration]": All tests passed (1121 assertions in 61 test cases).
  • Mutation 1, executeWhenBound → return execute(std::move(action));: test cases: 5 | 1 passed | 4 failed. Only the already-bound case survives, as it should.
  • Mutation 2, the continuation captures _binding strongly: test cases: 5 | 3 passed | 2 failed. Both lifetime cases fail, and the destroyed-before-bind case reports executeCount() == 1 with resolved == true. The strong capture forms a cycle (binding → waiter → continuation → binding) that keeps an orphaned instance alive and dispatches onto it. That is why the capture is weak, and it is written down in bridge.md.

Not verified

  • No Qt/WASM run. executeWhenBound was exercised only against the AsyncRegisterBackend double.
  • The "bridge destroyed" arm has no test. Reaching it needs a bridge destroyed while a handler is still alive and a dispatch is queued, which the handler's own contract already forbids.

Review notes

  • The bridge gate is read and then released before executeVia. Holding it would mean a recursive shared_lock whenever the backend settles inline, because BridgeSink's settle path takes the same gate. So the check turns the ordinary "bridge retired while the dispatch sat in the GUI queue" into a rejection. It does not make concurrent bridge destruction safe, and the doc comment and spec both say so.
  • Of the six rows in the spec table, five are tested (the four failure modes plus the bound path); "bridge destroyed" is the gap noted above.

#831: detail::pollUntil, one bounded-wait helper (commit 3089d28)

What changed

  • morph::net::detail::pollUntil(fd, events, deadline) -> PollResult{PollOutcome::{kReady,kTimedOut,kFailed}, error} lives in tcp_socket.hpp. It is not public API.
    • It uses one deadline for the whole wait and recomputes the remaining budget after every EINTR.
    • It clamps each poll() call to int milliseconds.
    • It reports rather than throws, so every caller keeps its own error policy.
  • TcpSocket::connect: a timeout or a poll failure still closes the candidate and moves on to the next one.
  • waitReadableUntil (handshake): still throws, with the same two messages as before. It dropped <algorithm>, <cerrno> and <limits>, which it no longer uses.
  • FakeWsServer::acceptWithin (test): still throws. It also picks up the int clamp, which it lacked before.
  • Triage suggested the helper throw on a non-EINTR error. I went with a reported kFailed, so that connect can keep "try the next candidate" without a try/catch.

Verified

  • New [poll_until] cases in test_tcp_socket.cpp (All tests passed (13 assertions in 4 test cases)):
    • a deadline that has already passed;
    • an idle listener timing out (≥ 100 ms);
    • a pending connection reporting kReady;
    • a thread hit with SIGUSR1 every 50 ms for 1.5 s (no SA_RESTART) still ends at its 300 ms deadline.
  • Mutation A, EINTR treated as failure: the signal case fails with 2 == 1 (kFailed) and 56608750 ns >= 300 ms.
  • Mutation B, the full budget re-armed after EINTR: the signal case fails with 1968299500 ns < 1200 ms.
  • Full morph_net_tests: All tests passed (1137 assertions in 198 test cases).

Not verified

  • Linux was not run locally; CI covers it.
  • The signal test's upper bound is 1200 ms against an expected ~300 ms, which should hold on a loaded runner. The margin is the 900 ms gap: a re-arming loop cannot finish before the 1.5 s signal train ends.

#840: find core-cpp first, load CPM only to fetch (commit 1e5769e)

What changed

  • find_package(core-cpp ${MORPH_CORE_CPP_VERSION} CONFIG QUIET) now runs before CPMAddPackage. MORPH_CORE_CPP_VERSION (0.5) is the single place the bound is stated, and morphConfig.cmake.in uses it as well. That also fixes its comment, which said "0.3".
  • morph_use_cpm(<what> [<why>]) is a macro that includes cmake/CPM.cmake only if CPMAddPackage is not already a command. It is called at every fetch site: glaze, core-cpp, Catch2, docs/ (doxygen-awesome-css), and examples/common and examples/bank (Lightweight). The first site to load CPM prints morph: loading CPM to fetch <what>: <why>.
  • (a) The error message. cmake/CPM.cmake is still byte-identical to core-cpp's copy below its header. Only the header, which morph owns, changed: it now says the FATAL_ERROR's CORE_CPP_FETCH_DEPS=OFF / FetchTransferBound.cmake advice is core-cpp's, and gives what applies at morph's top level instead. The STATUS line printed just before the failure names the dependency and the remedy. I kept the body identical because the copy is documented as a verbatim mirror, so the next CPM pin bump stays a copy rather than a merge. The cost is that the misleading sentence is still printed; the line directly above it now says what to do.
  • (b) Sanitizers. Under AF_SANITIZER, morph skips find_package(core-cpp) and always builds it in-tree, so it can be instrumented. An empty CORE_CPP_TARGETS under a sanitizer is now a FATAL_ERROR.
  • (c) Install. A found core-cpp is not installed again. The CORE_CPP_INSTALL and EXCLUDE_FROM_ALL handling now lives only on the fetch path.
  • tests/CMakeLists.txt: the try_compile guard probes read an installed core-cpp's include directories from core::base. Before, they appended ${core-cpp_SOURCE_DIR}/src, which is empty for a found package.

Verified

A core-cpp and a glaze were built from the cached sources and installed into scratch prefixes. core-cpp used -DCORE_CPP_TESTING=OFF ... -DCORE_CPP_INSTALL=ON, installed both into prefix and separately into p-core; glaze went into p-glaze.

  • Offline, deps installed (branch): https_proxy=http://127.0.0.1:9 cmake -S . -B cfgA2 -G Ninja -DMORPH_BUILD_TESTS=OFF -DMORPH_BUILD_EXAMPLES=OFF -DCPM_SOURCE_CACHE=<empty dir> -DCMAKE_PREFIX_PATH="p-core;p-glaze" → exit 0, -- Build files have been written to. The output also includes CMake's own Manually-specified variables were not used by the project: CPM_SOURCE_CACHE, which is direct evidence that CPM never loaded. core-cpp_DIR and glaze_DIR point into the prefixes.
  • Same command on origin/master (git archive into a scratch dir, with the single prefix): exit 1, CMake Error at cmake/CPM.cmake:48 ... could not download the CPM.cmake bootstrap: 7;"Couldn't connect to server", reached from CMakeLists.txt:149 (include). So the check tells the two trees apart.
  • Offline, no deps (branch): exit 1. The output shows -- morph: loading CPM to fetch glaze: not found installed (install it and point CMAKE_PREFIX_PATH at it to configure without the network), then could not download the CPM.cmake bootstrap: 7;"Couldn't connect to server", then Configuring incomplete.
  • Sanitizer, deps installed: -DAF_SANITIZER=tsan -DCMAKE_PREFIX_PATH=prefix configures with -- morph: loading CPM to fetch core-cpp: AF_SANITIZER builds it in-tree so that it can be instrumented and CPM: Adding package core-cpp@0.5.0, and no core-cpp_DIR appears in the cache. -fsanitize=thread shows up on the core-cpp-* targets in build.ninja.
  • Sanitizer guard mutation (find_package allowed under a sanitizer): exit 1, AF_SANITIZER=tsan but core-cpp lists no targets to instrument (the CORE_CPP_TARGETS global property is empty) ....
  • Install: cmake --install cfgA2 --prefix morph-inst installs only lib/cmake/morph and headers, no core-cpp, and morphConfig.cmake has find_dependency(core-cpp 0.5 CONFIG). A scratch consumer (find_package(morph CONFIG REQUIRED), links morph::morph, constructs a Bridge over LocalBackend) configured, built and ran with exit 0 against morph-inst;prefix.
  • Tests on, core-cpp found (p-core;p-glaze, Catch2 found installed): configure exit 0, and the morph_client_only_guard_links probe built and ran with exit 0. The same configure with master's tests/CMakeLists.txt: exit 1, MORPH_CLIENT_ONLY guard check failed ... the linker did not name — so the tests/ change is load-bearing. With a single shared prefix the old file passes by accident, because glaze's include dir is the same prefix/include.
  • Default fetch path (this build dir, tests + net on): reconfigures and builds. The Doxygen docs build (-DMORPH_BUILD_DOCUMENTATION=ON, --target doc) exits 0 and fetches doxygen-awesome-css through morph_use_cpm.

Not verified

  • examples/common and examples/bank were not configured (ladder and bank are off here). Their morph_use_cpm(Lightweight) is a no-op whenever CPM is already loaded, which it is on every CI leg that fetches glaze or core-cpp.
  • No Windows/vcpkg run. vcpkg provides glaze and Catch2 but not core-cpp, so CPM still loads there, for core-cpp.
  • No Emscripten configure.

Whole-branch checks

  • morph_tests (full): test cases: 1666 | 1665 passed | 1 failed as expected (the one is a pre-existing [!shouldfail] case).
  • morph_net_tests (full): All tests passed (1137 assertions in 198 test cases).
  • Doxygen doc target: exit 0 under WARN_AS_ERROR.
  • clang-format applied to every touched C++ file. clang-tidy was not run: it is not installed on this machine, so the tidy leg is the first to check that.
  • Valgrind and all-optional-features are nightly-only, so they will not run on this PR.

🤖 Generated with Claude Code

Yaraslaut and others added 3 commits September 29, 2026 15:48
… lands (#841)

execute() on a handler built over a kCallerMustNotBlock backend fails fast
with "handler not bound" until the registration reply arrives, so every
embedder that dispatches right after constructing a handler wrote the same
whenBound() gate plus a liveness guard. executeWhenBound(action) is that
gate, named at the call site; execute() stays fail-fast.

- Bound: dispatches exactly as execute().
- In flight: chains on whenBound(); rejects with the registration's error,
  or "handler not bound" if whenBound() resolves false.
- The held dispatch captures the binding weakly and not the handler, so a
  handler destroyed first drops it. A strong capture would close a cycle
  (binding -> waiter -> dispatch -> binding) and dispatch onto an instance
  nobody holds; the lifetime tests catch that mutation.
- NoSharing only (static_assert): on a shared handler whenBound() never waits.

Specs: bridge.md gains the executeWhenBound subsection and API row;
backend.md's "does not queue" sentence names the opt-in.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…oops (#831)

TcpSocket::connect's EINPROGRESS wait, the handshake's waitReadableUntil
and the test FakeWsServer::acceptWithin each hand-rolled the same loop:
one deadline, recompute the remaining budget, clamp to int ms, poll,
retry on EINTR. pollUntil(fd, events, deadline) owns it once and reports
{kReady, kTimedOut, kFailed + errno} rather than throwing, so each caller
keeps its own policy: connect closes the candidate and tries the next,
the handshake throws with its existing messages, the test fake throws.

acceptWithin also gains the int clamp it lacked.

New [poll_until] cases cover the passed deadline, a real timeout, a
ready listener, and a thread hit with SIGUSR1 every 50 ms: mutating the
helper to fail on EINTR, or to re-arm the full budget after one, each
fails that case.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…840)

A configure whose dependencies are all installed no longer touches the
network. core-cpp gets the treatment glaze already had, find_package(
core-cpp 0.5 CONFIG QUIET) before CPMAddPackage, and the CPM bootstrap,
which downloads CPM.cmake, is loaded by morph_use_cpm(<what>) at each
fetch site only when that site actually fetches. The first such call
names the dependency in a STATUS line, so a failed bootstrap download
says which dependency asked for it and how to avoid it.

- AF_SANITIZER skips find_package(core-cpp) and builds it in-tree, and
  an empty CORE_CPP_TARGETS under a sanitizer is now a FATAL_ERROR
  rather than a silently uninstrumented core-cpp.
- A found core-cpp is not installed again; morph's export names its
  imported targets. MORPH_CORE_CPP_VERSION states the bound once for
  both find_package and morphConfig.cmake (whose comment said 0.3).
- tests/: the try_compile guard probes read an installed core-cpp's
  include directories from core::base rather than core-cpp_SOURCE_DIR.
- cmake/CPM.cmake stays byte-identical to core-cpp's below its header;
  the header now says which parts of the FATAL_ERROR's advice are
  core-cpp's and what applies at morph's top level instead.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Yaraslaut Yaraslaut added enhancement New feature or request area: core Subsystem: core area: ci Subsystem: ci labels Sep 29, 2026
@Yaraslaut

Copy link
Copy Markdown
Member Author

Runner review: one open design point in executeWhenBound (#841)

I checked the file list: it stays within this lane's area and has no overlap with the other open lane. The implementation matches the triage: the binding is held weakly, this is never captured, NoSharing is enforced by static_assert, and plain execute() is unchanged.

Open point: "handler destroyed before the bind → the returned Completion never settles."

  • In that case the .then continuation never runs at all, because it hangs off whenBound()'s waiter, which is freed along with the binding. The if (!binding) return; branch only covers the narrower race where the continuation runs after destruction. Either way the CompletionState is released without ever being settled.
  • For .then/.onError callers this is harmless: nothing runs.
  • For a coroutine that co_awaits the result, it is the leak docs/spec/core/coroutines.md already lists ("A handler whose await never completes is leaked"). This method would add a new, easy way to reach it: a screen that dispatches on open and closes before the reply arrives.
  • Alternative: capture a small guard in the continuation whose destructor settles the state with an error (e.g. "handler destroyed") if nothing else has. A caller then always gets exactly one answer, like every other execute path, where the backend always settles.
  • whenBound() itself already behaves this way, so the current choice is at least consistent. That makes this the maintainer's call rather than a defect. I am not blocking the PR on it.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment