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
@github-actions
Copy link

github-actions bot commented Oct 13, 2025

Test Results

   280 files  +  1    280 suites  +1   11m 29s ⏱️ -14s
10 188 tests +125  9 123 ✅  - 895  1 065 💤 +1 020  0 ❌ ±0 
 2 703 runs  +  2  2 703 ✅ +  2      0 💤 ±    0  0 ❌ ±0 

Results for commit d296279. ± Comparison against base commit 1d8d075.

This pull request skips 1011 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 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
Copy link

@jit-ci jit-ci bot left a 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.

Copy link
Contributor

@atakavci atakavci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ggivo ggivo self-assigned this Oct 21, 2025
@ggivo ggivo added the bug label Oct 21, 2025
@ggivo ggivo added this to the 7.0.1 milestone Oct 21, 2025
@ggivo ggivo merged commit 4ddd7f4 into master Oct 21, 2025
14 of 18 checks passed
@ggivo ggivo deleted the ggivo/4323-sentineled-scan-leak branch October 21, 2025 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ScanIteration (JedisCommandIterationBase) connection leak with JedisSentineled

3 participants