Skip to content

Tighten Connection.initializeFromClientConfig() visibility to package-private#4548

Merged
ggivo merged 5 commits into
masterfrom
topic/ggivo/tighten-connection-init-visibility
Jun 10, 2026
Merged

Tighten Connection.initializeFromClientConfig() visibility to package-private#4548
ggivo merged 5 commits into
masterfrom
topic/ggivo/tighten-connection-init-visibility

Conversation

@ggivo

@ggivo ggivo commented May 29, 2026

Copy link
Copy Markdown
Collaborator

What

Tightens Connection.initializeFromClientConfig() (no-arg) from public to package-private. Follow-up to #4547.

Why

After #4547, this method has no out-of-package caller — and importantly, no external caller could legitimately use it:

Constructor / Builder path Behaviour
Connection.builder().build() Initializes for you
new Connection(socketFactory, clientConfig) Initializes for you
new Connection(HostAndPort, JedisClientConfig) Initializes for you
new Connection() / (host,port) / (HostAndPort) etc. Leaves clientConfig == null → calling initializeFromClientConfig() would NPE
Connection.Builder.buildUninitialized() Package-private since #4547

There is no public construction path that produces an uninitialized-but-configured Connection. Keeping the method public means: an external caller can only reach it on a fully-initialized connection (where it would re-run the handshake — the exact bug #4547 fixed).

The protected overload initializeFromClientConfig(JedisClientConfig) stays as-is — CacheConnection overrides it.

Breaking change

This is a binary-incompatible change.


Note

Medium Risk
Binary-incompatible API change for any external caller of the no-arg initializer; runtime behavior for supported construction paths should be unchanged.

Overview
Narrows the API surface by making Connection.initializeFromClientConfig() (no-arg) package-private instead of public, with Javadoc stating it is an internal lifecycle step for Builder#build() and pool ConnectionFactory only. The protected initializeFromClientConfig(JedisClientConfig) overload is unchanged for subclasses like CacheConnection.

Strengthens the regression test for TrackingConnectionPool double-init: drops Mockito construction/verify of initializeFromClientConfig() and instead borrows a real connection against TcpMockServer, asserting HELLO is sent exactly once per pooled connection.

Reviewed by Cursor Bugbot for commit 8c607e2. Bugbot is set up for automated code reviews on this repo. Configure here.

ggivo added 2 commits May 29, 2026 18:58
Connection.Builder.build() already initializes the connection, so
FailFastConnectionFactory.makeObject() was running initializeFromClientConfig()
a second time. This re-registered PUBSUB_CONSUMER and re-ran the HELLO/CLIENT
handshake on every MultiDbClient database connection.
Split Builder.build() into buildUninitialized() + init so the existing
factoryTrackedObjects set can wrap the (now sole) init — preserving the
ability for forceDisconnect() to interrupt a connection mid-handshake.
@ggivo ggivo added this to the 8.0.0 milestone May 29, 2026
@ggivo ggivo added the breakingchange Pull request that has breaking changes. Must include the breaking behavior in release notes. label May 29, 2026
@ggivo ggivo requested review from atakavci and uglide May 29, 2026 18:08
@github-actions

github-actions Bot commented May 29, 2026

Copy link
Copy Markdown

Test Results

  203 files  ±0    203 suites  ±0   9m 32s ⏱️ -6s
7 767 tests ±0  6 981 ✅ ±0  786 💤 ±0  0 ❌ ±0 
7 787 runs  ±0  6 999 ✅ ±0  788 💤 ±0  0 ❌ ±0 

Results for commit 8c607e2. ± Comparison against base commit c98737c.

♻️ This comment has been updated with latest results.

@jit-ci

jit-ci Bot commented May 29, 2026

Copy link
Copy Markdown

🛡️ Jit Security Scan Results

CRITICAL HIGH MEDIUM

✅ No security findings were detected in this PR


Security scan by Jit

Follow-up to #4547: no out-of-package caller remains.
@ggivo ggivo changed the base branch from master to topic/ggivo/fix-tracking-pool-double-init May 29, 2026 18:13

@uglide uglide left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@atakavci atakavci left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

Base automatically changed from topic/ggivo/fix-tracking-pool-double-init to master June 8, 2026 07:37
ggivo added 2 commits June 10, 2026 09:14
…-connection-init-visibility

# Conflicts:
#	src/test/java/redis/clients/jedis/mcf/TrackingConnectionPoolInitTest.java
@ggivo ggivo merged commit 60b6eaa into master Jun 10, 2026
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breakingchange Pull request that has breaking changes. Must include the breaking behavior in release notes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants