redis-plus-plus icon indicating copy to clipboard operation
redis-plus-plus copied to clipboard

[FEATURE] Flexibility to set socket_timeout for each request to redis

Open eriktuantran opened this issue 2 years ago • 11 comments

Hi @sewenew I'm using RedisCluster and running into the situation that different socket_timeout values for each request are needed:

  • When a redis instance is going down, I need the socket_timeout small enough to return the error immediately.
  • When triggering a redis command that takes long time to finished, I need another socket_timeout value that long enough to wait for the reply.

The socket_timeout value will be decided by application. However, it looks like that we can not adjust the socket_timeout for each redis request for now. It would be great that we can support this feature. How do you think?

eriktuantran avatar Dec 23 '21 14:12 eriktuantran

When a redis instance is going down, I need the socket_timeout small enough to return the error immediately.

First of all, how can you tell if a redis instance is going down? If it's already down, socket_timeout won't help. Since it will fail to reconnect to Redis, and won't send any command to it.

When triggering a redis command that takes long time to finished, I need another socket_timeout value that long enough to wait for the reply.

First of all, it's not a good idea to run a long running command with Redis, since it's single-threaded, and the long running command will block Redis. If you do need to do that, I think you'd better use a dedicated Redis object to do the job. Because Redis operation is blocking, if a connection is waiting for a command for a long time, the connection will not be returned to the underlying connection pool, and other threads could not reuse this connection either.

It would be great that we can support this feature. How do you think?

If this feature is added, the API might change a lot. I'm not sure if this is a good idea. So I'll keep this issue open, but hold this request for now. If there're many others need this feature, I'll reconsider it.

Anyway, thanks for your proposal!

Regards

Regards

sewenew avatar Dec 27 '21 15:12 sewenew

Hi @sewenew, I actually have a use-case for this request:

I am handling data from sensors that deliver their data at pre-defined intervals. Eg. sensor A, @ a 3 second interval, sensor B @ a 15 second interval, etc. My orchestration of the handling of the data, is data-triggered. This means I need to know if a sensor stopped delivering data. If so, I need to trigger the data handling in any case, marking the data as invalid.

At the moment, I have a C++ process with a thread per sensor. In order not to use a Redis instance per thread, I use a reduced socket_timeout and only handle the data under special conditions, when a timeout occurs. Here is a piece of code, roughly explaining it with a socket_timeout of 4 seconds, sensor A @ 3 sec and sensor B @ 15 sec. The code can be scaled to many sensors/inputs, using only one Redis instance.

sw::redis::ConnectionOptions connectionOptions;
connectionOptions.socket_timeout = std::chrono::seconds(4);
sw::redis::Redis redis(connectionOptions);

std::thread a(processValue(std::ref(redis), "A", std::chrono::seconds(3), std::ref(connectionOptions.socket_timeout)));
std::thread b(processValue(std::ref(redis), "B", std::chrono::seconds(15), std::ref(connectionOptions.socket_timeout)));

void processValue(sw::redis::Redis& redis,
                  const std::string& key,
                  const std::chrono::seconds& interval,
                  const std::chrono::seconds& timeout)
{
    auto sub = redisInstance.subscriber();
    sub.on_message([](std::string key, std::string val) {doCalc(key, val, true);});

    sub.subscribe(key);
    sub.consume();

    std::chrono::seconds subTimeWithoutData(0);
    while (running) {
        try {
                sub.consume();
                subTimeWithoutData = std::chrono::seconds(0);
        }
        catch (const sw::redis::TimeoutError &e) {
            subTimeWithoutData = subTimeWithoutData + timeout;
            if (subTimeWithoutData > interval) {
                // Reduce the subTimeWithoutData accordingly.
                subTimeWithoutData = subTimeWithoutData - interval;
                // Do the calc with invalid data.
                doCalc(key, dummyVal, false);
            }
        }
    }
    sub.unsubscribe(key);
    sub.consume();
}

Having a (p)subscribe or a consume method that takes a timeout value, would simplify the implementation of the above use-case enormously.

BTW, I cannot think of any non-pub/sub kind of commands that would benefit from custom timeouts, as in the real world all non-pub/sub commands should be super fast with their response, otherwise something is wrong in any case.

Regards

wingunder avatar Dec 28 '21 10:12 wingunder

Hi @sewenew and @quoctuanuit,

@quoctuanuit wrote:

When a redis instance is going down, I need the socket_timeout small enough to return the error immediately.

Like @sewenew said, you might have a design problem. It depends on the use-case, but using streams and acking each stream item, once successfully handled, is a way to prevent loosing stuff in some cases. On the other hand, if you're using RedisCluster, your cluster should failover without problem. If your cluster really goes down, you'll either need to add hardware to make it more robust, or simply cope with the disaster.

@sewenew wrote:

First of all, how can you tell if a redis instance is going down?

Dead Redis nodes can be determined independently, by subscribing to specific keys of a Sentinel server. Sentinel can also be used in combination with RedisCluster.

Regards

wingunder avatar Dec 28 '21 11:12 wingunder

@wingunder

I actually have a use-case for this request: .... In order not to use a Redis instance per thread

In this case, you CAN have a Redis instance per thread. Because Redis::subscriber always create a Subscriber with a new connection, and the Redis object creates connection lazily. So even if you have a Redis object per thread, there will be only one connection to Redis server per thread. In a word, in your case, a Redis object per thread works the same as a global Redis object.

std::thread a(processValue("A", std::chrono::seconds(3)));
std::thread b(processValue("B", std::chrono::seconds(15)));

void processValue(const std::string& key,
                  const std::chrono::seconds& timeout)
{
    sw::redis::ConnectionOptions connectionOptions;
    connectionOptions.socket_timeout = timeout;
    sw::redis::Redis redis(connectionOptions);           // <------ since no command is sent with Redis object, no connection will be created.
    auto sub = redis.subscriber();   // <----- always create a new connection.
    sub.on_message([](std::string key, std::string val) {doCalc(key, val, true);});

    sub.subscribe(key);
    sub.consume();

    while (running) {
        try {
                sub.consume();
        }
        catch (const sw::redis::TimeoutError &e) {
                doCalc(key, dummyVal, false);
            }
        }
    }
    sub.unsubscribe(key);
    sub.consume();
}

Regards

sewenew avatar Dec 29 '21 14:12 sewenew

@wingunder

Dead Redis nodes can be determined independently, by subscribing to specific keys of a Sentinel server.

Yes, you can be notified by Sentinel, but only when the node is already down. However, if I understood correctly, I think @quoctuanuit want to modify the timeout before the node is down, i.e. the node is still alive, but is going to be down.

Correct me if I misunderstand his question, since I'm not a native speaker :)

Regards

sewenew avatar Dec 29 '21 14:12 sewenew

Hi @sewenew,

In this case, you CAN have a Redis instance per thread.

Thanks for your explanation and your example. The missing clue for me was:

    sw::redis::Redis redis(connectionOptions); // <------ since no command is sent with Redis object, no connection will be created.

Would it be possible to mention this example in the documentation, or maybe even enhance the documentation with an example like this?

BTW, this information could also have been helpful for #240.

Regards

wingunder avatar Dec 29 '21 15:12 wingunder

Hi @sewenew,

However, if I understood correctly, I think @quoctuanuit want to modify the timeout before the node is down, i.e. the node is still alive, but is going to be down.

Correct me if I misunderstand his question, since I'm not a native speaker :)

I read @quoctuanuit's explanation again and I see your point. One could read it the way you explained. So, in this case something that will bring a node down, will tell his application to reduce the timeout, so that the app can know almost immediately when things are not working any more.

I however think he basically wants a very short timeout so that he can know that something is wrong, as soon as a node is down or not responding any more. I don't know of a way to automatically detect if a node is on its way down (not down, but will be down soon). Maybe @quoctuanuit should try to explain this to us, if it is still relevant.

Anyway, I think that his real issue was to be able to set the socket_timeout to some value, before any redis call:

However, it looks like that we can not adjust the socket_timeout for each redis request for now.

This is indeed not possible at the moment. However, after reading the example that you posted a few moments ago, I really see no use-case for being able to adjust the socket_timeout before every redis call.

Regards

wingunder avatar Dec 29 '21 16:12 wingunder

Hi @wingunder

Thanks for your suggestion! I've updated the doc, and you can take a look: commit .

Regards

sewenew avatar Dec 30 '21 13:12 sewenew

Hi @sewenew,

Thanks for your suggestion! I've updated the doc, and you can take a look: commit .

You're welcome. Thanks for adding this to the documentation.

Regards

wingunder avatar Dec 30 '21 18:12 wingunder

Hi @sewenew @wingunder ! Sorry for the late response! You are right @sewenew that I want to modify the timeout before the node is down, then the error will be returned as fast as possible. In my situation, the redis client is running on a host machine, and the redis server is running in few pluggable devices. The term "node is down" means that the pluggable devices is unplugged. FYI, redis-server can be extended by redis-modules where we can add new redis commands other than the regular one (GET/SET...). The new redis command could take long time to finished (e.g it need to send the data to high latency hardware) (Here is more info about redis-modules if you want: https://redis.io/topics/modules-intro)

One more benefit of setting socket_timeout per request is that application can control what command should be blocked until server replied, or should we have some timeout for it.

"since it's single-threaded, and the long running command will block Redis."

Well, I believe that it's not single-threaded, e.g every blocking module command is handled by it's own thread in redis.

eriktuantran avatar Jan 08 '22 22:01 eriktuantran

One more benefit of setting socket_timeout per request is that application can control what command should be blocked until server replied, or should we have some timeout for it.

In this case, so far, I'd suggest that you create different Redis or RedisCluster objects for different tasks. So that you can set different socket_timeout for these Redis objects.

Regards

sewenew avatar Jan 09 '22 08:01 sewenew