Prevents watches from being orphaned when KVS blocking queries loop. #1632
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.
This fixes #1626.
The basic problem is that we were returning the underlying
NotifyGroup
object here:https://github.com/hashicorp/consul/blob/v0.6.0/consul/state/watch.go#L100-L112
This worked fine the first time through, but when a prefix was notified, this
NotifyGroup
would get deleted:https://github.com/hashicorp/consul/blob/v0.6.0/consul/state/watch.go#L140-L143
So in a blocking RPC, once you looped around because the watch was notified, you'd end up calling
Wait()
on aNotifyGroup
that was no longer part of the watch manager:https://github.com/hashicorp/consul/blob/v0.6.0/consul/rpc.go#L342
The solution is to never expose the underlying
NotifyGroup
to the blocking RPC code. I created a wrapper object that re-registers the wait channel with whateverNotifyGorup
is in the tree, or it creates one.This bug was pretty subtle because you had to have these conditions:
This is probably not super common, but this would have been difficult to detect. The query timeout would eventually un-block the first query, but it would report stale results from the first time it looked at the state store.
/cc @armon or @ryanuber