proxy-io: Reference-count Connection objects - #336
Draft
ryanofsky wants to merge 19 commits into
Draft
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline and AI policy for information on the review process. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. LLM Linter (✨ experimental)Possible typos and grammar issues:
Possible places where named args for integral literals may be used (e.g.
2026-09-11 20:45:54 |
Collaborator
Author
|
Rebased a067599 -> 5187179 ( Rebased 5187179 -> 6c7f1bf ( |
|
🐙 This pull request conflicts with the target branch and needs rebase. |
…sier to understand. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ryanofsky
added a commit
to ryanofsky/libmultiprocess
that referenced
this pull request
Sep 4, 2026
Add accessors to avoid libmultiprocess applications needing to create Connection objects directly or access their internals. It adds an EventLoop::incomingConnections method and a ServeStream overload that accepts a shared interface pointer instead a reference. This is just a refactoring that does not change behavior. It allows Bitcoin Core code to be simplified and to avoiding needing to change again with upcoming PRs such as bitcoin-core#336 which the change the way Connection objects work. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Waiter::m_mutex must not be locked before EventLoop::m_mutex. That is the reverse of the documented and actual lock order (Waiter::m_mutex first, as ~ProxyServer<Thread> does). The constraint it was reaching for is the EventLoop blocking rule now documented on Waiter::m_mutex. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YKQfSnMnUzqyDxKp7GpFam
Currently, a ListenConnections listener that reaches its max-connection limit stops accepting new connections permanently if one of its connections is closed locally instead of by a remote disconnect. Closing a connection locally (e.g. erasing it from m_incoming_connections) leaves the listener's active-connection count stuck at the limit, so it never resumes accepting. This happens because the count is decremented by a callback which only fires on a remote disconnects, not local disconnects. Fix by moving the decrement to callback which fires on both local and remote disconnects. Add a regression test that closes a connection locally and checks the listener resumes accepting; it fails before this change (the listener never accepts the waiting client) and passes after. Co-Authored-By: Enoch Azariah <enirox001@gmail.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Fix a use-after-free, possible since the destroy_connection option was added in 2019 (c685fa9): a Connection's disconnect handler could run after the Connection had already been destroyed, deleting it a second time and crashing. Reported by enirox001 in bitcoin-core#335 (comment) Give each Connection a shared_ptr "alive" token that disconnect handlers hold a weak_ptr to and check before running, so a handler is skipped once its Connection is gone. Having this check also enables the simplifications described below. Previously each Connection kept its disconnect handlers in its own kj::TaskSet, and when the network disconnected it moved a handler onto the shared event loop TaskSet with kj::evalLater. Destroying the Connection destroyed that per-connection TaskSet, canceling a still-pending handler -- but a handler already moved onto the shared TaskSet was no longer canceled and could run after the Connection was gone. (The evalLater step existed only to avoid a "promise callback destroyed itself" error when a handler deletes its own Connection, which the per-connection TaskSet made possible.) With the token doing the cancellation, neither the per-connection TaskSet nor the evalLater step is needed, and both are removed. Co-Authored-By: Enoch Azariah <enirox001@gmail.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
connection being destroyed on the event loop thread, which could destroy the same ProxyClient<Thread> object twice. ~ThreadContext destroyed the thread-local request_threads/callback_threads maps with no locking while the SetThread cleanup callback run by ~Connection erased entries from the same maps. When both ran at once, each side destroyed the entry's ProxyClient<Thread>, and ~Connection then ran the ProxyClientBase disconnect callback on the freed map node (heap-use-after-free, then a glibc "double free or corruption" abort). Fix by making map entry removal decide which side destroys an entry: ~ThreadContext and the SetThread callback each remove entries under Waiter::m_mutex before destroying them, and a side that finds an entry already gone leaves it to the other. See the code comments for why the entries are destroyed with the mutex released. Add a regression test, "Thread exiting while its connection is destroyed", which uses a new testing_hook_thread_client_destroy hook to interleave the two sides deterministically and fails on every run without the fix. The race is long-standing and reachable on master via connections created by ConnectStream, whose onDisconnect handler deletes the client Connection on the event loop thread when the peer disconnects. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BnBLP1xuPf4fLpnQto8mEX Claude-Session: https://claude.ai/code/session_01YKQfSnMnUzqyDxKp7GpFam
…uction Split connection teardown out of ~Connection into an idempotent disconnect() method, with the destructor delegating to it. For existing callers, this is a behavior-neutral refactor: the same steps run in the same order on destruction. Having a separate disconnect() method allows severing a connection while keeping the Connection object alive, which the next commits use to let shutdown code wait for in-flight server call bodies to finish after a disconnect (bitcoin/bitcoin#35845). Two details are new in the disconnect() method which were not present in the destructor method: - disconnect() expires the m_alive token explicitly, where previously it was expired implicitly by member destruction. This keeps onRemoteDisconnect able to distinguish a local disconnect from a remote one when a connection is severed without destroying the object (see the disconnect() code comment). - disconnect() explicitly releases m_thread_pool and m_thread_map so worker thread teardown happens at disconnect time whether or not the object is destroyed right away. Previously this happened implicitly during member destruction. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…calls Add a per-connection ServerObjectTracker counting live ProxyServer objects, incremented in the ProxyServerBase constructor and decremented in its destructor, with Connection::waitDrained() blocking until the count reaches zero and Connection::pendingServerObjects() exposing it for logging. Disconnecting a connection cancels the KJ promise of an in-flight call, but a C++ server method body already dispatched to a worker thread runs to completion. Counting live server objects turns Cap'n Proto's object lifetime rules into a usable quiescence signal: a ProxyServer object is not destroyed until its outstanding calls finish (the target capability is kept alive for the duration of a call and pinned by post()/PassField via thisCap()), so after disconnect() the count drains to zero exactly when no server call body is still executing. Waiting for that lets shutdown code avoid freeing application state that a still-running call body dereferences (bitcoin/bitcoin#35845). The tracker is held via shared_ptr by the Connection and by every ProxyServer object because objects kept alive by in-flight calls can outlive the Connection on some teardown paths (see ~ProxyServerBase), and their destructors must decrement state that is still valid. It must be declared before m_rpc_system, whose construction creates the bootstrap server object that registers itself with the tracker. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Add a deterministic mptest regression test for bitcoin/bitcoin#35845: hold a server method body in flight on a worker thread, call Connection::disconnect(), and assert that Connection::waitDrained() blocks until the body finishes and its server object is destroyed. Also covers destroying an already-disconnected connection (~Connection noticing disconnect() has run). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Co-Authored-By: Enoch Azariah <enirox001@gmail.com>
Add an accessor and a Connections type alias for the EventLoop's list of incoming connections, so future code can be simplified to locate a specific connection without directly accessing the private list or embedding a Connection object itself. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Add a _Serve/ServeStream overload accepting the init object as a shared_ptr, so callers can transfer ownership instead of always passing a reference to an object they keep alive themselves. Existing reference-taking callers keep working through a thin overload that wraps the reference in a shared_ptr with an empty deleter. Also return the constructed ProxyServer along with an iterator to its Connection in loop.m_incoming_connections, so callers can look up or erase the connection later without embedding a Connection object themselves. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…Stream Give ServeStream and ConnectStream a destroy_connection parameter, defaulting to true, so callers can opt out of automatic connection teardown and manage the Connection's lifetime themselves instead. ServeStream gates the internal disconnect handler's list erase on the parameter; ConnectStream just forwards it to the existing ProxyClientBase parameter of the same name. This lets callers that need to keep a connection alive past a disconnect notification (e.g. to let in-flight server calls finish) use these helpers instead of constructing a Connection manually. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Replace TestSetup's manual Connection construction with ServeStream/ConnectStream, following the same pattern already used in Bitcoin Core's own IPC test and fuzz code. This drops server_on_disconnect entirely: ServeStream's destroy_connection parameter now controls whether a remote disconnect erases the server Connection, so the only test that needed to suppress that (the mp#348 getResults race test) just constructs TestSetup with server_owns_connection=false instead of overriding a callback afterward. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
# Conflicts: # include/mp/proxy-io.h # test/mp/test/test.cpp
Make Connection objects shared_ptr-owned, created via a new Connection::make() factory, and have every proxy object share ownership of its connection (ProxyContext::connection becomes a shared_ptr, populated via enable_shared_from_this). A Connection now always outlives its proxy objects and survives disconnect() as an inert husk until its last reference drops. Sharing ownership removes two workarounds that existed only because a Connection could previously be destroyed while proxy objects still referenced it: - ~ProxyServerBase no longer has to avoid dereferencing m_context.connection; the connection is guaranteed to still exist. - The separate m_alive liveness token is dropped. The deferred disconnect handler (renamed onRemoteDisconnect -> afterDisconnect, since it fires on any disconnect and not only remote ones) instead runs off a weak_ptr to the Connection and passes each handler a Connection* -- the live connection, or null if it was already destroyed before the handler ran. Call sites branch on that explicitly rather than the framework silently skipping them; handler bodies are idempotent disconnect() calls, not deletions. Two supporting changes fall out of this and are documented at the code rather than repeated here: connection construction is split into make() and serve() (see their comments), and because the shared references form a cycle that reference-dropping can't break, all teardown now routes through disconnect() -- so every teardown path calls it (see the note on Connection::make). The _Serve disconnect handler's list bookkeeping changed to match (see the comment there). Also drop the test that checked a queued disconnect handler does not run after its Connection is destroyed. It reproduced the m_alive race by constructing a Connection directly and freeing it while the handler was still queued. Both premises are gone: a Connection can only be built through make() now, and handler bodies no longer delete anything, so the double deletion the test guarded against is structurally impossible. afterDisconnect now hands such a handler a null Connection* instead, which its callers handle explicitly, and the ordinary disconnect and drain tests exercise that path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Remove the per-client disconnect tracking from ProxyClientBase: client objects no longer register a cleanup callback with their Connection, and a disconnect no longer eagerly releases their m_client capability handles or nulls their connection pointers. Neither is necessary now that proxy objects share ownership of their Connection. The connection pointer stays valid after a disconnect because the Connection outlives its proxies, and keeping the capability handle is safe: Cap'n Proto's per-connection state is refcounted and outlives the RPC system as long as handles reference it, with calls on handles of a disconnected connection failing cleanly with DISCONNECTED errors. The handle is simply released (on the event loop thread, since capability refcounts are not thread safe) whenever the client object is eventually destroyed, and clientInvoke checks the connection's disconnected() predicate instead of a nulled pointer, throwing the same 'IPC client method called after disconnect' error as before. This deletes the detach machinery from ~ProxyClientBase, including the FIXME'd duplicate-cleanup code path. Connection::onDisconnect (the renamed addSyncCleanup) remains for its one other user, the per-thread connection maps (see SetThread), which the next commit converts. Because the connection pointer is no longer nulled on disconnect, ~ProxyClient <Thread> can no longer use it to tell whether Connection::disconnect() has already run and freed its m_disconnect_cb cleanup node. It now keys off the connection's disconnected() predicate instead: if the connection is disconnected the node is already gone and must not be passed to cancelOnDisconnect. Without this, ~ProxyClient<Thread> erased an already-freed list node -- a heap-use-after-free (see the "Waiting for in-flight server call to finish after disconnect" test, which this commit re-enables). That test also has to hold its own shared reference to the server Connection across the disconnect() + waitDrained() sequence, the way Ipc::disconnect Incoming does: under shared ownership the last server proxy (destroyed once the drained body finishes) would otherwise free the Connection while the test is still observing it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Update comments to reflect that after the previous commit, the sync cleanup callback list has exactly one remaining purpose: eagerly removing a disconnected connection's ProxyClient<Thread> entries from the thread_local per-thread connection maps (ThreadContext::request_threads / callback_threads) via callbacks registered by SetThread. Unlike interface clients, these entries cannot simply be left alive across a disconnect: they are owned by other threads that may never touch their maps again, and a surviving entry would hold the disconnected Connection object -- and through its EventLoopRef the event loop -- alive indefinitely, preventing the loop from ever exiting. (Replacing the callbacks with lazy garbage collection in SetThread was tried and hangs mptest for exactly this reason: entries owned by long-lived threads pin the loop after their connection is gone.) So this per-object disconnect tracking is retained by design, now clearly documented as thread-map-specific rather than a general client-object mechanism. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Now that proxy objects hold shared ownership of their Connection, a ProxyServer object kept alive by an in-flight call can no longer outlive the Connection, so ~ProxyServerBase can always reach the tracker through m_context.connection. Drop the shared_ptr indirection that existed to keep the tracker valid past the Connection's death, and the separate tracker handle member on ProxyServerBase. No behavior change; Connection::waitDrained() semantics are identical. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Needed because changing m_incoming_connections type is an API change.
This was referenced Sep 11, 2026
ryanofsky
force-pushed
the
pr/notrack
branch
from
September 11, 2026 20:45
5187179 to
6c7f1bf
Compare
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
2 participants
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.
Use reference counting to manage Connection object lifetimes. This implements an old idea from #176 (comment) and has two benefits:
disconnectandwaitDrainedmethods #335 for server objects, and this PR extends it to treat client and server objects symmetrically.ProxyClientobjects register with Connections, soConnectionobjects no longer need to store lists ofProxyClientobjects and can just use use counts instead.This is based on #335 + #365. The non-base commits are:
c092c18proxy-io: manage Connection lifetime with shared_ptr7635c12proxy-io: keep client capability handles across disconnect162b7a6doc: scope Connection cleanup callbacks to per-thread map entriesf0a629dproxy-io: make server object tracker a plain Connection member6c7f1bfci: Check out bitcoin/bitcoin PR #35932 instead of master