Repository navigation
Conversation
…recycling Expose dial_timeout, read_timeout, write_timeout, pool_timeout, conn_max_idle_time and max_retries in the redis cache config. conn_max_idle_time defaults to 5m (go-redis default is 30m) so pooled connections are recycled before firewall/NAT idle timeouts (e.g. 10m on GCP) silently drop them, which made cache operations stall on dead connections.
This branch has not been deployed
This file contains hidden or 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
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.
Description
Fixes #596. Related: #337, #259.
The redis cache client had no configurable timeouts, and go-redis keeps idle pooled connections for 30 minutes. When a firewall or NAT silently drops idle connections (e.g. GCP VPC connection tracking after 10 minutes), commands stall on dead connections: each one waits for the 3s read timeout, up to 8 times with retries. That blocks request handling for a long time.
This PR adds optional redis client settings to the cache config:
clients/redis.go: option building moved into a pureredisOptions(cfg)function so it can be unit-tested without a redis server.max_retriesstays at 7, so existing behaviour is preserved. The one change is that idle connections are now recycled after 5m instead of 30m.config/README.mddocuments the new fields.Pull request type
Checklist
go vet)TestReverseProxy_ServeHTTP1/queue_overflow_for_userfails intermittently, and fails the same way on master; it is unrelated to redis.Does this introduce a breaking change?
Further comments
Not in scope here (possible follow-up): setting
ContextTimeoutEnabled: trueso the existing per-operationcontext.WithTimeoutdeadlines incache/redis_cache.go(500ms–2s) also bound socket I/O, and giving the redis transaction registry real deadlines instead ofcontext.Background(). That would change behaviour for existing users, so I kept it separate.