MasterReplicaConnectionProvider with zero connections causes IllegalArgumentException

[travispeloton]

Bug Report

I was testing failover scenarios and found that there is an edge case where, after initially starting and connecting to a master-replica, the number of connections in MasterReplicaConnectionProvider can be 0 and ThreadLocalRandom will throw an java.lang.IllegalArgumentException: bound must be positive.

Current Behavior

When there are 0 connections in MasterReplicaConnectionProvider, the following block of code can cause ThreadLocalRandom to throw java.lang.IllegalArgumentException: bound must be positive:

                return connections.filter(StatefulConnection::isOpen).collectList().map(it -> {
                    int index = ThreadLocalRandom.current().nextInt(it.size());

Stack trace

08:49:05.754 [reactor-http-nio-10 @coroutine#9] ERROR o.s.b.a.w.r.e.AbstractErrorWebExceptionHandler - [01e8349c-9]  500 Server Error for HTTP POST "/endpoint"
java.lang.IllegalArgumentException: bound must be positive
	at java.base/java.util.concurrent.ThreadLocalRandom.nextInt(ThreadLocalRandom.java:310)
	Suppressed: reactor.core.publisher.FluxOnAssembly$OnAssemblyException: 
Error has been observed at the following site(s):
	*__checkpoint ⇢ Handler com.XXX.Controller#endpoint(String, ServerWebExchange, Continuation) [DispatcherHandler]
	*__checkpoint ⇢ org.springframework.boot.actuate.metrics.web.reactive.server.MetricsWebFilter [DefaultWebFilterChain]
	*__checkpoint ⇢ HTTP POST "/endpoint" [ExceptionHandlingWebHandler]
Original Stack Trace:
		at java.base/java.util.concurrent.ThreadLocalRandom.nextInt(ThreadLocalRandom.java:310)
		at io.lettuce.core.masterreplica.MasterReplicaConnectionProvider.lambda$getConnectionAsync$0(MasterReplicaConnectionProvider.java:148)
		at reactor.core.publisher.FluxMapFuseable$MapFuseableSubscriber.onNext(FluxMapFuseable.java:113)
		at reactor.core.publisher.Operators$MonoSubscriber.complete(Operators.java:1816)
		at reactor.core.publisher.MonoCollectList$MonoCollectListSubscriber.onComplete(MonoCollectList.java:128)
		at reactor.core.publisher.FluxFilter$FilterSubscriber.onComplete(FluxFilter.java:166)
		at reactor.core.publisher.FluxConcatArray$ConcatArraySubscriber.onComplete(FluxConcatArray.java:230)
		at reactor.core.publisher.FluxConcatArray.subscribe(FluxConcatArray.java:78)
		at reactor.core.publisher.Mono.subscribe(Mono.java:4400)
		at reactor.core.publisher.Mono.subscribeWith(Mono.java:4515)
		at reactor.core.publisher.Mono.toFuture(Mono.java:4920)
		at io.lettuce.core.masterreplica.MasterReplicaConnectionProvider.getConnectionAsync(MasterReplicaConnectionProvider.java:150)
		at io.lettuce.core.masterreplica.MasterReplicaChannelWriter.write(MasterReplicaChannelWriter.java:69)
		at io.lettuce.core.RedisChannelHandler.dispatch(RedisChannelHandler.java:191)
		at io.lettuce.core.StatefulRedisConnectionImpl.dispatch(StatefulRedisConnectionImpl.java:163)
		at io.lettuce.core.RedisPublisher$RedisSubscription.dispatchCommand(RedisPublisher.java:395)
		at io.lettuce.core.RedisPublisher$CommandDispatch$1.dispatch(RedisPublisher.java:466)
		at io.lettuce.core.RedisPublisher$RedisSubscription.checkCommandDispatch(RedisPublisher.java:390)
		at io.lettuce.core.RedisPublisher$State$2.request(RedisPublisher.java:537)
		at io.lettuce.core.RedisPublisher$RedisSubscription.request(RedisPublisher.java:248)
		at reactor.core.publisher.MonoNext$NextSubscriber.request(MonoNext.java:108)
		at reactor.core.publisher.MonoNext$NextSubscriber.request(MonoNext.java:108)
		at reactor.core.publisher.FluxContextWrite$ContextWriteSubscriber.request(FluxContextWrite.java:136)
		at reactor.core.publisher.StrictSubscriber.onSubscribe(StrictSubscriber.java:77)
		at reactor.core.publisher.FluxContextWrite$ContextWriteSubscriber.onSubscribe(FluxContextWrite.java:101)
		at reactor.core.publisher.MonoNext$NextSubscriber.onSubscribe(MonoNext.java:70)
		at reactor.core.publisher.MonoNext$NextSubscriber.onSubscribe(MonoNext.java:70)
		at io.lettuce.core.RedisPublisher$State$1.subscribe(RedisPublisher.java:514)
		at io.lettuce.core.RedisPublisher$RedisSubscription.subscribe(RedisPublisher.java:231)
		at io.lettuce.core.RedisPublisher.subscribe(RedisPublisher.java:130)
		at reactor.core.publisher.MonoFromPublisher.subscribe(MonoFromPublisher.java:63)
		at reactor.core.publisher.Mono.subscribe(Mono.java:4400)
		at kotlinx.coroutines.reactive.AwaitKt.awaitOne(Await.kt:190)
		at kotlinx.coroutines.reactive.AwaitKt.awaitOne$default(Await.kt:183)
		at kotlinx.coroutines.reactive.AwaitKt.awaitFirstOrNull(Await.kt:45)
		at io.lettuce.core.api.coroutines.RedisStringCoroutinesCommandsImpl.get(RedisStringCoroutinesCommandsImpl.kt:67)
		at io.lettuce.core.api.coroutines.RedisCoroutinesCommandsImpl.get$suspendImpl(RedisCoroutinesCommandsImpl.kt)
		at io.lettuce.core.api.coroutines.RedisCoroutinesCommandsImpl.get(RedisCoroutinesCommandsImpl.kt)
		...

Expected behavior/code

The code should probably check for a 0 value and not invoke ThreadLocalRandom.nextInt.

Also, the IllegalArgumentException is passed to io.lettuce.core.internal.Exceptions#bubble and is not wrapped:

        if (throwableToUse instanceof RuntimeException) {
            return (RuntimeException) throwableToUse;
        }

Why not wrap RuntimeExceptions in RedisException, which is also a RuntimeException?

Originally my code was only catching RedisException, but to be safe I am now catching Exception in places that I interact with the Lettuce command classes.

What do you think the correct behavior should be in these two situations?

Environment

• Lettuce version(s): 6.1.6.RELEASE
• Redis version: 6.2.5 on macOS

[mp911de]

RuntimeExceptions are propagated directly as it doesn't make sense to wrap any IAE/ISE/NPE. These exceptions are either bugs or something is odd that isn't directly tied to Redis. Thanks for looking into the matter. The mentioned code from MasterReplicaConnectionProvider assumes there's at least one node to connect to so we need to fix the bug in the client.

[mp911de]

That's fixed now.