Skip to content

[reactor-optional] Fix remaining reactive leakage - #3840

Open
a-TODO-rov wants to merge 1 commit into
feature/reactor-optional-1from
refactor-connection-plumbing
Open

[reactor-optional] Fix remaining reactive leakage#3840
a-TODO-rov wants to merge 1 commit into
feature/reactor-optional-1from
refactor-connection-plumbing

Conversation

@a-TODO-rov

@a-TODO-rov a-TODO-rov commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

Follow up of #3697


Note

Medium Risk
Changes how cluster and multi-Sentinel connections are established and retried; behavior is intended to be equivalent but this is on the critical connection path.

Overview
Removes Reactor Mono from Sentinel and cluster connection retry paths so those flows use only CompletionStage/CompletableFuture.

Adds Futures.firstSuccess, which runs ordered async attempts lazily (next supplier only after the previous fails), returns the first success, and on total failure applies a custom errorHandler over failures collected in order. Unit tests cover short-circuit on success, fall-through, and ordered aggregation with suppressed causes.

RedisClient.connectSentinelAsync (multiple sentinels) drops the Mono onErrorResume chain and a side-channel exception queue; each sentinel attempt is a supplier wired through firstSuccess, with RedisConnectionException aggregation matching the prior suppressed-exception behavior.

RedisClusterClient cluster and cluster pub/sub connect*Async use firstSuccess with repeated connectSupplier instances instead of a deferred Mono plus a retry loop; private connect helpers now return CompletionStage and log failures via whenComplete instead of doOnError.

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

@a-TODO-rov
a-TODO-rov requested a review from atakavci July 13, 2026 13:47
.doOnNext(
c -> connection.registerCloseables(closeableResources, clusterWriter, pooledClusterConnectionProvider))
.map(it -> (StatefulRedisClusterConnection<K, V>) it).toFuture();
return Futures.firstSuccess(Collections.nCopies(getConnectionAttempts(), connectSupplier),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

i acknowledge this firstSuccess looks fancy,, and function-wise it would be also pretty ok. but i suggest something more easy to follow and maintain. more intuitional..
semantics of it is a bit implicit, i mean, it is not clear whether it suggests eager execution or bind-next approach.
also in term of implementation, personally i tend to avoid recursion if it is not suggested by nature of task.
how about something like Futures.withFallback(Supplier<CompletableFuture> asycnTask, Supplier<CompletableFuture> fallbackTask).
have an overload with errorHandlers may be,, in case its needed.

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.

2 participants