fix: guard capabilities map with RWMutex#3092
Conversation
There was a problem hiding this comment.
Pull request overview
Guards EngineClient.capabilities against concurrent access by introducing an RWMutex, addressing a potential data race between capability exchange (writes) and capability checks (reads).
Changes:
- Add
capabilitiesMu sync.RWMutextoEngineClientand protectHasCapabilityreads withRLock. - Protect
ExchangeCapabilitiesmap writes withLock. - Add a concurrency/race-focused unit test for concurrent read/write access to the capabilities map.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
execution/client/client.go |
Adds an RWMutex to EngineClient and locks HasCapability reads. |
execution/client/engine.go |
Wraps capability map writes during ExchangeCapabilities with a mutex. |
execution/client/client_test.go |
Introduces a test intended to exercise concurrent read/write behavior under the race detector. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Capture and log the capabilities that the execution client has. | ||
| s.capabilitiesMu.Lock() | ||
| for _, capability := range result { | ||
| s.logger.Info("Exchanged capability", "capability", capability) |
There was a problem hiding this comment.
The write lock is held while calling s.logger.Info(...) for each capability. Since logging can block, this unnecessarily prolongs the critical section and can starve concurrent HasCapability readers. Consider logging outside the lock and only holding the lock around the map writes (or locking per write) to minimize contention.
| // Capture and log the capabilities that the execution client has. | |
| s.capabilitiesMu.Lock() | |
| for _, capability := range result { | |
| s.logger.Info("Exchanged capability", "capability", capability) | |
| // Log the capabilities that the execution client has. | |
| for _, capability := range result { | |
| s.logger.Info("Exchanged capability", "capability", capability) | |
| } | |
| // Capture the capabilities that the execution client has. | |
| s.capabilitiesMu.Lock() | |
| for _, capability := range result { |
| for range iterations { | ||
| c.capabilitiesMu.Lock() | ||
| c.capabilities[cap] = struct{}{} | ||
| c.capabilitiesMu.Unlock() | ||
| } | ||
| }() | ||
|
|
||
| // Goroutine 2: repeatedly read a capability (simulates HasCapability). | ||
| go func() { | ||
| defer wg.Done() | ||
| for range iterations { | ||
| c.HasCapability(cap) | ||
| } |
There was a problem hiding this comment.
for range iterations does not compile because iterations is an untyped integer constant, not an iterable. Use a counted loop (e.g., incrementing index) or range over a slice of length iterations so the test builds and can be exercised under go test -race.
There was a problem hiding this comment.
for range iterations is valid Go 1.22+ syntax for ranging over an integer constant, and this repo targets Go 1.26, so this compiles and runs correctly
|
can anyone from Berachain team please take a look to this PR |
Summary
EngineClient.capabilitiesmap had no mutex protecting it, while ExchangeCapabilities (write) and HasCapability (read) can be called concurrently from different goroutinesExchangeCapabilitiesrepeatedly while ReportingService callsHasCapabilityevery 5 minutes independently