-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Fix connection leak in scanIteration with JedisSentineled #4323 #4328
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
Conversation
…sing failover client
Override getConnectionMap() in SentineledConnectionProvider to return the pool instead of a raw connection. This prevents connection leaks when using scanIteration() with JedisSentineled. Fixes #4323
Test Results 280 files + 1 280 suites +1 11m 29s ⏱️ -14s Results for commit d296279. ± Comparison against base commit 1d8d075. This pull request skips 1011 tests.♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a connection leak issue in SentineledConnectionProvider when using scanIteration() operations. The problem occurred because connections were allocated from the pool but never returned.
- Overrides
getConnectionMap()andgetPrimaryNodesConnectionMap()to return the pool instead of raw connections - Adds comprehensive test coverage to verify the connection leak fix
- Includes integration tests to ensure compatibility with sentinel-based operations
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
SentineledConnectionProvider.java |
Implements pool-based connection maps to prevent leaks |
SentineledConnectionProviderTest.java |
Adds unit tests verifying no connection leaks occur |
SentinelAllKindOfValuesCommandsIT.java |
Adds integration test class for sentinel command operations |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ The following Jit checks failed to run:
- static-code-analysis-semgrep-pro
#jit_bypass_commit in this PR to bypass, Jit Admin privileges required.
More info in the Jit platform.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Problem
When using
scanIteration()withJedisSentineled, connections are leaked from the pool and never returned.SentineledConnectionProvideruses the defaultConnectionProvider.getConnectionMap()implementation, which allocates a rawConnectionfrom the pool.JedisCommandIterationBaseuses this connection directly (line 68-69) without returning it to the pool.Solution
Override
getConnectionMap()andgetPrimaryNodesConnectionMap()inSentineledConnectionProviderto return the pool itself, consistent withPooledConnectionProvider:This allows
JedisCommandIterationBaseto properly manage connections using try-with-resources (lines 71-73).Note on volatile pool: The iteration captures the pool reference at construction time. Connections from the old pool remain valid even if failover occurs during iteration.
Note on returning a
Pool<Connection>bygetConnectionMapvsConnectionto:JedisCommandIterationBasealready handles bothConnectionandPooltypes (lines 68-76)getConnectionMap()is only used internally by iteration classesSentineledConnectionProviderwithPooledConnectionProvider's existing behaviorscanoperaionsFixes #4323