Skip to content

Conversation

ggivo
Copy link
Collaborator

@ggivo ggivo commented Oct 13, 2025

Problem

When using scanIteration() with JedisSentineled, connections are leaked from the pool and never returned.

SentineledConnectionProvider uses the default ConnectionProvider.getConnectionMap() implementation, which allocates a raw Connection from the pool. JedisCommandIterationBase uses this connection directly (line 68-69) without returning it to the pool.

Solution

Override getConnectionMap() and getPrimaryNodesConnectionMap() in SentineledConnectionProvider to return the pool itself, consistent with PooledConnectionProvider:

@Override
public Map<?, Pool<Connection>> getConnectionMap() {
  return Collections.singletonMap(currentMaster, pool); 
}
 
@Override
public Map<?, Pool<Connection>> getPrimaryNodesConnectionMap() {
   return Collections.singletonMap(currentMaster, pool);
}

This allows JedisCommandIterationBase to 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> by getConnectionMap vs Connection to:

  • JedisCommandIterationBase already handles both Connection and Pool types (lines 68-76)
  • getConnectionMap() is only used internally by iteration classes
  • This change aligns SentineledConnectionProvider with PooledConnectionProvider's existing behavior
  • Previous behavior would cause connections to be exhausted using multilpe times scan operaions

Fixes #4323

ggivo added 2 commits October 10, 2025 13:06
Override getConnectionMap() in SentineledConnectionProvider to return
the pool instead of a raw connection. This prevents connection leaks
when using scanIteration() with JedisSentineled.

Fixes #4323
Copy link

github-actions bot commented Oct 13, 2025

Test Results

   280 files  +  1    280 suites  +1   11m 39s ⏱️ +16s
10 188 tests +125  9 123 ✅  - 484  1 065 💤 +609  0 ❌ ±0 
 2 703 runs  +  2  2 703 ✅ +  2      0 💤 ±  0  0 ❌ ±0 

Results for commit 8b76086. ± Comparison against base commit 20e7081.

This pull request skips 600 tests.
redis.clients.jedis.commands.commandobjects.CommandObjectsHashCommandsTest[1] ‑ testHgetdel
redis.clients.jedis.commands.commandobjects.CommandObjectsHashCommandsTest[1] ‑ testHgetdelBinary
redis.clients.jedis.commands.commandobjects.CommandObjectsHashCommandsTest[1] ‑ testHgetex
redis.clients.jedis.commands.commandobjects.CommandObjectsHashCommandsTest[1] ‑ testHgetexBinary
redis.clients.jedis.commands.commandobjects.CommandObjectsHashCommandsTest[1] ‑ testHsetex
redis.clients.jedis.commands.commandobjects.CommandObjectsHashCommandsTest[1] ‑ testHsetexBinary
redis.clients.jedis.commands.commandobjects.CommandObjectsHashCommandsTest[2] ‑ testHgetdel
redis.clients.jedis.commands.commandobjects.CommandObjectsHashCommandsTest[2] ‑ testHgetdelBinary
redis.clients.jedis.commands.commandobjects.CommandObjectsHashCommandsTest[2] ‑ testHgetex
redis.clients.jedis.commands.commandobjects.CommandObjectsHashCommandsTest[2] ‑ testHgetexBinary
…

♻️ This comment has been updated with latest results.

@ggivo ggivo marked this pull request as ready for review October 14, 2025 06:45
@ggivo ggivo requested a review from Copilot October 14, 2025 06:46
Copy link
Contributor

@Copilot Copilot AI left a 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() and getPrimaryNodesConnectionMap() 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.

@ggivo ggivo requested review from atakavci and uglide October 14, 2025 06:47
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.

ScanIteration (JedisCommandIterationBase) connection leak with JedisSentineled

1 participant