Fix reply timeout

[jappja]

No description provided.

[IlyaSkriblovsky]

Hi! You are right that current timeout handling is broken. It is effectively disabled since we never call setTimeout.

But I think your fix needs a bit more to be ideal:

• Please add setTimeout(None) to connectionLost. Your test_connection_lost doesn't fails with Reactor was unclean only because you monkey-patch callLater. In real life setTimeout will leak delayed call if it is not cancelled in connectionLost
• resetTimeout doesn't cancels watchdog timer, it only reschedules it to timeOut seconds from now. So if we issue a command, receive a reply and then connection will be idle for timeOut seconds, watchdog timer will fire and will close the connection. I don't think this is what we want. Timeout should be disabled if we received responses to all commands that we issued. This can be done by adding something like this to the end of replyReceived:

if not self.replyQueue.waiting:
    self.setTimeout(None)

• calling setTimeout multiple times with same argument is effectively the same than calling resetTimeout: it just calls self.__timeoutCall.reset(period). So, each command we send thru connection will reschedule watchdog timer and if we send commands regularly, timeout will never fire even we are not receiving any replies from Redis.

Actually it seems that TimeoutMixin works well for server protocols when we want to close connection if it is idle for N seconds. But we need different behavior: we need to close connection if we doesn't receive reply within N seconds from sending corresponding request. May be it's better to remove TimeoutMixin and replace it with timeout handling at execute_command?..

Please correct me if I'm wrong

[jappja]

Yes you are right, TimeoutMixin is intended for server side protocol handling. And this whole timeout handling is much more complicated than it first looked. I'm afraid I don't have time nor deep enough knowledge of the code (or Twisted itself) at this time to have proper fix for it. I'll close this PR and create an issue instead.

[jappja]

Created #101

[IlyaSkriblovsky]

Thanks!