Skip to content

Fix cache instance divergence when custom ConnectionProvider is used with client builders - #4664

Open
AmariahAK wants to merge 3 commits into
redis:masterfrom
AmariahAK:master
Open

Fix cache instance divergence when custom ConnectionProvider is used with client builders#4664
AmariahAK wants to merge 3 commits into
redis:masterfrom
AmariahAK:master

Conversation

@AmariahAK

@AmariahAK AmariahAK commented Jul 30, 2026

Copy link
Copy Markdown

Fixes #4588

What was the issue

When a custom ConnectionProvider is passed to a client builder (e.g. MultiDbClient.builder().connectionProvider(myProvider).build() or RedisClient.builder().connectionProvider(p).build()), and that provider internally carries a Cache, UnifiedJedis.getCache() returns null. The connections use client-side caching, but the client API reports none — the cache instance diverges.

Where was the issue

In AbstractClientBuilder.build(). The builder's cache field is only populated via .cache() / .cacheConfig(). When a custom provider is injected via .connectionProvider(...), the builder skips createDefaultConnectionProvider() (which would have passed the builder's cache) and passes this.cache (still null) directly to the client constructor. Meanwhile, the custom provider — built externally with a cache — creates cache-enabled connections, creating a mismatch.

How it was fixed

Three small, orthogonal changes:

  1. ConnectionProvider interface — added default Cache getCache() returning null (safe for all existing implementations that don't use cache).

  2. Four provider implementations — each now stores and exposes its cache via @Override getCache():

    • MultiDbConnectionProvider — already had the field, just added the getter
    • SentineledConnectionProvider — already had the field, just added the getter
    • PooledConnectionProvider — now stores the cache (was previously passing it to the pool but discarding the reference)
    • ClusterConnectionProvider — now stores a separate clientSideCache field alongside the existing JedisClusterInfoCache cache
  3. AbstractClientBuilder.build() — after the provider is finalised, if this.cache is still null, pull it from connectionProvider.getCache(). This bridges the gap: a custom provider's cache now reaches UnifiedJedis.

Why this approach

  • Minimal diff — one new interface method (with a safe default), one per-provider getter, five lines in the builder.
  • No API breakagedefault method means all existing ConnectionProvider implementations compile unchanged.
  • Reuses existing patternsConnectionProvider already has default methods (getConnectionMap, getPrimaryNodesConnectionMap); this follows the same pattern. The builder already has type-specific knowledge of providers; the getCache() bridge is in that same spirit.
  • Covers the future — any ConnectionProvider implementation that holds a cache can override getCache() and the builder will pick it up automatically.

How to test locally

# Unit tests (no Redis needed):
mvn -Dtest=CustomProviderCacheTest test

# Integration tests (requires Docker):
make start version=8.6
mvn -Dtest=MultiDbCSCTest test
make stop

# Existing MultiDbCacheTest integration tests:
make start version=8.6
mvn -Dtest=MultiDbCacheTest test
make stop

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **Low Risk**
> Additive API (default getCache()) and builder wiring only; no auth or connection semantics change beyond exposing the existing cache reference.
> 
> **Overview**
> Fixes **cache instance divergence** when clients are built with a **custom `ConnectionProvider`** that already enables client-side caching: connections use CSC, but **`getCache()` returned `null`**.
> 
> **`ConnectionProvider`** gains a default **`getCache()`** (null when unused). Built-in providers (**`PooledConnectionProvider`**, **`ClusterConnectionProvider`**, **`MultiDbConnectionProvider`**, **`SentineledConnectionProvider`**) override it; **`PooledConnectionProvider`** now keeps the cache reference (and can read it from **`ConnectionFactory`**). **`ConnectionFactory`** exposes **`getCache()`** for the cache wired at build time.
> 
> **`AbstractClientBuilder.build()`** copies **`connectionProvider.getCache()`** into the builder’s cache when no explicit **`.cache()`** / **`.cacheConfig()`** was set, so **`UnifiedJedis.getCache()`** matches the provider’s cache.
> 
> Adds **`CustomProviderCacheTest`** and asserts **`getCache()`** in **`MultiDbCSCTest`**.
> 
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit cc7b5c8c9f95281b6b1ae3a2f3279494d8cb25c4. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->

…with client builders

When a custom ConnectionProvider is passed to a client builder (e.g.
MultiDbClient.builder().connectionProvider(myProvider).build()), and that
provider internally carries a Cache, UnifiedJedis.getCache() returned null
because the builder never learned about the cache. The connections used
client-side caching but the client API reported none.

This fix:
- Adds default Cache getCache() to the ConnectionProvider interface
- Stores and exposes the Cache in all four provider implementations
  (PooledConnectionProvider, MultiDbConnectionProvider,
  ClusterConnectionProvider, SentineledConnectionProvider)
- Bridges the cache in AbstractClientBuilder.build() so the builder picks
  up the cache from a custom provider when none was set explicitly
- Adds unit tests for RedisClient and MultiDbClient with custom providers
- Adds cache assertions to existing MultiDbCSCTest integration tests

Co-authored-by: atlarix-agent <agent@atlarix.dev>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 361bf2029f

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/main/java/redis/clients/jedis/providers/ConnectionProvider.java
- Add @SInCE 8.0 Javadoc to ConnectionProvider.getCache()
- Expose cache from ConnectionFactory so factory-backed
  PooledConnectionProvider constructors preserve it
- PooledConnectionProvider factory constructors now extract
  cache from ConnectionFactory via instanceof check

Fixes the two reviewer comments on PR redis#4664:
1. Missing @SInCE tag on new public API
2. getCache() returning null when PooledConnectionProvider is
   built from a cache-enabled ConnectionFactory

Co-authored-by: atlarix-agent <agent@atlarix.dev>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c8132f3c21

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/main/java/redis/clients/jedis/ConnectionFactory.java
Codex bot flagged the new public method on the public class
as missing the @SInCE tag required by AGENTS.md for all new
public API.

Co-authored-by: atlarix-agent <agent@atlarix.dev>
@ggivo ggivo added waiting-for-triage Still needs to be triaged status: waiting-for-review and removed waiting-for-triage Still needs to be triaged labels Jul 31, 2026
@ggivo

ggivo commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

@AmariahAK

The team is stretched right now and may take some time before we get to it.

Adding @atakavci to take a look as well.
I know you will be off next week. Just in case you have some time.

@AmariahAK

Copy link
Copy Markdown
Author

@ggivo no worries, thanks for taking time to review regardless.
If you need any changes, feel free to tell me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cache instance would diverge or lost when provider or executors are customized with client builders.

2 participants