Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cache/v3: fix loop variable scope issue and ensure no nil cancel is returned #689

Merged
merged 1 commit into from
May 19, 2023

Conversation

dfawley
Copy link
Member

@dfawley dfawley commented May 9, 2023

With loop variables, the variable is reused on all iterations, meaning the value of i for all of the worker sub-tests, run in parallel, is not reliably incrementing.

Fixing the issue leads to a panic, because CreateWatch can return nil (?). This seems like an awkward and undesirable API, so I also made CreateWatch return func(){} instead. If you'd prefer me to if cancel != nil { defer cancel() } instead, I can make that change, but I thought this change would be better.

@@ -389,7 +389,7 @@ func (cache *snapshotCache) CreateWatch(request *Request, streamState stream.Str
request.ResourceNames, nodeID, err)
}

return nil
return func() {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

@jpeach
Copy link
Contributor

jpeach commented May 16, 2023

@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>
@dfawley
Copy link
Member Author

dfawley commented May 16, 2023

@dfawley Can you please sign off the second commit so the DCO action will pass? If you like, squash them and force-push.

Done, sorry!

@jpeach jpeach merged commit dec76ca into envoyproxy:main May 19, 2023
@jpeach
Copy link
Contributor

jpeach commented May 19, 2023

Thanks @dfawley 👍

@dfawley dfawley deleted the testfix branch May 23, 2023 18:26
@jpeach jpeach mentioned this pull request May 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants