Summary
PR #60 (fix: synchronize HubConnection control channel lifecycle) introduced mutex-guarded writes to HubConnection.Heartbeat and HubConnection.ControlChannel in Start() and Stop(), along with a ccWg WaitGroup to prevent goroutine leaks across reconnects. However, several read sites in other files still access these fields without acquiring hc.mu, which means data races remain possible under the Go memory model.
Unsynchronized reads of guarded fields
The following locations read conn.Heartbeat or conn.ControlChannel without holding conn.mu.RLock():
handlers.go:179-180 — handleHubConnections() reads both fields for status reporting
server.go:1303-1304 — getFirstHeartbeat() reads conn.Heartbeat (only hubMu is held, not conn.mu)
server.go:1336-1337 — logHubConnections() reads both fields for logging
handlers.go:2091-2092 — forceHeartbeatAll() reads conn.Heartbeat (only hubMu is held)
The hubMu lock protects the connection map, but does not synchronize access to fields within a HubConnection. Since Stop() now writes these fields under hc.mu.Lock(), all readers must also acquire hc.mu.RLock().
Suggested fix
Add accessor methods (e.g., hc.GetHeartbeat(), hc.GetControlChannel()) that acquire hc.mu.RLock(), similar to the existing GetStatus() pattern, and update all call sites.
Missing race-condition test coverage
There are no tests exercising concurrent Start/Stop/Reinitialize paths. A go test -race scenario that calls Reinitialize while a Connect goroutine is in-flight would validate the fix and catch regressions.
Unguarded writes in Reinitialize
In Reinitialize() (hub_connection.go:203-209), fields like Credentials, BrokerID, SecretKey, and HubClient are written without holding hc.mu. This is likely safe today because Stop() serializes via ccWg.Wait() before Start() is called, but it is inconsistent with the locking discipline established by this PR and could become a problem if call patterns change.
Summary
PR #60 (
fix: synchronize HubConnection control channel lifecycle) introduced mutex-guarded writes toHubConnection.HeartbeatandHubConnection.ControlChannelinStart()andStop(), along with accWgWaitGroup to prevent goroutine leaks across reconnects. However, several read sites in other files still access these fields without acquiringhc.mu, which means data races remain possible under the Go memory model.Unsynchronized reads of guarded fields
The following locations read
conn.Heartbeatorconn.ControlChannelwithout holdingconn.mu.RLock():handlers.go:179-180—handleHubConnections()reads both fields for status reportingserver.go:1303-1304—getFirstHeartbeat()readsconn.Heartbeat(onlyhubMuis held, notconn.mu)server.go:1336-1337—logHubConnections()reads both fields for logginghandlers.go:2091-2092—forceHeartbeatAll()readsconn.Heartbeat(onlyhubMuis held)The
hubMulock protects the connection map, but does not synchronize access to fields within aHubConnection. SinceStop()now writes these fields underhc.mu.Lock(), all readers must also acquirehc.mu.RLock().Suggested fix
Add accessor methods (e.g.,
hc.GetHeartbeat(),hc.GetControlChannel()) that acquirehc.mu.RLock(), similar to the existingGetStatus()pattern, and update all call sites.Missing race-condition test coverage
There are no tests exercising concurrent
Start/Stop/Reinitializepaths. Ago test -racescenario that callsReinitializewhile aConnectgoroutine is in-flight would validate the fix and catch regressions.Unguarded writes in
ReinitializeIn
Reinitialize()(hub_connection.go:203-209), fields likeCredentials,BrokerID,SecretKey, andHubClientare written without holdinghc.mu. This is likely safe today becauseStop()serializes viaccWg.Wait()beforeStart()is called, but it is inconsistent with the locking discipline established by this PR and could become a problem if call patterns change.