fix(asio): order the read handoff so a completed read cannot be missed - #871
Merged
Merged
Conversation
A connection could stop receiving while staying open, answering nothing until the peer's own timeout gave up on it. Caught on arm64 with a read-path trace: handleRead(10)[c=10 p=0] <- ASIO thread: content set, pending cleared block(8) [c=10 p=1] <- main thread: new content, stale flag HandleRead publishes a completed read by writing the buffer contents and then clearing m_readPending, and Read() consults that flag to decide whether a background read is still outstanding. Both were plain members written on the ASIO thread and read on the main thread, so nothing required the two stores to become visible in that order. Where they arrive out of order the main thread sees the new content with the flag still set, takes the "still running" branch, and returns without serving data already sitting in the buffer -- and without arming another read or posting an event. That branch is reached from an event which has by then been consumed, so nothing looks again. Both callers of PostReadEvent are unreachable: no read is pending to complete, and Read is only entered from an event. The socket is finished while remaining perfectly healthy at the TCP layer. Making m_readPending atomic with release/acquire is enough. HandleRead already writes the buffer before clearing it, so a reader that acquire-loads false is guaranteed the buffer state that preceded it; the content, pointer and blocking flag need no change. Its neighbours on the write path are already atomic with comments saying they are shared across those same two threads, which is what makes the read side look like an oversight rather than a decision. Observed against a daemon over EC, where a stalled connection is visible as a frozen client, but the members belong to the shared TCP socket implementation and every connection uses them. The UDP path has its own implementation and does not share these members.
got3nks
force-pushed
the
fix/asio-read-state-visibility
branch
from
August 9, 2026 17:38
9a07ab0 to
3aed8cd
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
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.
CAsioSocketImplhands a completed read from the asio strand to the main thread through two plain fields:HandleReadwritesm_readBufferContentand then clearsm_readPending, andRead()testsm_readPending || m_readBufferContent == 0before serving buffered bytes.Neither field is atomic, so nothing orders those writes against the reader. On a weakly-ordered target the main thread can observe the new content while still seeing
m_readPendingset. It then takes the "background read still running" branch and returns 0 — without serving the buffered data and without re-arming a read. The receive event for that data has already been consumed, so nothing ever looks at the socket again and the connection wedges silently.Captured on a live amulegui/amuled pair, both ARM64 (sequence numbers;
M= main thread,A= asio strand;c= content,p= pending,e= event):Event accounting was exact across the whole capture (
ev_posted=17314 ev_delivered=17314 ev_spurious=0), which rules out a lost notification: the post was delivered, and the reader simply mis-read the state.Fix: make
m_readPendingstd::atomic<bool>, release-store it inHandleReadafter the buffer writes, and acquire-load it inRead(). The release/acquire pair publishes everything written before it, so a cleared flag now implies visible content.Testing
LAN transfer, 30 GB file, patched daemon on both ends, alternating against master with a discarded warm-up run to prime the seeder's page cache. Steady-state median over T+30…T+90:
Delta +0.1%, well inside the noise — both distributions have the same shape, including a single low run each, so that outlier belongs to the test rig rather than to either build. No measurable throughput cost.
Scope
CAsioSocketImplis TCP-only and backs eD2k peers, server links, proxies and EC alike. UDP goes throughCAsioUDPSocketImpl, which has nom_readPending, so Kad is untouched.