sewenew / redis-plus-plus

Redis client written in C++
Apache License 2.0
1.64k stars 351 forks source link

Redis-plus-plus terminates/crashes when redis-server gets stopped [BUG] #360

Closed vishnu-marthala closed 2 years ago

vishnu-marthala commented 2 years ago

Describe the bug When the redis-server is stopped or crashed due to some reason. Redis-plus-plus also gets terminated /crashed. I was expecting a reconnection to the redis-server. When the redis-server is back to live then my application doesn't reconnect.

To Reproduce

  1. Connect to Redis-server

    
    PLOGD << "Connecting to redis on " << host << ":" << port << " ...";
    
    try {
        redis::ConnectionOptions connection_options;
        connection_options.host = host;
        connection_options.port = port;
    
        redis::ConnectionPoolOptions pool_options;
        pool_options.size = 5;
    
        m_Redis = QSharedPointer<redis::Redis>(new redis::Redis(connection_options, pool_options));
    
        if (m_Redis->ping() != "PONG") {
            throw new redis::Error("Pong failed!");
        }
    } catch (exception& e) {
        PLOGE << "Connecting to redis failed!";
        exit(1);
    }
2. Stop the redis-server manually . Now you will see the following error msg

terminate called after throwing an instance of 'sw::redis::ClosedError' what(): Failed to get reply: Server closed the connection

3. debugger points to the line -> auto reply.

ReplyUPtr Redis::_command(Connection &connection, Cmd cmd, Args &&...args) { assert(!connection.broken());

cmd(connection, std::forward<Args>(args)...);

auto reply = connection.recv();

return reply;

}



**Expected behavior**
Redis-plus-plus should not terminate and rather reconnect automatically.

**Environment:**
 - OS: [ubuntu]
 - Compiler: [ gcc 10]
 - hiredis version: [ master]
 - redis-plus-plus version: [master]

**Additional context**
Add any other context about the problem here.
sewenew commented 2 years ago

It seems that you did not try-catch possible exception when the server is down. Try the following code:

try {
    ConnectionOptions opts;
    opts.host = "127.0.0.1";
    opts.port = 6379;
    ConnectionPoolOptions pool_options;
    pool_options.size = 5;
    auto r = Redis(opts, pool_options);
    while (true) {
        try {        // <------ you need to catch possible exceptions.
            cerr << r.ping() << endl;       // <----- if the server recovers, you'll get a PONG
        } catch (const exception &e) {
            cerr << e.what() << endl;         // <---- when the server is down, it throws, you can catch it, and try again
        }                                                      // <---- redis-plus-plus will reconnect the next time you retry.
        this_thread::sleep_for(chrono::seconds(1));
    }
} catch (const Error &e) {
    cerr << "failed to run app: " << e.what() << endl;
}

Regards

sewenew commented 2 years ago

Since there's no update, I'll close the issue.

Regards

vishnu-marthala commented 2 years ago

@sewenew sorry for the late response. due to weekend and other high priority task, I couldn't respond. Thanks for you eg code.

while (true) {
        try {        // <------ you need to catch possible exceptions.
            cerr << r.ping() << endl;       // <----- if the server recovers, you'll get a PONG
        } catch (const exception &e) {
            cerr << e.what() << endl;         // <---- when the server is down, it throws, you can catch it, and try again
        }                                                      // <---- redis-plus-plus will reconnect the next time you retry.
        this_thread::sleep_for(chrono::seconds(1));
    }

The problem with above code is that this infinet while loop which always pings the redis makes my program 2 issues. It seems not efficient. Another issues is It is blocking my Main Thread, I can push it to another thread but it just makes my communication with redis from different thread more complex. Is there any callback or event from the redis-plus-plus that i can listen to? eg: OnConnectionchanged () -> I try to reconnect. FYI: I am using Qt .

vishnu-marthala commented 2 years ago

I took you suggestion and try to catch it inside my function that runs every sec.


    try {
        redis::ConnectionOptions connection_options;
        connection_options.host = host;
        connection_options.port = port;

        redis::ConnectionPoolOptions pool_options;
        pool_options.size = 5;

        m_Redis = QSharedPointer<redis::Redis>(new redis::Redis(connection_options, pool_options));

        QTimer *timer = new QTimer(this);
        timer->setInterval(1000);
        connect(timer, &QTimer::timeout, this, [this](){
                try {
                    PLOGV << m_Redis->ping() << endl;
                } catch (const exception& e) {
                    PLOGE << "Ping to redis failed! due to " <<e.what()
                          << endl;
                }

        });
        timer->start();

    } catch (const exception& e) {
        PLOGE << "Connecting to redis failed!";
    }

When i restarted the redis server, i still get the following error and my program terminates..

terminate called after throwing an instance of 'sw::redis::IoError' what(): Failed to get reply: Connection reset by peer

What am i doing wrong?

sewenew commented 2 years ago

The problem with above code is that this infinet while loop which always pings the redis makes my program 2 issues

That code is an example on testing the auto reconnection. You should not do it in production code.

Is there any callback or event from the redis-plus-plus that i can listen to? eg: OnConnectionchanged () -> I try to reconnect

No, there's no such callback. Since redis-plus-plus will try to reconnect automatically, you don't need to do that.

Instead, you can retry the operation (maybe, after a while), when you get an exception, e.g. the server is down. After a pre-defined retry times, you can stop the retry, since the server might be down for a long time. You should set your retry policy based on your application.

Regards

sewenew commented 2 years ago

I took you suggestion and try to catch it inside my function that runs every sec.

I think you misunderstood my comment. You do not need to have a periodic task to ping it. Instead, catch the exception, and retry the operation if needed.

When i restarted the redis server, i still get the following error and my program terminates..

I cannot reproduce your problem. You should set try-catch block for all operations related to redis-plus-plus, not only for your periodic routine.

If you still have the problem, please give minimum compilable code (without QT), which can reproduce your problem.

Regards