fix(amqp091): close race on BrokerDetails.clientDisconnect#102
Open
miotte wants to merge 1 commit into
Open
Conversation
This was referenced May 22, 2026
Open
The connectionWatcher goroutine reads bd.clientDisconnect in its main loop and in the reconnect retry loop, while Disconnect writes it from the caller goroutine. Both sides were plain bool accesses with no synchronization, which the race detector reliably flags during Test_StreamRetry (and several other tests that exercise the Connect/Disconnect path). Switch the field to atomic.Bool, matching the pattern already used for the analogous flag on streamConnectionShim in streamshim.go. The connect() early-out and both watcher loops use Load(); Disconnect uses Store(true). The struct-literal initializer was dropped because atomic.Bool's zero value is already false, and the three test sites that re-set the flag to false on a freshly-constructed BrokerDetails were similarly redundant. Signed-off-by: Michael Otteni <MichaelGOtteni@gmail.com>
dlawregiets
approved these changes
May 28, 2026
This was referenced May 30, 2026
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
2 participants
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 #101
Summary
BrokerDetails.clientDisconnectfrombooltoatomic.Boolso theconnectionWatchergoroutine and theDisconnectcaller can no longer race. Mirrors the pattern already used on the analogous flag instreamshim.go:40.connectionWatcher(amqp091.go:1797,:1817) andconnect()(amqp091.go:1863) move toLoad(); the single write indisconnectClientByIdentifier(amqp091.go:1337) moves toStore(true).clientDisconnect: falseline in theConnectstruct literal and the threebd.clientDisconnect = falsere-initializations inTest_connect_connecting_*were dropped —atomic.Bool's zero value is already false.Test plan
go test -race -run "Test_StreamRetry|Test_Disconnect$|Test_SubscribeStream" -count=5 ./internal/provider/connectors/amqp091/...—Test_Disconnect(amqp091_test.go:1721) and theTest_SubscribeStream*family (stream_test.go:584, …) hit the samedisconnectClientByIdentifier↔connectionWatcherrace asTest_StreamRetryand are also cleared by this fix.go test -race ./internal/provider/connectors/amqp091/...(note: pre-existingbd.stateraces inTest_Retry,Test_WaitForConnect,Test_connect_connecting_*are out of scope for this PR)go vet ./...Verification (2026-05-30):
-race -count=5command passes cleanly 3/3 — theclientDisconnectrace is fixed.go test -raceshows only the documented out-of-scope failures (Test_Retry,Test_WaitForConnect,Test_connect_connecting_*— thebd.stateraces owned by fix(amqp091): close BrokerDetails.state and Test_Retry races #104); noclientDisconnectraces remain. With fix(amqp091): close BrokerDetails.state and Test_Retry races #104 merged the full package run passes (verified on a combined branch of the whole stack).go vet ./...clean aside from one pre-existing, unrelated warning (test/config/env.go:60, also onmain).