[release-3.5] mvcc: avoid double decrement of watcher gauge on close/cancel race#20066
Conversation
This occurs specifically when the watch is for a compacted revision, as there's
a possible interleaving of cancel/close that invokes the `cancelWatch` function
twice.
It's fairly difficult to provoke the race condition but it is possible to
observe on `main` the racing test can fail with a negative gauge:
```
$ go test ./... -run TestNewWatcherCountGauge/compacted_watch,_close/cancel_race
--- FAIL: TestNewWatcherCountGauge (0.34s)
watchable_store_test.go:86: # HELP etcd_debugging_mvcc_watcher_total Total number of watchers.
# TYPE etcd_debugging_mvcc_watcher_total gauge
-etcd_debugging_mvcc_watcher_total -1
+etcd_debugging_mvcc_watcher_total 0
FAIL
FAIL go.etcd.io/etcd/server/v3/storage/mvcc 0.830s
? go.etcd.io/etcd/server/v3/storage/mvcc/testutil [no test files]
FAIL
```
It seems as though it is partially expected for the cancel function to be
invoked multiple times and to handle that safely (i.e., the existing `ch == nil`
check) - the bug here is that in the `if/else if` branches it comes "too late",
and multiple invocations where `wa.compacted` is true will both decrement the
counter. Shifting the case up one ensures that we can't follow that decrement
branch multiple times.
In fact, it seems logically more sensible to put this `wa.ch == nil` case
_first_, as a guard for the function being invoked multiple times, but moving i
before the sync/unsynced watch set delete functions could have a greater
inadvertent functional impact (i.e., if we never deleted cancelled watches from
these sets it would presumably introduce a leak), so from an abundance of
caution I've made the smallest change I think will fix my issue.
Signed-off-by: Kieran Gorman <kieran@kjgorman.com>
|
Hi @kjgorman. 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 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. |
kjgorman
changed the title
|
/ok-to-test |
|
link to #19600 |
ahrtr
changed the title
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahrtr, kjgorman, serathius The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kjgorman
changed the title
4 participants
This occurs specifically when the watch is for a compacted revision, as there's a possible interleaving of cancel/close that invokes the
cancelWatchfunction twice.It's fairly difficult to provoke the race condition but it is possible to observe on
mainthe racing test can fail with a negative gauge:It seems as though it is partially expected for the cancel function to be invoked multiple times and to handle that safely (i.e., the existing
ch == nilcheck) - the bug here is that in theif/else ifbranches it comes "too late", and multiple invocations wherewa.compactedis true will both decrement the counter. Shifting the case up one ensures that we can't follow that decrement branch multiple times.In fact, it seems logically more sensible to put this
wa.ch == nilcase first, as a guard for the function being invoked multiple times, but moving i before the sync/unsynced watch set delete functions could have a greater inadvertent functional impact (i.e., if we never deleted cancelled watches from these sets it would presumably introduce a leak), so from an abundance of caution I've made the smallest change I think will fix my issue.Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.