fix: guard ContainerManager.active map with a mutex - #10049
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a mutex to the ContainerManager to synchronize access to the active map within the checkPod method. While this addresses potential concurrency issues, the reviewer pointed out that holding the lock while calling external notification functions could lead to contention or blocking, suggesting a refactor to minimize the lock's scope.
| d.mu.Lock() | ||
| defer d.mu.Unlock() |
There was a problem hiding this comment.
Holding the mutex while calling external notification functions (such as notifyDebuggingContainerStarted and debuggingContainerStartedV2) is generally discouraged. These functions eventually send events over channels, which could block if the channel is full or the consumer is slow. Holding the lock during these operations can lead to increased contention and potentially stall the pod watcher goroutine.
Consider refactoring the loop to collect the necessary information for notifications into a local slice, then dispatching the notifications after releasing the lock.
There was a problem hiding this comment.
That makes sense. Alternatively, the mutex could be locked and unlocked inside the loop just for the duration of the map access, but then defer can't be used as easily and the mutex may get acquired and released many times, which may impact performance negatively.
There was a problem hiding this comment.
So is current implementation fine?
ContainerManager.active is a plain map accessed from the goroutine in Start (via checkPod) without synchronisation. Under concurrent pod events this causes a fatal "concurrent map read and map write" crash. Add a sync.Mutex and lock it around the map access in checkPod. Fixes GoogleContainerTools#9081 Signed-off-by: alliasgher <alliasgher123@gmail.com>
9ecee1d to
77c556b
Compare
2 participants
Fixes #9081. ContainerManager.active is a plain map accessed from the goroutine in Start (via checkPod) without synchronisation, causing a fatal "concurrent map read and map write" crash. Add a sync.Mutex and lock around the map access.