PYTHON-4860 - Async client should use asyncio.Lock and asyncio.Condition #1934
Conversation
NoahStapp
changed the title
NoahStapp
force-pushed
the
PYTHON-4860
branch
from
02cc8e8
to
81ea0bc
Compare
|
Relies on #1928. |
test/asynchronous/test_locks.py
Outdated
|
|
||
| if __name__ == "__main__": | ||
| unittest.main() | ||
| # # Copyright 2024-present MongoDB, Inc. |
There was a problem hiding this comment.
Now that we're using the stdlib Lock + Condition, we should be able to remove these tests as they're copied from the asyncio test suite: https://github.com/python/cpython/blob/v3.13.0rc2/Lib/test/test_asyncio/test_locks.py.
There was a problem hiding this comment.
However, the two issues fixed in 3.13 are still present on older versions of Python: (python/cpython#111693 and python/cpython#112202)
There was a problem hiding this comment.
I think we'll need to polyfill those fixes then
There was a problem hiding this comment.
Yeah, vendoring asyncio.locks seems like the best option here until we drop 3.12 support 😅
|
The test failures appear to be linux-specific, investigating. |
pymongo/lock.py
Outdated
| @@ -11,15 +11,17 @@ | |||
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | |||
| # See the License for the specific language governing permissions and | |||
| # limitations under the License. | |||
| """Lock and Condition classes vendored from https://github.com/python/cpython/blob/main/Lib/asyncio/locks.py | |||
| to port 3.13 fixes to older versions of Python.""" | |||
There was a problem hiding this comment.
We need to add attribution in THIRD_PARTY_NOTICES
There was a problem hiding this comment.
And we should make it a separate file with a note that it can one day be removed when we drop 3.12 support.
There was a problem hiding this comment.
Which parts of the CPython license do we need? Just the PYTHON SOFTWARE FOUNDATION LICENSE VERSION 2 section?
There was a problem hiding this comment.
Here's the guidance I found: https://discuss.python.org/t/how-do-i-properly-adhere-to-licenses-when-copying-code-from-cpython/5288/5
test/asynchronous/test_locks.py
Outdated
| @@ -1,513 +0,0 @@ | |||
| # Copyright 2024-present MongoDB, Inc. | |||
There was a problem hiding this comment.
If we're vendoring the 3.13 locks, we'll need to keep this test file.
There was a problem hiding this comment.
Couldn't we only run these tests on older versions of Python?
There was a problem hiding this comment.
Sure. By the way did you investigate the bugs to confirm that we actually need to vendor the locks? I'm wondering if we can just use the built in classes and accept any potential built in bugs.
There was a problem hiding this comment.
After looking, python/cpython#111693 seems fine.
python/cpython#112202 would could result in deadlocks or timeouts so it seems serious enough to warrant vendoring.
There was a problem hiding this comment.
python/cpython#111693 doesn't seem like an issue agreed.
| @@ -1019,15 +1018,13 @@ def __init__( | |||
| # The first portion of the wait queue. | |||
| # Enforces: maxPoolSize | |||
| # Also used for: clearing the wait queue | |||
| self.size_cond = _ACondition(threading.Condition(_lock)) | |||
There was a problem hiding this comment.
Let's move these back since the Condition vars are defined alongside the variables they protect as well as the explanatory comments.
pymongo/lock.py
Outdated
| self.release() # type: ignore[attr-defined] | ||
|
|
||
|
|
||
| class Lock(_ContextManagerMixin, _LoopBoundMixin): |
There was a problem hiding this comment.
Now that we're directly vendoring, we should split the vendored code into its own file. then we can do something like this:
if sys.version_info >= (3, 13):
Lock = asyncio.Lock
Condition = asyncio.Condition
else
Lock = pymongo._asyncio_lock.Lock
Condition = pymongo._asyncio_lock.ConditionThis way we're not mixing vendored code with our own.
pymongo/lock.py
Outdated
| import weakref | ||
| from typing import Any, Callable, Optional, TypeVar | ||
| from asyncio import TimeoutError, wait_for |
There was a problem hiding this comment.
Suggest using asyncio.TimeoutError to avoid shadowing the builtin TimeoutError.
pymongo/lock.py
Outdated
| @@ -28,6 +33,14 @@ | |||
|
|
|||
| _T = TypeVar("_T") | |||
|
|
|||
| # Needed to support 3.13 asyncio fixes in older versions of Python | |||
There was a problem hiding this comment.
Can you add https://github.com/python/cpython/issues/112202 here? That's the most important bugfix we need.
| lock: threading.Lock, condition_class: Optional[Any] = None | ||
| ) -> threading.Condition: | ||
| """Represents a threading.Condition.""" | ||
| if condition_class: |
There was a problem hiding this comment.
Sigh, I can't believe we still have to write code for condition_class even though it's never used. I reopened https://jira.mongodb.org/browse/PYTHON-1149.
There was a problem hiding this comment.
Yeah I wasn't sure if we actually still used it, removing it entirely would be great.
No description provided.