fix(torrent): don't lose msg writer wakeups during buffer fill - #1071
Open
aslafy-z wants to merge 1 commit into
Open
fix(torrent): don't lose msg writer wakeups during buffer fill#1071aslafy-z wants to merge 1 commit into
aslafy-z wants to merge 1 commit into
Conversation
aslafy-z
force-pushed
the
fix/msg-writer-lost-wakeup
branch
from
August 3, 2026 15:49
4035c7d to
0a306d4
Compare
Signed-off-by: Zadkiel AHARONIAN <hello@zadkiel.fr>
aslafy-z
force-pushed
the
fix/msg-writer-lost-wakeup
branch
from
August 3, 2026 16:33
0a306d4 to
efe5232
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.
Fixes #1070.
Change
Two changes in
peer-conn-msg-writer.go'srunloop:writeCond.Signaled()beforefillWriteBuffer(): a Broadcast landing mid-fill now closes the pre-installed channel instead of being dropped, so the park returns immediately.keepAliveTimerbefore each park with the time remaining until the keepalive deadline, falling back to a full interval once it has passed. The timer was previously onlyResetafter 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 holdingcn.mu(it previously ran under it in the park branch). That's safe:BroadcastCondis internally synchronized — lock-free in chansync v0.8.0 (atomic.Pointer+ CAS) — so concurrentSignaled/Broadcastneed no external locking. The type's doc line about callers bringing their own synchronization concerns coordinating the guarded state (it explains why there is noSignal), not these calls.Tests
Each change is pinned by its own deterministic test; both fail on master with distinct diagnostics and pass under
-racewith the fix:TestMsgWriterTickleDuringFillNotLost: holds the writer insidefillWriteBufferand 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.