reconnect doesn't work properly

[bigunyak]

Hi Pavel!

First of all thank you for your module, in my opinion it's the best Redis client for Perl. But there is a problem with reconnection to server I've faced recently. The thing is that in the _connect subroutine you have such code:

    # this is to prevent recursion
    confess "Couldn't connect to the redis-server."
      . " Connection was immediately closed by the server."
      if $self->{_in_connect}++;

It works ok if there are no errors during the connection process. But if we get an error here for example

 croak "Can't connect to redis server $self->{host}:$self->{port}: $!";

we remain $self->{_in_connect} set and next time wouldn't try to reconnect and die immediately. Real reconnection logic should try to reconnect every time I try to communicate with server. For example, when Redis server goes down for 5 minutes and gets back after that I expect to get working connection and continue normal work. What do you think about that?

[trinitum]

The problem with exceptions is that sometimes the object left in inconsistent state after throwing an exception. For example, imagine _connect successfully established a connection and then died at send_command('AUTH'..., if the object would try to just connect again next time you call send_command method, it wouldn't work, because the parser still expects reply to the previously sent 'AUTH'. I thought about this problem before and the only solution I could find is to add reset_connection method which should be called before using the object again after an exception. This is documented in the 'ERROR HANDLING' section. I think I can fix this particular case by incrementing {_in_connect} before authentication, instead of doing it in if, but generally, if you caught an exception, you should call reset_connection before using the object again. I think I should improve documentation on this topic.

Thanks for raising this question. If you have ideas how to improve error handling I would be glad to hear.

[bigunyak]

Thanks for your response. Now I see that reset_connection method deletes all underscore params in $self. So, your current implementation obliges users to call reset_connection after any exception. Maybe it's better to do it yourself or you think there is some points to get the object in inconsistent state? At least you can consider to add a special parameter auto_reset (for example) in the constructor to save users from calling reset_connection when they don't expect the object gets in inconsistent state. Yes, if you move the incrementing of {_in_connect} after the socket creation it will solve the problem with reconnection. I will be able to reconnect to redis server without calling reset_connection.

[trinitum]

I would definitely call reset_connection myself if I'd see the way to do it nicely. Currently I see following ways:

• Wrap bodies of all public methods into eval blocks. I don't want to do it, it will have some performance overhead, and will break error reporting.
• Reset object before throwing exception. This will complicate code, is error prune, and won't work if exception is a result of a bug in my code.

If you see a better way to implement it let me know and I will consider it. But I don't see requirement to reset object after exception as such a big problem, after all if read from socket returned error, you're not expecting this socket to still be connected.

There's of course a particular problem with reconnecting. Currently if module detected that server closed connection, it tries to reconnect, but just once. Perhaps it could try harder and make several attempts. Another problem is that it only tries to reconnect if no data were lost, so if you restart redis-server while module sending data you will likely get an exception. Maybe if you describe circumstances when you're getting the error, I could suggest a better solution.