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

Make the Agent Cache more Context aware #8092

Merged
merged 1 commit into from
Jun 15, 2020
Merged

Conversation

mkeeler
Copy link
Member

@mkeeler mkeeler commented Jun 11, 2020

Blocking queries issues will still be uncancellable (that cannot be helped until we get rid of net/rpc). However this makes it so that if calling getWithIndex (like during a cache Notify go routine) we can cancell the outer routine. Previously it would keep issuing more blocking queries until the result state actually changed.

This prevents leaking of go routines by the agents ServiceManager used for resolving a services full configuration with central config entries.

TODOS:

  • Unit Test?

Copy link
Contributor

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

Nice! Looks good, and makes sense to me.

Some questions/suggestions on a couple of places where TODO or req.Context() are not used.

agent/agent_endpoint.go Outdated Show resolved Hide resolved
agent/cache-types/connect_ca_leaf.go Show resolved Hide resolved
agent/cache/cache.go Outdated Show resolved Hide resolved
agent/cache/cache.go Outdated Show resolved Hide resolved
Copy link
Contributor

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

LGTM

@mkeeler mkeeler marked this pull request as ready for review June 12, 2020 00:36
Blocking queries issues will still be uncancellable (that cannot be helped until we get rid of net/rpc). However this makes it so that if calling getWithIndex (like during a cache Notify go routine) we can cancell the outer routine. Previously it would keep issuing more blocking queries until the result state actually changed.
@mkeeler mkeeler merged commit 8837907 into master Jun 15, 2020
@mkeeler mkeeler deleted the bugfix/cache-notify-leak branch June 15, 2020 15:01
hashicorp-ci pushed a commit that referenced this pull request Jun 15, 2020
Blocking queries issues will still be uncancellable (that cannot be helped until we get rid of net/rpc). However this makes it so that if calling getWithIndex (like during a cache Notify go routine) we can cancell the outer routine. Previously it would keep issuing more blocking queries until the result state actually changed.
@hanshasselberg
Copy link
Member

Judging by the number of mentions, this wasn't cherry-picked into every branch

@mkeeler
Copy link
Member Author

mkeeler commented Jun 15, 2020

It wasn't cherry-pickable into release/1.6.x or release/1.7.x due to other changes to the cache. If we want it in those branches at any time we will have to backport more than just this manually.

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.

4 participants