cache/v3: fix loop variable scope issue and ensure no nil cancel is returned #689
Conversation
| @@ -389,7 +389,7 @@ func (cache *snapshotCache) CreateWatch(request *Request, streamState stream.Str | |||
| request.ResourceNames, nodeID, err) | |||
| } | |||
|
|
|||
| return nil | |||
| return func() {} | |||
There was a problem hiding this comment.
Yeh, I agree that the nil return is ambiguous. I can mean that there was a failure to sends an immediate response, or that an immediate response was sent successfully. In other CreateWatch implementations, it can also mean that there was an error setting up the watch (e.g. typeURL mismatch in the linear cache).
So, overall I agree with you that always returning a no-op cancellation function is an improvement to the API (though we should update the API docs and make the same change in other cache implementation).
But, it seems like there is still a need to propagate errors out of this CreateWatch. Maybe we need to change the signature to
func (ConfigWatcher).CreateWatch(*discovery.DiscoveryRequest, stream.StreamState, chan Response) (err error, cancel func())But we would probably want to change CreateDeltaWatch too, and that's an incompatible change for all the consumers of the API.
Alternatively, we could update just the case to only return nil when the cache.respond fails? That makes the semantics a bit more consistent, and makes the tests pass.
cc @alecholmez
There was a problem hiding this comment.
We have an open internal bug about this loop variable reuse behavior, so I'm fine with doing whatever it takes to get the immediate issue of the test resolved ASAP. If you don't want me to change the code's behavior like this, I can instead make the test check if the callback is nil before deferring it.
I do think the API you proposed looks better (but err always comes last by convention).
Also FYI this Go proposal was created at almost the same time about the same thing: golang/go#60078
There was a problem hiding this comment.
OK, let's just return func(){} when the response us successfully delivered inline. This is consistent with the current API semantics, and should fix the test too.
There was a problem hiding this comment.
Given this lib is pre-1.0 I'm okay making a slightly better API improvement. I think @jpeach gave a good suggestion having watch creations return: (err error, cancel func()).
The no op cancel func works too so if we want to roll with that I'm good with it. Whatever you guys feel like is the best improvement
There was a problem hiding this comment.
@jpeach I'm not sure if this is what you meant, but I made it so nil is returned when errors occur and func(){} is returned in the other cases. I think an API change here would be fine, but I don't know that I am comfortable enough with (or have enough time for) the potentially cascading set of changes that may be required with an API change.
|
@dfawley Can you please sign off the second commit so the DCO action will pass? If you like, squash them and force-push. |
Signed-off-by: Doug Fawley <dfawley@google.com>
Done, sorry! |
|
Thanks @dfawley 👍 |
With loop variables, the variable is reused on all iterations, meaning the value of
ifor all of the worker sub-tests, run in parallel, is not reliably incrementing.Fixing the issue leads to a panic, because
CreateWatchcan returnnil(?). This seems like an awkward and undesirable API, so I also madeCreateWatchreturnfunc(){}instead. If you'd prefer me toif cancel != nil { defer cancel() }instead, I can make that change, but I thought this change would be better.