Fix ClientOptions race condition during async connection - #3891
Fix ClientOptions race condition during async connection#3891aartisonigra wants to merge 1 commit into
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 3 potential issues.
Reviewed by Cursor Bugbot for commit 49e24f1. Configure here.
|
|
||
| StatefulRedisPubSubConnectionImpl<K, V> connection = newStatefulRedisPubSubConnection(endpoint, writer, codec, timeout); | ||
| StatefulRedisPubSubConnectionImpl<K, V> connection = newStatefulRedisPubSubConnection(endpoint, writer, codec, | ||
| timeout, clientOptions); |
There was a problem hiding this comment.
Subclass factory hooks bypassed
High Severity
Async connect paths now call the new five-argument newStatefulRedis* factories directly, so subclasses that override the documented four-argument hooks are never invoked. That breaks existing extensibility, including MyExtendedRedisClient and its integration test that expects a MyPubSubConnection.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 49e24f1. Configure here.
| */ | ||
| protected <K, V> StatefulRedisConnectionImpl<K, V> newStatefulRedisConnection(RedisChannelWriter channelWriter, | ||
| PushHandler pushHandler, RedisCodec<K, V> codec, Duration timeout, ClientOptions clientOptions) { | ||
| return new StatefulRedisConnectionImpl<>(channelWriter, pushHandler, codec, timeout, clientOptions.getJsonParser()); |
There was a problem hiding this comment.
Endpoint still races on options
High Severity
createEndpoint and createPubSubEndpoint still call getOptions() after options were already snapshotted for the connection. Concurrent setOptions() can leave DefaultEndpoint/PubSubEndpoint with different ClientOptions than CommandHandler, handshake, and ConnectionBuilder, which is the inconsistency issue #3591 calls out.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 49e24f1. Configure here.
| * @return new instance of StatefulRedisPubSubConnectionImpl | ||
| */ | ||
| protected <K, V> StatefulRedisPubSubConnectionImpl<K, V> newStatefulRedisPubSubConnection(PubSubEndpoint<K, V> endpoint, | ||
| RedisChannelWriter channelWriter, RedisCodec<K, V> codec, Duration timeout, ClientOptions clientOptions) { |
There was a problem hiding this comment.
PubSub options parameter unused
Medium Severity
The new newStatefulRedisPubSubConnection overload accepts clientOptions but never uses it, and still builds the connection with the default JSON parser. The four-argument overload also re-reads getOptions() instead of the ThreadLocal used by the standalone and sentinel factories, so Pub/Sub stays inconsistent with the rest of this fix.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 49e24f1. Configure here.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 49e24f1445
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| StatefulRedisPubSubConnectionImpl<K, V> connection = newStatefulRedisPubSubConnection(endpoint, writer, codec, | ||
| timeout, clientOptions); |
There was a problem hiding this comment.
With subclasses that override only the existing 4-argument newStatefulRedisPubSubConnection(...) hook, such as MyExtendedRedisClient in the extensibility test, this call now dispatches to the new 5-argument overload instead, whose default returns StatefulRedisPubSubConnectionImpl. That bypasses the documented extension point and makes connectPubSub() stop returning the subclass connection, so the existing MyPubSubConnection assertion path regresses unless the new flow preserves or delegates to the old hook.
Useful? React with 👍 / 👎.
| ClientOptions clientOptions = getOptions(); | ||
| clientOptionsThreadLocal.set(clientOptions); | ||
| try { | ||
| DefaultEndpoint endpoint = createEndpoint(); |
There was a problem hiding this comment.
Although this method captures ClientOptions before entering the setup block, the endpoint is still built through createEndpoint(), whose default implementation calls getOptions() again. If setOptions() runs in that window, the DefaultEndpoint can pick up newer request-queue/reconnect settings while the writer, handler, auth, and handshake use the captured options, so a single new connection still observes mixed options; pass the captured options into the endpoint factory, and do the same for the Pub/Sub endpoint factory, to close the race.
Useful? React with 👍 / 👎.
| protected <K, V> StatefulRedisPubSubConnectionImpl<K, V> newStatefulRedisPubSubConnection(PubSubEndpoint<K, V> endpoint, | ||
| RedisChannelWriter channelWriter, RedisCodec<K, V> codec, Duration timeout, ClientOptions clientOptions) { |
There was a problem hiding this comment.
This newly added protected overload is an extension point in RedisClient, but its Javadoc omits @since; the other new ClientOptions-aware factory overloads do the same. Add the current release tag (7.7 after dropping -SNAPSHOT) so generated API docs remain versioned for new public API.
AGENTS.md reference: AGENTS.md:L145-L146
Useful? React with 👍 / 👎.
| StatefulRedisConnectionImpl<K, V> connection = newStatefulRedisConnection(writer, endpoint, codec, timeout, | ||
| clientOptions); |
There was a problem hiding this comment.
For clients that subclass RedisClient and override the existing 4-argument newStatefulRedisConnection(...) hook, this changed call now statically selects the new 5-argument overload on the base class, so connect()/connectAsync() silently stop creating the subclass connection. Bridge existing overrides when adding the ClientOptions-aware overload so normal connection customization remains effective.
Useful? React with 👍 / 👎.
|
|
||
| connectionBuilder(getSocketAddressSupplier(redisURI), connectionBuilder, connection.getConnectionEvents(), redisURI); | ||
| connectionBuilder.connectionInitializer(createHandshake(state)); | ||
| connectionBuilder.connectionInitializer(createHandshake(state, clientOptions)); |
There was a problem hiding this comment.
If a subclass overrides the existing protected createHandshake(ConnectionState) hook to customize activation, this direct call to the new two-argument overload bypasses that override for standalone connections. In those subclassed clients the custom handshake is ignored even though the code still compiles, so adapt the captured-options path without skipping existing one-argument overrides.
Useful? React with 👍 / 👎.
| StatefulRedisSentinelConnectionImpl<K, V> connection = newStatefulRedisSentinelConnection(writer, codec, timeout, | ||
| clientOptions); |
There was a problem hiding this comment.
For subclasses that override the existing 3-argument newStatefulRedisSentinelConnection(...) hook, this call now binds to the new 4-argument overload on RedisClient instead of dispatching through the overridden method. Sentinel connections from those clients therefore revert to the default StatefulRedisSentinelConnectionImpl, so the new overload should be bridged without bypassing existing custom factories.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Hey @aartisonigra,
Thanks for your interest in Lettuce and for taking the time to work on this.
I think the PR still needs some work before it’s ready for a full review. If you’d like to continue with it, I’d suggest starting by adding a test that reproduces the issue first, and then building the fix on top of that. That will make it much easier to validate the proposed solution.
It would also be helpful to take a look at the currently failing CI tests and address the comments left by the AI code review bots.
Thanks again for contributing — happy to take another look once those pieces are in place.
2 participants
Summary
Fixes a race condition where concurrent
setOptions()calls during asyncconnection establishment could cause different components of the same
connection to observe different
ClientOptions.Changes
ClientOptionsonce at async connection entry points.ClientOptionsfor Standalone, Pub/Sub, and Sentinel connections.ClientOptions-aware connection initialization methods.Checklist
ClientOptionsonce during async connection setupClientOptionsto connection initializationsetOptions()Fixes #3591
Note
Medium Risk
Touches core connection bootstrap for all
RedisClientconnect modes; behavior change is intentional butThreadLocaland factory-hook semantics need careful review for subclass overrides and cross-thread connect.Overview
Fixes a race where concurrent
setOptions()during async connect could leave handlers, handshake, builders, and connection objects on differentClientOptionssnapshots.RedisClientnow readsClientOptionsonce at the start of standalone, pub/sub, and sentinel async connect paths, stores it in aThreadLocalfor factory hooks, and threads that snapshot through writers,CommandHandler/PubSubCommandHandler,connectStatefulAsync, auth,ConnectionBuilder, and connection constructors. TheThreadLocalis cleared infinallyblocks.AbstractRedisClientaddscreateHandshake(state, clientOptions)so handshake uses the captured options instead of re-reading client state. NewClientOptions-aware overloads ofnewStatefulRedisConnection,newStatefulRedisPubSubConnection, andnewStatefulRedisSentinelConnectionkeep subclass overrides compatible via delegating no-arg overloads.Reviewed by Cursor Bugbot for commit 49e24f1. Bugbot is set up for automated code reviews on this repo. Configure here.