PYTHON-4782 Fix deadlock and blocking behavior in _ACondition.wait #1875
Conversation
|
Hmm, these changes are causing a few test regressions. I believe it's due to the new behavior of server selection because of the topology lock.wait(). Previously, server selection would block the loop entirely for 500ms on each iteration where a server could not be selected. Now, we correctly wait without blocking the loop. |
|
Requesting review now, I'll keep investigating the test failures. |
| await asyncio.wait_for(fut, timeout) | ||
| return True | ||
| except asyncio.TimeoutError: | ||
| return False # Return false on timeout for sync pool compat. |
There was a problem hiding this comment.
Do we still need to acquire the lock if we timeout here?
There was a problem hiding this comment.
Yes, the API contract for wait() says you MUST hold the lock before calling and you MUST still hold the lock when it returns, even on timeout.
There was a problem hiding this comment.
Got it, so you always have to acquire the lock if you call wait and don't raise an error?
There was a problem hiding this comment.
Can you add a comment linking to the API contract here? It would be good to make it more understandable for readers unfamiliar with it.
There was a problem hiding this comment.
There's already this comment:
# Must re-acquire lock even if wait is cancelled.
| loop = asyncio.get_running_loop() | ||
| fut = loop.create_future() | ||
| self._waiters.append((loop, fut)) | ||
| self.release() |
There was a problem hiding this comment.
Is this just to ensure we don't hold the lock while waiting for it?
There was a problem hiding this comment.
Yes without releasing the lock, this code would deadlock since nothing would be able to notify the waiter.
| err = None | ||
| while True: | ||
| try: | ||
| await self.acquire() |
There was a problem hiding this comment.
Can this possibly loop forever?
There was a problem hiding this comment.
Yes but only if something else holds the lock forever.
There was a problem hiding this comment.
And we're asserting that this new code can't do that, got it!
|
The failures seem relevant: |
|
It is the same failure across several tasks that all previously passed. |
|
Yes I'm looking into test_to_list_csot_applied today. |
|
I'm running a patch with both #1870 and this PR to see if the failures will work themselves out: https://spruce.mongodb.com/version/66f712658ea18700077002df/tasks Edit: this one also has some failures. I pushed a fix for test_reconnect. Although I am a little concerned by these failures. One explanation is that an AsyncMongoClient takes much longer to process SDAM updates (eg on init or when rediscovering a server after an error). |
|
Bad timing for docker to start failing... |
|
New patch with both #1870 and this PR: https://spruce.mongodb.com/version/66f73f178ea187000770b298/ |
|
This is ready for another look. |
|
The Windows failures seem relevant, since they're in |
|
I fixed the remaining test failures. |
PYTHON-4782 Fix deadlock and blocking behavior in _ACondition.wait
TODO: