Deprecate StatefulConnection.reset()

[semberal]

Bug Report

Current Behavior

When StatefulRedisConnection is reset, results of subsequent commands are non-deterministic. Results of completely different commands are returned. I extracted minimal example, where there are 2 redis keys: a->a and b->b. Two parallel threads are reading these two keys in a loop. Then, at some point I reset StatefulRedisConnection. After that, the result of client.get("a") can be "b" and vice versa.

Input Code

Minimal example:

package test;

import io.lettuce.core.RedisClient;
import io.lettuce.core.RedisURI;
import io.lettuce.core.api.StatefulRedisConnection;
import io.lettuce.core.api.sync.RedisCommands;
import org.junit.Test;

import java.time.Duration;
import java.util.concurrent.CancellationException;

public class LettuceTest {
    @Test
    public void testParallelExecution() throws InterruptedException {

        final RedisURI uri = new RedisURI("127.0.0.1", 6379, Duration.ofSeconds(1));
        final StatefulRedisConnection<String, String> conn = RedisClient.create(uri).connect();
        final RedisCommands<String, String> sync = conn.sync();
        sync.set("a", "a");
        sync.set("b", "b");

        final Thread a = runThread(sync, "a");
        final Thread b = runThread(sync, "b");

        a.start();
        b.start();

        while(a.isAlive() && b.isAlive()) {
            Thread.sleep(1000);
            conn.reset();
        }

        throw new IllegalStateException("Threads should not terminate");

    }

    private Thread runThread(RedisCommands<String, String> async, String key) {

        return new Thread(() -> {
            while (true) {
                try {
                    final String result = async.get(key);
                    System.out.println(String.format("Expected %s, received %s", key, result));
                    if (!key.equals(result)) break;
                } catch (CancellationException e) {
                    // ok
                }
            }
        });
    }
}

Environment

• Lettuce version(s): 5.1.2.RELEASE
• Redis version: 5.0.0

[mp911de]

This behavior is expected. Calling .reset() should be a last resort that resets the protocol state. You should call reset() only if the connection is idle.

What is your use case to call .reset()?

[semberal]

I am using Lettuce with fairly small max queue size of 20 commands. I am facing an issue that if the command queue is full when Redis is restarted, the client is somehow not reconnected successfully because all commands are rejected because of the full queue. Seems like the queue is not flushed or something. Occasionally resetting the connection seemed to help, but it lead to the problem I reported.

[semberal]

Is it possible that after one such invalid reset() call, the connection is broken forever? When I modify the test to call reset() just once, I am still getting invalid responses a long time after that.

[anagamca]

In our application no where we are invoking conn.reset(). But still we are seeing the same behavior, with HGET command returns different values. Apart from conn.reset(), is there any other scenarios which can start behaving like this?

We are using AWS Elastic Cache Redis Cluster 3.2.6

RedisClusterCommands(RedisAdvancedClusterCommands<String, Object>) defined as class variable and created through constructor. All the HMSET and HGET calls from application made through class variable redisClusterCommands.

Does lettuce client make conn.reset() for anything? If yes, what are all situations it can do so.

Lettuce version we are using,

biz.paluch.redis
lettuce
4.3.3.Final

[mp911de]

@anagamca care to file a new issue along logs and a reproducer?

Regarding reset(): We introduced reset back while we had no SSL support yet. Back then, SSL usage was only possible through a client-side SSL tunnel. If connections broke in the SSL layer, we had to reset the protocol state to recover from external SSL issues.

[mp911de]

@semberal can you provide a reproducer? We should fix the queue size issue instead of trying to recover the client state in the application. This is probably a good time to deprecate .reset() as it causes more harm than good.