client/v3: remove cancelled substream from w.substreams before closing recvc#21981
client/v3: remove cancelled substream from w.substreams before closing recvc#21981crawfordxx wants to merge 2 commits into
Conversation
…g recvc When a watch cancel response is received in run(), ws.recvc is closed and the substream is added to the closing map, but was not removed from w.substreams. If a progress-notify response (WatchId == InvalidWatchID) arrives before closingc is drained, broadcastResponse iterates all entries in w.substreams and tries to send on the already-closed recvc channel, causing a panic: "send on closed channel". Fix: delete the cancelled substream from w.substreams immediately after closing its recvc, so that broadcastResponse cannot observe a closed channel. closeSubstream, which is called later when closingc is drained, still performs delete(w.substreams, ws.id); the redundant delete is a safe no-op. Add a regression test that reproduces the race window and confirms broadcastResponse does not panic after the substream is removed. Fixes etcd-io#21969 Signed-off-by: crawfordxx <crawfordxx@users.noreply.github.com>
|
Hi @crawfordxx. Thanks for your PR. I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: crawfordxx The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
| close(ws.recvc) | ||
| delete(w.substreams, ws.id) |
There was a problem hiding this comment.
I commented out the substreams deletion in watch.go and this test still passes. Is that intentional?
There was a problem hiding this comment.
Good catch — you are right. The original test called delete(w.substreams, ws.id) in the test body itself, so broadcastResponse never saw the closed channel regardless of whether the deletion in run() was present. The test was essentially testing a no-op.
I have restructured it into two subtests (e884c38):
-
"panics when closed recvc is still in substreams" — closes
ws.recvcbut does not remove it fromw.substreams, then callsbroadcastResponseand asserts that a panic occurs. This documents the pre-fix bug state and will catch any future regression that re-introduces a closed channel into the broadcast path. -
"no panic after substream removed from substreams" — applies the same eager-delete that
run()now does, then verifiesbroadcastResponsedoes not panic.
The first subtest is the key addition: it would have failed with the original test, and will fail again if the delete is removed from watch.go.
The original TestBroadcastResponseNoPanicOnCancelledSubstream always passed regardless of whether the fix in watch.go was present, because the test itself called delete(w.substreams, ws.id) before invoking broadcastResponse — bypassing the exact code path under test. Restructure the test as two subtests: 1. "panics when closed recvc is still in substreams" — proves that broadcastResponse panics on a closed channel that is still present in w.substreams. This documents the pre-fix bug state and will catch any future regression that re-introduces a closed channel into the broadcast path. 2. "no panic after substream removed from substreams" — proves that eagerly removing the substream (what run() now does) prevents the panic. Signed-off-by: crawfordxx <crawfordxx@users.noreply.github.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: crawfordxx The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
3 participants
Root cause
When a watch cancel response arrives in
run(),ws.recvcis closed andthe substream is added to the
closingmap — but it is not removed fromw.substreams.If a progress-notify response (
WatchId == InvalidWatchID) is receivedbefore
closingcis drained,broadcastResponseiterates every entry inw.substreamsand executes:Sending on an already-closed channel panics with
"send on closed channel".Fix
Delete the cancelled substream from
w.substreamsimmediately afterclose(ws.recvc), closing the race window.closeSubstreamstill callsdelete(w.substreams, ws.id)whenclosingcis drained; the redundantdelete is a safe no-op on a map.
Test
Added
TestBroadcastResponseNoPanicOnCancelledSubstreamwhich reproducesthe race window (close
ws.recvc, remove fromw.substreams, then callbroadcastResponse) and asserts no panic occurs.Fixes #21969