fix: race condition on connIdIssued in Client.request#1039
Merged
anacrolix merged 7 commits intoanacrolix:masterfrom Feb 26, 2026
Merged
fix: race condition on connIdIssued in Client.request#1039anacrolix merged 7 commits intoanacrolix:masterfrom
anacrolix merged 7 commits intoanacrolix:masterfrom
Conversation
The write to connIdIssued on error response (Connection ID mismatch) was not protected by the client mutex, racing with concurrent requestWriter goroutines that read connIdIssued via writeRequest -> connect -> shouldReconnect (which do hold the lock). Fixes erigontech/erigon#18901
I followed the instructions at https://github.com/golangci/golangci-lint-action.
_pendingPieces.Add(0) was called without holding the client lock and without updating the piece request order, causing two issues: 1. Data race with timer goroutines accessing the roaring bitmap 2. Panic in checkPendingPiecesMatchesRequestOrder due to _pendingPieces and requestOrder being out of sync Replace the direct bitmap manipulation with updatePiecePriority(0, ...) which properly updates both _pendingPieces and the piece request order while holding the client lock. Co-authored-by: info@weblogix.biz <admin@10gbps.weblogix.it> Co-authored-by: Matt Joiner <anacrolix@gmail.com>
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.
Problem
The write to
connIdIssuedinClient.request()(line 235) on error response (Connection ID mismatch) is not protected by the client mutex. This races with concurrentrequestWritergoroutines that readconnIdIssuedviawriteRequest→connect→shouldReconnect, which do hold the lock.When multiple announce goroutines are spawned by
startAnnounce(), they share the sameClientand can concurrently enterrequest(). If two of them receive error responses simultaneously, both write toconnIdIssuedwithout synchronization.Race detected with
-race:Fix
Acquire
cl.mubefore writingconnIdIssued = time.Time{}in the error response handler, matching the locking discipline used bywriteRequestwhen reading the same field.Uses
LockCtx(ctx)so the lock attempt is cancelled if the context is already done.Fixes erigontech/erigon#18901