Repository navigation
Conversation
BlockingConnectionPool.get_connection() treated any readable data on a pooled connection as the leftovers of a connection that was returned before all of its data had been read, and reconnected it. With client-side caching enabled that data is expected: it holds the invalidation messages the server pushed while the connection was idle. Reconnecting throws those messages away and flushes the local cache, so caching never takes effect with a BlockingConnectionPool. ConnectionPool.get_connection() already ignores readable data when the connection has a local cache; do the same in the blocking pool. Signed-off-by: simpleqt <2015900818@qq.com>
simpleqt
requested review from
ahmedsobeh,
bogdanp05,
mkmkme and
smeso
as code owners
October 26, 2026 12:42
Collaborator
|
Thanks for your submissions. We'll review them when we have time. I noticed you've created a plenty of PRs at this point, so I want to ask: are you fixing issues you stumbled upon during your usage of |
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.
Pull Request check-list
Description of change
BlockingConnectionPool.get_connection()reconnects a pooled connection wheneverconnection.can_read()is true. With client-side caching enabled that is wrong: readable data on an idle pooled connection is the push (invalidation) message that the server sent for tracked keys, and_get_from_local_cache()consumes it. Reconnecting discards those messages and flushes the local cache (AbstractConnection.disconnect()), so caching never takes effect with aBlockingConnectionPool, and every invalidation triggers a full disconnect/reconnect (handshake,HELLO,CLIENT TRACKING ON,SELECT).ConnectionPool.get_connection()already guards this case withconnection.client_cache is None, and so does the async pool (valkey/asyncio/connection.py); this change applies the same condition (and comment) to the sync blocking pool.Verification (no server required,
tests/test_connection_pool.py::test_pool_keeps_connection_with_pending_cache_invalidations): a stub connection that reports a pending invalidation message must surviveget_connection()in both pool classes. Before the change it is dropped (1 disconnect) inBlockingConnectionPoolonly; after the change both pools keep it.Prepared with AI assistance; every line was verified by running the cases above.