better thread-safety for ConnectionPool #1270
Conversation
redis/__init__.py
Outdated
| 'AuthenticationError', 'BusyLoadingError', 'ConnectionError', 'DataError', | ||
| 'InvalidResponse', 'PubSubError', 'ReadOnlyError', 'RedisError', | ||
| 'ResponseError', 'TimeoutError', 'WatchError' | ||
| 'Redis', |
There was a problem hiding this comment.
If listing these on line by line, consider sorting them by name to make it easier to find when looking through the code.
redis/connection.py
Outdated
| @@ -1085,49 +1095,96 @@ def __eq__(self, other): | |||
|
|
|||
| def reset(self): | |||
| self.pid = os.getpid() | |||
There was a problem hiding this comment.
Apparently, this code still exhibits the race condition described in #1138 (comment) and addressed in PR #1262.
The PR does not change the behavior (w.r.t. issue #1138) for me. Moving this line to the end of the method does help.
There was a problem hiding this comment.
@gmbnomis Do you have a brief code sample that can repro what you're seeing?
There was a problem hiding this comment.
Unfortunately, there are at least three layers of modules between my failing tests and redis-py.
But, based on my assumption what goes wrong, I cobbled together gmbnomis@23a9c46. Notice how the assertion in gmbnomis@23a9c46#diff-81266569122b74f1e11f8a1cd3b12d24R115 suddenly fails, as it reuses old data from the parent process.
There was a problem hiding this comment.
I see what's going on now. You're right, setting the PID should happen as the very last call as the PID check isn't protected via a lock for performance reasons.
There was a problem hiding this comment.
@gmbnomis I pushed the change to this PR. Would you mind testing this branch in your setup and see if it fixes your issue? I'd like to get this out in the next release.
There was a problem hiding this comment.
@andymccurdy I gave it a spin and I can't reproduce the problem anymore. Thank you for fixing the issue!
There was a problem hiding this comment.
Excellent! Thanks for helping to figure out what was happening and the testing effort! I'm going to merge this and release redis-py 3.4.0 now.
andymccurdy
force-pushed
the
threadsafe-pools
branch
from
fcd714f
to
013c963
Compare
Codecov Report
@@ Coverage Diff @@
## master #1270 +/- ##
==========================================
- Coverage 92.67% 91.24% -1.43%
==========================================
Files 20 20
Lines 6426 6273 -153
==========================================
- Hits 5955 5724 -231
- Misses 471 549 +78
Continue to review full report at Codecov.
|
The fix redis/redis-py#1270 is part of redis-py 3.4.0. fixes #6038 https://pulp.plan.io/issues/6038
The fix redis/redis-py#1270 is part of redis-py 3.4.0. fixes #6038 https://pulp.plan.io/issues/6038
This PR implements better thread safety for
ConnectionPool.Fixes #1138
Fixes #1178
Fixes #906
Fixes #1262