Make sentinel thread-safe during master failover #909
Closed
Conversation
This file contains 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
lalinsky
commented
Sep 30, 2017
| @@ -59,9 +59,6 @@ def read_response(self): | |||
| # When talking to a master, a ReadOnlyError when likely | |||
| # indicates that the previous master that we're still connected | |||
| # to has been demoted to a slave and there's a new master. | |||
| # calling disconnect will force the connection to re-query | |||
| # sentinel during the next connect() attempt. | |||
| self.disconnect() | |||
There was a problem hiding this comment.
The connection will get closed when it's returned to the pool.
| self.master_address = master_address | ||
| elif master_address != self.master_address: | ||
| # Master address changed, disconnect all clients in this pool | ||
| self.disconnect() |
There was a problem hiding this comment.
All old connections will get closed when attempting to fetch them from the pool. I could theoretically close connections from _available_connections here, but I don't know what would the thread-safety implications be. The fact that this code depends on atomic operations in Python is confusing enough.
redis/sentinel.py
Outdated
| @@ -125,16 +120,36 @@ def rotate_slaves(self): | |||
| pass | |||
| raise SlaveNotFoundError('No slave found for %r' % (self.service_name)) | |||
|
|
|||
| def _checkpid(self): | |||
There was a problem hiding this comment.
The implementation in ConnectionPool is thread-safe and the __init__() call is not needed, reset() does everything necessary.
lalinsky
added a commit
to exponea/flask-redis-sentinel
that referenced
this pull request
Sep 30, 2017
This was referenced Sep 30, 2017
lalinsky
commented
Sep 30, 2017
| master_address = self.sentinel_manager.discover_master( | ||
| self.service_name) | ||
| if self.is_master: | ||
| if self.master_address is None: | ||
| if master_address != self.master_address: | ||
| self.master_address = master_address |
In the current version, the sentinel code tries to close all connections immediately after discovering there is a new master. This is a problem in multi-threaded environment, because neither `ConnectionPool.disconnect` nor `Connection.disconnect` are thread-safe. If you call `SentinelConnectionPool.disconnect` after master failover, that will close all connections that are potentially used from other threads, causing all kinds of errors. This change avoids that behavior by adding acquire/release checks, so connections that don't belong the current master are never returned to the pool and they are closed instead.
lalinsky
force-pushed
the
thread-safe-sentinel
branch
from
38a0157 to
bbf553c
Compare
|
This pull request is marked stale. It will be closed in 30 days if it is not updated. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
In the current version, the sentinel code tries to close all
connections immediately after discovering there is a new master.
This is a problem in multi-threaded environment, because
neither
ConnectionPool.disconnectnorConnection.disconnectarethread-safe. If you call
SentinelConnectionPool.disconnectafter masterfailover, that will close all connections that are potentially used
from other threads, causing all kinds of errors.
This change avoids that behavior by adding acquire/release checks, so
connections that don't belong the current master are never returned
to the pool and they are closed instead.