Conversation
… 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>
Member
Author
Runner review: one open design point in
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
BridgeHandler::executeWhenBound(action), shaped the way the triage comment recommends: a separately named method rather than a constructor policy.execute()still fails fast.execute()does.whenBound(), then dispatches on the GUI executor. A failed registration rejects with the registration's error. IfwhenBound()resolvesfalse, it rejects with"handler not bound".Completionnever settles."bridge destroyed".static_assert(!kShared): the method isNoSharingonly, because on a shared handlerwhenBound()never waits. A secondstatic_assertrequires a copyable result, since the deferred path copies the value from oneCompletionto another, the same way the keyed branches ofexecute()do.weak_ptr<HandlerBinding>,&_bridge, theBridgeLifetimegate, the executor and the state. It never capturesthis.bridge.mdgains anexecuteWhenBound()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 onqt_websocket_backend.hpp'sasyncRegistrationEnabledmentions it too.Verified
morph_tests "[executeWhenBound]":All tests passed (31 assertions in 5 test cases). The five cases sit next to thewhenBoundcases:completeNext();onErrorwith its message;DeterministicExecutor) leaves nothing dispatched;AsyncRegisterBackendgained anexecuteCount()so "nothing dispatched" is measured at the backend.morph_tests "[registration]":All tests passed (1121 assertions in 61 test cases).executeWhenBound→return execute(std::move(action));:test cases: 5 | 1 passed | 4 failed. Only the already-bound case survives, as it should._bindingstrongly:test cases: 5 | 3 passed | 2 failed. Both lifetime cases fail, and the destroyed-before-bind case reportsexecuteCount() == 1withresolved == 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 inbridge.md.Not verified
executeWhenBoundwas exercised only against theAsyncRegisterBackenddouble."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
executeVia. Holding it would mean a recursiveshared_lockwhenever the backend settles inline, becauseBridgeSink'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."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 intcp_socket.hpp. It is not public API.EINTR.poll()call tointmilliseconds.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 theintclamp, which it lacked before.EINTRerror. I went with a reportedkFailed, so thatconnectcan keep "try the next candidate" without a try/catch.Verified
[poll_until]cases intest_tcp_socket.cpp(All tests passed (13 assertions in 4 test cases)):kReady;SIGUSR1every 50 ms for 1.5 s (noSA_RESTART) still ends at its 300 ms deadline.EINTRtreated as failure: the signal case fails with2 == 1(kFailed) and56608750 ns >= 300 ms.EINTR: the signal case fails with1968299500 ns < 1200 ms.morph_net_tests:All tests passed (1137 assertions in 198 test cases).Not verified
#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 beforeCPMAddPackage.MORPH_CORE_CPP_VERSION(0.5) is the single place the bound is stated, andmorphConfig.cmake.inuses it as well. That also fixes its comment, which said "0.3".morph_use_cpm(<what> [<why>])is a macro that includescmake/CPM.cmakeonly ifCPMAddPackageis not already a command. It is called at every fetch site: glaze, core-cpp, Catch2,docs/(doxygen-awesome-css), andexamples/commonandexamples/bank(Lightweight). The first site to load CPM printsmorph: loading CPM to fetch <what>: <why>.cmake/CPM.cmakeis still byte-identical to core-cpp's copy below its header. Only the header, which morph owns, changed: it now says the FATAL_ERROR'sCORE_CPP_FETCH_DEPS=OFF/FetchTransferBound.cmakeadvice 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.AF_SANITIZER, morph skipsfind_package(core-cpp)and always builds it in-tree, so it can be instrumented. An emptyCORE_CPP_TARGETSunder a sanitizer is now aFATAL_ERROR.CORE_CPP_INSTALLandEXCLUDE_FROM_ALLhandling now lives only on the fetch path.tests/CMakeLists.txt: thetry_compileguard probes read an installed core-cpp's include directories fromcore::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 intoprefixand separately intop-core; glaze went intop-glaze.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 ownManually-specified variables were not used by the project: CPM_SOURCE_CACHE, which is direct evidence that CPM never loaded.core-cpp_DIRandglaze_DIRpoint into the prefixes.origin/master(git archiveinto a scratch dir, with the singleprefix): exit 1,CMake Error at cmake/CPM.cmake:48 ... could not download the CPM.cmake bootstrap: 7;"Couldn't connect to server", reached fromCMakeLists.txt:149 (include). So the check tells the two trees apart.-- morph: loading CPM to fetch glaze: not found installed (install it and point CMAKE_PREFIX_PATH at it to configure without the network), thencould not download the CPM.cmake bootstrap: 7;"Couldn't connect to server", thenConfiguring incomplete.-DAF_SANITIZER=tsan -DCMAKE_PREFIX_PATH=prefixconfigures with-- morph: loading CPM to fetch core-cpp: AF_SANITIZER builds it in-tree so that it can be instrumentedandCPM: Adding package core-cpp@0.5.0, and nocore-cpp_DIRappears in the cache.-fsanitize=threadshows up on thecore-cpp-*targets inbuild.ninja.find_packageallowed under a sanitizer): exit 1,AF_SANITIZER=tsan but core-cpp lists no targets to instrument (the CORE_CPP_TARGETS global property is empty) ....cmake --install cfgA2 --prefix morph-instinstalls onlylib/cmake/morphand headers, no core-cpp, andmorphConfig.cmakehasfind_dependency(core-cpp 0.5 CONFIG). A scratch consumer (find_package(morph CONFIG REQUIRED), linksmorph::morph, constructs aBridgeoverLocalBackend) configured, built and ran with exit 0 againstmorph-inst;prefix.p-core;p-glaze, Catch2 found installed): configure exit 0, and themorph_client_only_guard_linksprobe built and ran with exit 0. The same configure with master'stests/CMakeLists.txt: exit 1,MORPH_CLIENT_ONLY guard check failed ... the linker did not name— so thetests/change is load-bearing. With a single shared prefix the old file passes by accident, because glaze's include dir is the sameprefix/include.-DMORPH_BUILD_DOCUMENTATION=ON,--target doc) exits 0 and fetches doxygen-awesome-css throughmorph_use_cpm.Not verified
examples/commonandexamples/bankwere not configured (ladder and bank are off here). Theirmorph_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.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).doctarget: exit 0 underWARN_AS_ERROR.🤖 Generated with Claude Code