incrementalXDS does not cancel existing watch

[jparklab]

Cancel on watch object is called when watch does not exist in the map (https://github.com/envoyproxy/go-control-plane/blob/main/pkg/server/delta/v3/server.go#L168). (It is currently no-op since when 'ok' is false, watch will have the zero value. )
Cancel should be called when 'ok' is true since we expect cancel to be called when we find watch in the map.

// cancel existing watch to (re-)request a newer version
watch, ok := watches.deltaWatches[typeURL]
if !ok {
   ...
   // We must signal goroutine termination to prevent a race between the cancel closing the watch
   // and the producer closing the watch.
   watch.Cancel()

[alecholmez]

@snowp I think this is actually a problem that we might have to think about with regards to the nonce handling too.

SOTW will cancel if the nonces are equal:

responseNonce, seen := values.nonces[typeUrl]
if !seen || responseNonce == nonce {
	if cancel, seen := values.cancellations[typeUrl]; seen && cancel != nil {
		cancel()
	}
	values.cancellations[typeUrl] = s.cache.CreateWatch(req, values.responses)
}

But delta does the above and only looks for something that doesn't exist.

[snowp]

Wouldn't we just cancel the watch whenever we get a response? I'm not sure how nonce handling comes into play since its essentially not used in delta atm?

[alecholmez]

Ah yea we could do that, I was trying to rationalize around handling ACKs, etc... but that's a good point if the nonce really isn't used in delta atm then we can just cancel. I'll test this change

[jparklab]

@alecholmez thanks for looking into the issue, and making a patch quickly. I have been waiting for the incremental XDS and it's great to have it.
While I was testing our controller with the latest code, I found that there are a few issues and summarized it in #448. It would be nice if you can take a look at it as well while making a patch for this issue.

[github-actions]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

[alecholmez]

@jparklab This is now patched in the server code. Let me know if this issue is resolved!

[jparklab]

@alecholmez Thanks for the patch. I'll test it with out tests and post an update.

[alecholmez]

I'm going to close this with regards to the patch, but if the issue pops up again we can reopen