Skip to content

fix(asio): order the read handoff so a completed read cannot be missed - #871

Merged
got3nks merged 1 commit into
amule-org:masterfrom
got3nks:fix/asio-read-state-visibility
Aug 9, 2026
Merged

fix(asio): order the read handoff so a completed read cannot be missed#871
got3nks merged 1 commit into
amule-org:masterfrom
got3nks:fix/asio-read-state-visibility

Conversation

@got3nks

@got3nks got3nks commented Aug 9, 2026

Copy link
Copy Markdown

CAsioSocketImpl hands a completed read from the asio strand to the main thread through two plain fields: HandleRead writes m_readBufferContent and then clears m_readPending, and Read() tests m_readPending || m_readBufferContent == 0 before 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_readPending set. 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):

#1793400M: block(8)      [c=0  p=1 e=1]
#1793402A: handleRead(10)[c=10 p=0 e=1]   <- strand: content set, pending cleared
#1793403M: postSkip(2)   [c=10 p=0 e=1]
#1793404M: block(8)      [c=10 p=1 e=0]   <- main: new content, stale flag

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_readPending std::atomic<bool>, release-store it in HandleRead after the buffer writes, and acquire-load it in Read(). 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:

build run medians (MB/s) median spread
this branch 111.4, 124.2, 124.7 124.2 13.4
master 111.7, 119.6, 124.0, 124.2, 126.1 124.0 14.4

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

CAsioSocketImpl is TCP-only and backs eD2k peers, server links, proxies and EC alike. UDP goes through CAsioUDPSocketImpl, which has no m_readPending, so Kad is untouched.

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
got3nks force-pushed the fix/asio-read-state-visibility branch from 9a07ab0 to 3aed8cd Compare August 9, 2026 17:38
@got3nks
got3nks merged commit 2c6b0dd into amule-org:master Aug 9, 2026
14 checks passed
@got3nks
got3nks deleted the fix/asio-read-state-visibility branch August 9, 2026 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant