Fix race on watcher update check #260
Merged
Conversation
This file contains 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
When watchers are initialized, they wait until the Workload API has streamed back the initial response before returning from NewXXXWatcher. The semantics are intended such that a call to WaitForUpdate afterwards will only complete when the next response has arrived. When an update is received, the flow is to: 1. record the update 2. close the "got first response" channel (only happens once) 3. send on the "updated" channel (to signal callers of WaitForUpdate) However, this sequence is racy, since closing the "got first response" channel first unblocks the newWatcher call then drains the "updated" channel. If the drain happens after step (3) then everything is ok, but if it happens before, then step (3) will send on the channel, which is buffered. This causes the call to WaitForUpdate to unblock even though no update was received. This change fixes the race by swapping steps (2) and (3). The "updated" channel is sent on and THEN the "got first response channel" is closed so that the drain can take place afterwards. Signed-off-by: Andrew Harding <[email protected]>
azdagron
force-pushed
the
azdagron/fix-wait-race
branch
from
c5d02a2 to
61ead82
Compare
amartinezfayo
approved these changes
Jan 17, 2024
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 watchers are initialized, they wait until the Workload API has streamed back the initial response before returning from NewXXXWatcher. The semantics are intended such that a call to WaitForUpdate afterwards will only complete when the next response has arrived.
When an update is received, the flow is to:
However, this sequence is racy, since closing the "got first response" channel first unblocks the newWatcher call then drains the "updated" channel. If the drain happens after step (3) then everything is ok, but if it happens before, then step (3) will send on the channel, which is buffered. This causes the call to WaitForUpdate to unblock even though no update was received.
This change fixes the race by swapping steps (2) and (3). The "updated" channel is sent on and THEN the "got first response channel" is closed so that the drain can take place afterwards.