Connection error on persistent connection may lead to "poisoned" connection #1629
Description
Describe the bug
We experienced an issue where it appears garbage data ended up in the connection - it's not clear how or why this happened, and still debugging it (we saw a Unknown response prefix: 'O'. [tls://...). In the meantime though, we had a related persistent issue after this occurred.
From this point onwards, we saw issues with all Redis operations in this PHP worker. Opening this issue as I think I've identified a potential path for this issue persisting.
If a CommunicationException occurs while reading/writing data to a socket, this will be thrown and never caught by Predis itself. If this isn't handled specially by the caller, this will lead to an uncaught exception and eventually StreamConnection->__destruct() will be called which should clean this up.
If the connection is created in persistent mode, the destructor never calls StreamConnection->disconnect() - this is intentional, to leave the socket open for the next user, as the common case for the destructor is a normal request completing.
However, in the specific case of a CommunicationException which indicates a problem with the wire communication itself, this could lead to a broken connection persisting and continuing to be used.
To Reproduce
(tbd full steps)
Expected behavior
Predis could handle CommunicationException specifically and force a disconnection for persistent connections, to ensure that a bad connection is never incorrectly persisted.
Alternatively, if this error needs to be handled by the caller, it should be explicitly documented that communication errors on persistent connections should call ->disconnect()
Versions (please complete the following information):
- Predis: [e.g. 1.1.2] 3.3.0
- PHP [e.g. 8.0.0] 8.2.0
- Redis Server [e.g. 6.0.0] 7.0.7
- OS [e.g. Ubuntu 20.10] n/a