Skip to content

fix(torrent): don't lose msg writer wakeups during buffer fill - #1071

Open
aslafy-z wants to merge 1 commit into
anacrolix:masterfrom
aslafy-z:fix/msg-writer-lost-wakeup
Open

fix(torrent): don't lose msg writer wakeups during buffer fill#1071
aslafy-z wants to merge 1 commit into
anacrolix:masterfrom
aslafy-z:fix/msg-writer-lost-wakeup

Conversation

@aslafy-z

@aslafy-z aslafy-z commented Aug 3, 2026

Copy link
Copy Markdown

Fixes #1070.

Change

Two changes in peer-conn-msg-writer.go's run loop:

  • Obtain writeCond.Signaled() before fillWriteBuffer(): a Broadcast landing mid-fill now closes the pre-installed channel instead of being dropped, so the park returns immediately.
  • Arm keepAliveTimer before each park with the time remaining until the keepalive deadline, falling back to a full interval once it has passed. The timer was previously only Reset after a successful write, so a bare fire on an idle connection left it dead, removing the periodic backstop; per-park arming also keeps the deadline accurate instead of restarting the interval on every wakeup.

Note for review: Signaled() is now called without holding cn.mu (it previously ran under it in the park branch). That's safe: BroadcastCond is internally synchronized — lock-free in chansync v0.8.0 (atomic.Pointer + CAS) — so concurrent Signaled/Broadcast need no external locking. The type's doc line about callers bringing their own synchronization concerns coordinating the guarded state (it explains why there is no Signal), not these calls.

Tests

Each change is pinned by its own deterministic test; both fail on master with distinct diagnostics and pass under -race with the fix:

  • TestMsgWriterTickleDuringFillNotLost: holds the writer inside fillWriteBuffer and issues the Broadcast strictly within that window. Without the fix there is no subscriber yet and the writer parks forever.
  • TestMsgWriterKeepAliveTimerRearmed: lets the timer fire once on a not-yet-useful connection, then makes it useful; no tickle is involved, so the keepalive can only come from a re-armed timer. Without the fix the timer stays dead.

External validation

streamline's bittorrent integration suite (28 specs, including a 30-download-cycle spec that reliably wedged under CPU contention on v1.61) passes against this fix with their engine-side 5s-keepalive mitigation (DataHearth/streamline@8521da1) reverted: DataHearth/streamline#4.

@aslafy-z
aslafy-z force-pushed the fix/msg-writer-lost-wakeup branch from 4035c7d to 0a306d4 Compare August 3, 2026 15:49
Signed-off-by: Zadkiel AHARONIAN <hello@zadkiel.fr>
@aslafy-z
aslafy-z force-pushed the fix/msg-writer-lost-wakeup branch from 0a306d4 to efe5232 Compare August 3, 2026 16:33
@aslafy-z aslafy-z changed the title fix: Don't lose msg writer wakeups during buffer fill fix(torrent): don't lose msg writer wakeups during buffer fill Aug 3, 2026
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.

Peer request-update wakeup can be lost by the message writer, wedging the connection

1 participant