Fix permanent loss of semaphore count when using gevent Timeouts

[TylerWilley]

Gevent patches Lock objects such that when acquire is called on a lock, the context can switch to other coroutines.
Gevent also allows you to set a Timeout on executing code.

If you set a Timeout, and the Timeout is raised during this lock's acquire() section, then pymongo will have acquired _socket_semaphore, but not released it. Thus permanently losing a count on the semaphore.

[ShaneHarvey]

Did you determine that this was not an issue in pymongo?

[TylerWilley]

So I think I figured out what's going on. Gevent patches .acquire() so that it can switch out to other coroutines. During threading.Condition:wait(), 2 .acquire calls are made, one for the waiter, and one to re-acquire Condition._lock before modifying _waiters (see threading.py:263) If a timeout occurs during waiter.acquire(), Condition.wait's finally clause will catch it, re-acquire ._lock, and then raise, raising through pymongo, which should release the lock. If a timeout occurs during _lock.acquire(), this is due to someone else having the Condition._lock, and Condition will raise without removing the waiter lock from _waiters. In this case, if pymongo caught the exception and released the lock it would be releasing a lock someone else owned. So I believe I've found a fix that is 2 fold. 1. Modify pymongo to use `with self._cond:`, I'll update and resubmit the PR. To anyone not using gevent, this will be transparent. To anyone using gevent, this will be an improvement if they use timeouts... It will prevent the deadlock, but introduce the possibility of concurrent modification of _waiters 2. Create a new version of Condition that loops on _lock.acquire(), catching any Timeout exceptions to guarantee that the lock is acquired, remove the waiter from _waiters, then re-raise the Timeout exception that was caught. With both of these, it may delay the gevent Timeout being raised (nothing new there with coroutines though), but will prevent any concurrent modification of _waiters, and also prevent any unreleased locks from occurring.

…

-Tyler

On Fri, Jul 24, 2020 at 3:45 PM Shane Harvey ***@***.***> wrote: Did you determine that this was not an issue in pymongo? — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#471 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AQMQPDM5ZOIOTGRCAWXA473R5H6HTANCNFSM4PHAF5WQ> .

[TylerWilley]

I suppose I should add, when gevent patches .acquire, it becomes possible for it to raise gevent.timeout.Timeout.

…

On Fri, Jul 24, 2020 at 5:47 PM Tyler W ***@***.***> wrote: So I think I figured out what's going on. Gevent patches .acquire() so that it can switch out to other coroutines. During threading.Condition:wait(), 2 .acquire calls are made, one for the waiter, and one to re-acquire Condition._lock before modifying _waiters (see threading.py:263) If a timeout occurs during waiter.acquire(), Condition.wait's finally clause will catch it, re-acquire ._lock, and then raise, raising through pymongo, which should release the lock. If a timeout occurs during _lock.acquire(), this is due to someone else having the Condition._lock, and Condition will raise without removing the waiter lock from _waiters. In this case, if pymongo caught the exception and released the lock it would be releasing a lock someone else owned. So I believe I've found a fix that is 2 fold. 1. Modify pymongo to use `with self._cond:`, I'll update and resubmit the PR. To anyone not using gevent, this will be transparent. To anyone using gevent, this will be an improvement if they use timeouts... It will prevent the deadlock, but introduce the possibility of concurrent modification of _waiters 2. Create a new version of Condition that loops on _lock.acquire(), catching any Timeout exceptions to guarantee that the lock is acquired, remove the waiter from _waiters, then re-raise the Timeout exception that was caught. With both of these, it may delay the gevent Timeout being raised (nothing new there with coroutines though), but will prevent any concurrent modification of _waiters, and also prevent any unreleased locks from occurring. -Tyler On Fri, Jul 24, 2020 at 3:45 PM Shane Harvey ***@***.***> wrote: > Did you determine that this was not an issue in pymongo? > > — > You are receiving this because you modified the open/close state. > Reply to this email directly, view it on GitHub > <#471 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AQMQPDM5ZOIOTGRCAWXA473R5H6HTANCNFSM4PHAF5WQ> > . >