python / cpython

The Python programming language
https://www.python.org
Other
62.35k stars 29.94k forks source link

"loop argument must agree with lock" instantiating asyncio.Condition #89579

Closed 872deb43-dfce-4d53-844f-321c7ac92f28 closed 2 years ago

872deb43-dfce-4d53-844f-321c7ac92f28 commented 2 years ago
BPO 45416
Nosy @asvetlov, @ambv, @serhiy-storchaka, @1st1, @achimnol, @simonw, @miss-islington, @uriyyo
PRs
  • python/cpython#28850
  • python/cpython#28856
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields: ```python assignee = None closed_at = created_at = labels = ['3.11', '3.10', 'expert-asyncio'] title = '"loop argument must agree with lock" instantiating asyncio.Condition' updated_at = user = 'https://github.com/simonw' ``` bugs.python.org fields: ```python activity = actor = 'asvetlov' assignee = 'none' closed = True closed_date = closer = 'asvetlov' components = ['asyncio'] creation = creator = 'simonw' dependencies = [] files = [] hgrepos = [] issue_num = 45416 keywords = ['patch'] message_count = 7.0 messages = ['403500', '403501', '403502', '403506', '403599', '403601', '403719'] nosy_count = 8.0 nosy_names = ['asvetlov', 'lukasz.langa', 'serhiy.storchaka', 'yselivanov', 'Joongi Kim', 'simonw', 'miss-islington', 'uriyyo'] pr_nums = ['28850', '28856'] priority = 'normal' resolution = 'fixed' stage = 'resolved' status = 'closed' superseder = None type = None url = 'https://bugs.python.org/issue45416' versions = ['Python 3.10', 'Python 3.11'] ```

    872deb43-dfce-4d53-844f-321c7ac92f28 commented 2 years ago

    In Python 3.10 it is not possible to instantiate an asyncio.Condition that wraps an asyncio.Lock without raising a "loop argument must agree with lock" exception.

    This code raises that exception:

        asyncio.Condition(asyncio.Lock())

    This worked in previous Python versions.

    Note that the error only occurs if an event loop is running. Here's a simple script that replicates the problem:

        import asyncio
    
        # This runs without an exception:
        print(asyncio.Condition(asyncio.Lock()))
    
        # This does not work:
        async def example():
            print(asyncio.Condition(asyncio.Lock()))
    
        # This raises "ValueError: loop argument must agree with lock":
        asyncio.run(example())
    872deb43-dfce-4d53-844f-321c7ac92f28 commented 2 years ago

    I ran across this issue while trying to use the https://pypi.org/project/janus/ locking library with Python 3.10 - see my issue on their tracker here: https://github.com/aio-libs/janus/issues/358

    872deb43-dfce-4d53-844f-321c7ac92f28 commented 2 years ago

    It looks like the relevant test is here: https://github.com/python/cpython/blob/a1092f62492a3fcd6195bea94eccf8d5a300acb1/Lib/test/test_asyncio/test_locks.py#L722-L727

        def test_explicit_lock(self):
            lock = asyncio.Lock()
            cond = asyncio.Condition(lock)
    
            self.assertIs(cond._lock, lock)
            self.assertIs(cond._loop, lock._loop)

    But... that test doesn't appear to run inside an event loop, so it's not covering the behaviour described in this issue.

    ambv commented 2 years ago

    Issue confirmed. The problem is that Condition.__init__ compares its own loop with Lock's internal _loop attribute. That attribute is set to None unless the lock waited to be acquired.

    uriyyo, Andrew, Yury, since it's pretty likely that Lock objects will now have _loop set to None, I think the check for "loop agreement" in Condition is kind of useless. So I propose to simply remove it. It's the only such check in all of asyncio.

    If you insist on keeping the loop check, it should special-case _loop is None.

    Simon, thanks for your detailed report! As a backwards-compatible workaround till we get a fix in, your user code can do the following:

    >>> l = asyncio.Lock()
    >>> getattr(l, '_get_loop', lambda: None)()
    <_UnixSelectorEventLoop running=True closed=False debug=False>

    You can use such lock without issues now:

    >>> asyncio.Condition(l)
    <asyncio.locks.Condition object at 0x10c05bee0 [unlocked]>

    Alternatively, if the above disgusts you and you only want to trigger public APIs, you can do this dance:

    >>> l = asyncio.Lock()
    >>> await l.acquire()  # first acquire will just work
    True
    >>> try:
    ...   # second acquire will block so we time it out
    ...   await asyncio.wait_for(l.acquire(), 0.1)
    ... except asyncio.TimeoutError:
    ...   pass
    ...
    >>> l.release()

    Now the lock is fully initialized and we can use it:

    >> c = asyncio.Condition(l)

    Both workarounds should be compatible with Python 3.7+ asyncio.

    serhiy-storchaka commented 2 years ago

    New changeset 1a7892414e654aa5c99efa31db767baba7f4a424 by Joongi Kim in branch 'main': bpo-45416: Fix use of asyncio.Condition() with explicit Lock objects (GH-28850) https://github.com/python/cpython/commit/1a7892414e654aa5c99efa31db767baba7f4a424

    miss-islington commented 2 years ago

    New changeset 164dddf5f8c9c6b93f32c9f79b4301fc804576e9 by Miss Islington (bot) in branch '3.10': bpo-45416: Fix use of asyncio.Condition() with explicit Lock objects (GH-28850) https://github.com/python/cpython/commit/164dddf5f8c9c6b93f32c9f79b4301fc804576e9

    asvetlov commented 2 years ago

    Thanks, guys!