Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added an alternate fix for MemoryObjectReceiveStream.receive()` on asyncio #595

Merged
merged 5 commits into from
Jul 25, 2023

Conversation

agronholm
Copy link
Owner

@agronholm agronholm commented Jul 24, 2023

This fixes #146 properly. Also fixes #536.

@agronholm agronholm merged commit 6b0a1f3 into master Jul 25, 2023
12 checks passed
@agronholm agronholm deleted the dont_cancel_finished_futures branch July 25, 2023 19:21
@gschaffner
Copy link
Collaborator

wow, this is a much simpler technique than my proposed one. thank you!

@gschaffner
Copy link
Collaborator

gschaffner commented Aug 10, 2023

one concern: doesn't this fail when native cancellations are in the mix? for example:

@pytest.mark.parametrize("anyio_backend", ["asyncio"])
async def test_event_wait_before_set_before_asyncio_cancel() -> None:
    # compare this to test_event_wait_before_set_before_cancel.
    waiter_woke = False
    event = anyio.Event()

    async def waiter() -> None:
        nonlocal waiter_woke
        asyncio.get_running_loop().call_soon(setter)
        await event.wait()
        waiter_woke = True

    def setter() -> None:
        event.set()
        # trigger the race condition where cancel() is called before waiter() gets
        # rescheduled
        native_waiter.cancel()

    native_waiter = asyncio.create_task(waiter())
    await native_waiter # raises, but it shouldn't
    assert waiter_woke

(in this example I would expect anyio.Event to still behave like trio.Event.)

this means that if native cancellations are in the mix (e.g. due to third-party asyncio libraries), bugs like #146 (and more generally #536) can still happen.

the approach of #537 doesn't have this problem. (although, #537 only touches Event, and there may be other primitives with similar issues that this patch touches but #537 didn't.)

i suspect that this problem is probably quite rare (although asyncio cancellations might become more common with asyncio.timeout introduced in 3.11?), but it may still be worth mentioning.

@agronholm
Copy link
Owner Author

Asyncio's cancellation, as implemented today, is not trivially fixable, and requires some hard decisions upstream. I've had a lengthy post about this in the works for a while which I will post to Async-SIG.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

On asyncio, Event.set() sometimes fails to notify all waiting tasks Object stream randomly drops items
2 participants