Lock is not preventing concurrent acquiring

[rbedia]

asyncio.Lock() is not preventing concurrent acquiring of the lock.

Circuitpython version: Adafruit CircuitPython 7.3.3 on 2022-08-29; Adafruit Feather Bluefruit Sense with nRF52840

main.py:

import asyncio

async def lock_test(name, lock, sleep_time=0):
    print(name, 'start')
    async with lock:
        print(name, 'acquired lock')
        if sleep_time:
            await asyncio.sleep(sleep_time)
    print(name, 'released lock')
    return name

async def main(yield_mode):
    lock = asyncio.Lock()

    lock.yield_mode = yield_mode
    print('Lock test yield mode:', yield_mode)

    results = await asyncio.gather(
        asyncio.create_task(lock_test('A', lock, sleep_time=0.1)),
        asyncio.create_task(lock_test('B', lock, sleep_time=0.1)),
    )
    print('Task results:', results)


yield_mode = True
asyncio.run(main(yield_mode))

yield_mode = False
asyncio.run(main(yield_mode))

Output:

main.py output:
Lock test yield mode: True
A start
A acquired lock
B start
A released lock
B acquired lock
B released lock
Task results: ['A', 'B']
Lock test yield mode: False
A start
A acquired lock
B start
B acquired lock
A released lock
Traceback (most recent call last):
  File "asyncio/core.py", line 214, in run_until_complete
  File "main.py", line 8, in lock_test
  File "/lib/asyncio/lock.py", line 91, in __aexit__
  File "/lib/asyncio/lock.py", line 51, in release
RuntimeError: Lock not acquired

You are in safe mode because:
CircuitPython core code crashed hard. Whoops!
Crash into the HardFault_Handler.

When await core.sleep(0) is used in acquire(), task B is able to acquire the lock even though task A has not yet released the lock. This then leads to the exception when B tries to release the lock that was already released by A.

As a test I modified asyncio/lock.py to show that going back to using yield works while using await core.sleep(0) does not.

# SPDX-FileCopyrightText: 2019-2020 Damien P. George
#
# SPDX-License-Identifier: MIT
#
# MicroPython uasyncio module
# MIT license; Copyright (c) 2019-2020 Damien P. George
#
# This code comes from MicroPython, and has not been run through black or pylint there.
# Altering these files significantly would make merging difficult, so we will not use
# pylint or black.
# pylint: skip-file
# fmt: off
"""
Locks
=====
"""

from . import core

# Lock class for primitive mutex capability
class Lock:
    """Create a new lock which can be used to coordinate tasks. Locks start in
    the unlocked state.

    In addition to the methods below, locks can be used in an ``async with``
    statement.
    """

    def __init__(self):
        # The state can take the following values:
        # - 0: unlocked
        # - 1: locked
        # - <Task>: unlocked but this task has been scheduled to acquire the lock next
        self.state = 0
        # Queue of Tasks waiting to acquire this Lock
        self.waiting = core.TaskQueue()
        self.yield_mode = True

    def locked(self):
        """Returns ``True`` if the lock is locked, otherwise ``False``."""

        return self.state == 1

    def release(self):
        """Release the lock. If any tasks are waiting on the lock then the next
        one in the queue is scheduled to run and the lock remains locked. Otherwise,
        no tasks are waiting and the lock becomes unlocked.
        """

        if self.state != 1:
            raise RuntimeError("Lock not acquired")
        if self.waiting.peek():
            # Task(s) waiting on lock, schedule next Task
            self.state = self.waiting.pop_head()
            core._task_queue.push_head(self.state)
        else:
            # No Task waiting so unlock
            self.state = 0

    async def acquire(self):
        """Wait for the lock to be in the unlocked state and then lock it in an
        atomic way. Only one task can acquire the lock at any one time.

        This is a coroutine.
        """

        if self.state != 0:
            # Lock unavailable, put the calling Task on the waiting queue
            self.waiting.push_head(core.cur_task)
            # Set calling task's data to the lock's queue so it can be removed if needed
            core.cur_task.data = self.waiting
            try:
                if self.yield_mode:
                    yield
                else:
                    await core.sleep(0)
            except core.CancelledError as er:
                if self.state == core.cur_task:
                    # Cancelled while pending on resume, schedule next waiting Task
                    self.state = 1
                    self.release()
                raise er
        # Lock available, set it as locked
        self.state = 1
        return True

    async def __aenter__(self):
        return await self.acquire()

    async def __aexit__(self, exc_type, exc, tb):
        return self.release()

[jepler]

@tekktrik can you take a look at this? The change appears to have been introduced by a PR about "add docstrings" which is odd. https://github.com/adafruit/Adafruit_CircuitPython_asyncio/pull/19/files#diff-6c3d301ce3f21e806072e5b6da07862be0327e8fb5245117013252c725224348R72

[jepler]

Based on a quick read, "yield" in the context of cp/mp asyncio seems to mean "don't schedule me ever, something else will manually schedule me" while "await asyncio.sleep(0)" means "reschedule me as soon as possible". However, I think that's not CPython compatible; yield in an async def is a way of writing an async generator, since python 3.6: https://stackoverflow.com/questions/37549846/how-to-use-yield-inside-async-function

[tekktrik]

I'll take a look. The change was made to help get the linter pass, or something like that.

[tekktrik]

If it needs to be changed back, I didn't have any reason of my own to change them as I did.

[dhalbert]

I had to change several yield inside the asyncio library to asyncio.sleep(0) when I was proting it

[jepler]

I think they have different semantics in Circuit/MicroPython.

[jepler]

Closed by #30