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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
May 19, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 6 additions & 3 deletions pkg/cache/v3/simple.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,7 +321,8 @@ func superset(names map[string]bool, resources map[string]types.ResourceWithTTL)
return nil
}

// CreateWatch returns a watch for an xDS request.
// CreateWatch returns a watch for an xDS request. A nil function may be
// returned if an error occurs.
func (cache *snapshotCache) CreateWatch(request *Request, streamState stream.StreamState, value chan Response) func() {
nodeID := cache.hash.ID(request.Node)

Expand Down Expand Up @@ -365,8 +366,9 @@ func (cache *snapshotCache) CreateWatch(request *Request, streamState stream.Str
if err := cache.respond(context.Background(), request, value, resources, version, false); err != nil {
cache.log.Errorf("failed to send a response for %s%v to nodeID %q: %s", request.TypeUrl,
request.ResourceNames, nodeID, err)
return nil
}
return nil
return func() {}
}
}
}
Expand All @@ -387,9 +389,10 @@ func (cache *snapshotCache) CreateWatch(request *Request, streamState stream.Str
if err := cache.respond(context.Background(), request, value, resources, version, false); err != nil {
cache.log.Errorf("failed to send a response for %s%v to nodeID %q: %s", request.TypeUrl,
request.ResourceNames, nodeID, err)
return nil
}

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.

}

func (cache *snapshotCache) nextWatchID() int64 {
Expand Down
2 changes: 1 addition & 1 deletion pkg/cache/v3/simple_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,6 +342,7 @@ func TestSnapshotCacheWatch(t *testing.T) {
func TestConcurrentSetWatch(t *testing.T) {
c := cache.NewSnapshotCache(false, group{}, logger{t: t})
for i := 0; i < 50; i++ {
i := i
t.Run(fmt.Sprintf("worker%d", i), func(t *testing.T) {
t.Parallel()
id := fmt.Sprintf("%d", i%2)
Expand All @@ -358,7 +359,6 @@ func TestConcurrentSetWatch(t *testing.T) {
Node: &core.Node{Id: id},
TypeUrl: rsrc.EndpointType,
}, streamState, value)

defer cancel()
}
})
Expand Down