client: fix send on closed channel panic during disconnect#459
Draft
booxter wants to merge 3 commits into
Draft
Conversation
booxter
force-pushed
the
fix-trafficseen-close-race
branch
from
February 6, 2026 01:05
bf6cfe1 to
4399239
Compare
booxter
force-pushed
the
fix-trafficseen-close-race
branch
from
March 4, 2026 16:47
4399239 to
a607e03
Compare
Contributor
Author
|
I've disabled the test case for now so that CI is happier. |
booxter
marked this pull request as ready for review
March 4, 2026 16:47
booxter
marked this pull request as draft
March 4, 2026 17:55
When handleDisconnectNotification() closes the trafficSeen channel,
there's a race window where transact() may attempt to send on the
already-closed channel, causing a panic:
goroutine 1 (transact): goroutine 2 (handleDisconnectNotification):
if trafficSeen != nil {
close(trafficSeen)
trafficSeen <- struct{}{} // PANIC: send on closed channel
}
This was observed in CI as a coredump during e2e tests when the
services controller was syncing a service while the OVSDB connection
was being closed.
Fix this by:
1. Adding a disconnected flag (protected by a mutex) that is set before
stopping handlers in handleDisconnectNotification()
2. Checking this flag in transact() before sending to trafficSeen
3. Not closing trafficSeen in handleDisconnectNotification() - instead
let handleInactivityProbes() clean up when it exits via stopCh
The disconnected flag prevents sends to trafficSeen without the race
condition that would occur if we closed the channel explicitly.
This follows the same pattern as the isShutdown() mechanism proposed in:
ovn-kubernetes#457
A test using a TCP proxy to simulate abrupt network disconnections is
included to verify the fix. The test is disabled by default in short
mode because it revealed several data races in other places in this
library as well as in rpc2 dependency. The test is disabled for now to
avoid breaking CI job, with the expectation that eventually it will be
enabled once known races are fixed.
Fixes ovn-kubernetes#464
Signed-off-by: Ihar Hrachyshka <ihrachyshka@nvidia.com>
Assisted-by: opus (claude-opus-4-5-20251101)
booxter
force-pushed
the
fix-trafficseen-close-race
branch
from
March 4, 2026 20:08
a607e03 to
74c9978
Compare
…calls Move the start of handleDisconnectNotification goroutine to after all handlerShutdown.Add() calls. This prevents a race where handleDisconnectNotification could call handlerShutdown.Wait() during reconnection before all handlers have been registered, causing a premature return from Wait(). The goroutine must be started after all handler registration is complete to ensure Wait() will block until all handlers have actually shut down. Signed-off-by: Ihar Hrachyshka <ihrachyshka@nvidia.com> Assisted-by: opus (claude-opus-4-5-20251101)
TestOpsWaitForReconnect was failing because after Disconnect() is called, the connected flag is set to false immediately, but rpcClient is only set to nil later by handleDisconnectNotification(). This creates a window where SetOption() would check rpcClient != nil and fail even though the client is disconnected. Fix by checking the connected flag instead of rpcClient in SetOption(). This is the correct check since connected accurately reflects the client state from the caller's perspective. Fixes TestOpsWaitForReconnect integration test. Signed-off-by: Ihar Hrachyshka <ihrachyshka@nvidia.com> Assisted-by: opus (claude-opus-4-5-20251101)
booxter
force-pushed
the
fix-trafficseen-close-race
branch
from
March 4, 2026 20:10
74c9978 to
6084840
Compare
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.
When handleDisconnectNotification() closes the trafficSeen channel,
there's a race window where transact() may attempt to send on the
already-closed channel, causing a panic:
This was observed in CI as a coredump during e2e tests when the
services controller was syncing a service while the OVSDB connection
was being closed.
Fix this by adding a disconnected flag (protected by a mutex) that is
set before closing the trafficSeen channel. The transact() function
checks this flag before attempting to send on the channel.
This follows the same pattern as the isShutdown() mechanism proposed in:
#457
A test using a TCP proxy to simulate abrupt network disconnections is
included to verify the fix.