proxy-io: Fix theoretical disconnect bugs - #361
Open
ryanofsky wants to merge 5 commits into
Open
Conversation
This is a documentation-only change meant to make upcoming commits easier to understand. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
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.
If your review is incorrectly listed, please copy-paste 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. |
enirox001
reviewed
Sep 9, 2026
Contributor
There was a problem hiding this comment.
ACK bc98767
Thnaks for opening this @ryanofsky, this does address the issues i mentioned, i agree these improvements here are more conservative as they have not been seen in practice, but would be nice to have it if we want to make disconnection more robust.
Left some documentation nits
Correct the ThreadContext "Synchronization note", which said 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>
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>
Fix a race between a thread exiting after making IPC calls and its 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>
ryanofsky
commented
Sep 11, 2026
ryanofsky
left a comment
ryanofsky
left a comment
Collaborator
Author
There was a problem hiding this comment.
Updated bc98767 -> 0f5c633 (pr/ondis.1 -> pr/ondis.2, compare) dropping renames because they cause problems downstream in test_bitcoin, making more documentation updates and adding a third bugfix here "thread map teardown race" that was originally in #335, since it fixes another bug that precedes #335
ryanofsky
changed the title
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
3 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.
Two bugs in
Connection::onDisconnectwere pointed out in #335 review by @enirox001:evalLatercould allow an onDisconnect handler to run after a Connection object was destroyed.A third "thread map teardown race" bug was also encountered in a new unit test introduced by #335. This PR fixes each bug in a separate commit and also includes documentation commits to help make the changes more understandable.
The fixed bugs are "theoretical" just in the sense that they haven't been seen in practice and were found in code review. The first ListenConnections bug can't currently happen in bitcoin core because it doesn't disconnect IPC clients except when it is shutting down, and it would not make sense to accept new connections. The race condition bugs have just existed for many years and not been seen previously.