cache: Fix bug where connection errors can cause early cache expiry #9979
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This mostly affects streaming since streaming will immediately return from Fetch calls when the state is Closed on eviction which causes the race condition every time. However this also affects all other cache types if the fetch call happens to return between the eviction and then next time around the Get loop by any client. There is a separate bug that allows cache items to be evicted even when there are active clients which is the trigger here.
…r cache entry is returning fetch errors.
banks
added
theme/streaming
banks
commented
Apr 7, 2021
| @@ -1,3 +1,3 @@ | |||
| ```release-note:bug | |||
| cache: fix a bug in the client agent cache where streaming could potentially leak resources. [[GH-9978](https://github.com/hashicorp/consul/pulls/9978)]. | |||
| cache: fix a bug in the client agent cache where streaming could potentially leak resources. [[GH-9978](https://github.com/hashicorp/consul/pull/9978)]. | |||
There was a problem hiding this comment.
I fixed this in the upstream PR branch too and reset base but it's still showing here 🤷
dnephin
approved these changes
Apr 7, 2021
|
🍒 If backport labels were added before merging, cherry-picking will start automatically. To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/347197. |
|
🍒✅ Cherry pick of commit 1406671 onto |
hashicorp-ci
pushed a commit
that referenced
this pull request
Apr 8, 2021
…9979) Fixes a cache bug where TTL is not updated while a value isn't changing or cache entry is returning fetch errors.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Based on #9978.
This fixes a bug in the agent cache where TTL expiry times are only updated when successful updates are received.
There are two different ways this can cause items to be prematurely evicted from the cache:
Getrequests will go round their loop seeing only errors and not updating the expiry time even though they are still interested (i.e. the value is still "alive").Currently we don't run into this because until streaming, all cache-types had long TTLs of 72 hours.
It's also self-recovering in that the active clients will just cause the cache value to be refetched anyway even though it was evicted. For typical RPC-based cache types the cost is minimal but for leaf certificates, streaming and other possible future types the eviction causes connections to drop and data to be lost and recomputed which negatively effects performance or requires unnecessary work.
For streaming, this causes dropped connections which then take some time to reconnect based on the timing of the client which can mean delayed event delivery.