Disconnect clients from pool safely #753
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This PR tries to solve the issues raised by redis#732 regarding the danger of disconnect clients from the `ConnectionPool.disconnect` method executed by a Thread different that those ones that are in charge of the connections. Instead of call the `Connection.disconnect` method it uses the syscall `shutdown` to leave the socket unusable. Once the connection tries to use the socket, even when it is already blocked such us the `PubSub` pattern, it gets a `socket.error` exception that will be cactched by the `Connection` class to then raise an `ConnectionError` and disconnect the socket in a clean and safe way. The `Client.execute_command` function catches the `ConnectionError` exception and tries to connect again and run the command that raised the error. Worth mentioning that in the case of the `Sentinel` environment, if some changes regarding the Redis pool of servers - perhaps the mater went down and a slave was promoted - the next command will be executed using an other server.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR tries to solve the issues raised by #732 regarding
the danger of disconnect clients from the
ConnectionPool.disconnectmethod executed by a Thread different that those ones that are in charge
of the connections.
Instead of call the
Connection.disconnectmethod it uses the syscallshutdownto leave the socket unusable. Once the connection tries to usethe socket, even when it is already blocked such us the
PubSubpattern, itgets a
socket.errorexception that will be cactched by theConnectionclass to then raise anConnectionErrorand disconnect thesocket in a clean and safe way.
The
Client.execute_commandfunction catches theConnectionErrorexceptionand tries to connect again and run the command that raised the error.
Worth mentioning that in the case of the
Sentinelenvironment, if the disconnect was because of achange of the Redis pool servers - perhaps the master went
down and a slave was promoted, the next command will be executed using a new connection that will take into account these changes.