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

AsyncGenerator early exit doesn't raise CancelledError and doesn't run finally branch #759

Open
zpz opened this issue Jan 21, 2024 · 4 comments
Labels
Milestone

Comments

@zpz
Copy link

zpz commented Jan 21, 2024

This code:

import asyncio

async def gen():
    try:
        for x in range(100):
            try:
                yield x
            except BaseException as e:
                print('raised', repr(e))
                raise
    finally:
        print('cleaning up!')


async def test_earlystop():
    async for x in gen():
        print(x)
        if x > 8:
            break
    assert True


if __name__ == '__main__':
    asyncio.run(test_earlystop())

Run it:

$ python debug.py 
0
1
2
3
4
5
6
7
8
9
raised CancelledError()
cleaning up!

I was surprised to see CancelledError rather than GeneratorExit but it's OK. Now this code:

import asyncio
import pytest

async def gen():
    try:
        for x in range(100):
            try:
                yield x
            except BaseException as e:
                print('raised', repr(e))
                raise
    finally:
        print('cleaning up!')


@pytest.mark.asyncio
async def test_earlystop():
    async for x in gen():
        print(x)
        if x > 8:
            break
    assert True

Run it,

$ py.test -sv debug.py 
=================================================================== test session starts ====================================================================
platform linux -- Python 3.10.12, pytest-7.4.4, pluggy-1.3.0 -- /usr/bin/python
rootdir: /home/docker-user/mpservice
configfile: pyproject.toml
plugins: asyncio-0.23.3, cov-4.1.0, anyio-4.2.0, Faker-22.4.0, pudb-0.7.0
asyncio: mode=strict
collected 1 item                                                                                                                                           

debug.py::test_earlystop 0
1
2
3
4
5
6
7
8
9
PASSED
-------------------------------------------------------------------- live log teardown ---------------------------------------------------------------------
ERROR    asyncio:base_events.py:1758 Task was destroyed but it is pending!
task: <Task pending name='Task-2' coro=<<async_generator_athrow without __name__>()>>


=================================================================== slowest 3 durations ====================================================================
0.11s setup    tests/debug.py::test_earlystop
0.00s teardown tests/debug.py::test_earlystop
0.00s call     tests/debug.py::test_earlystop
==================================================================== 1 passed in 0.18s =====================================================================

Now I don't understand why

  1. The CancelledError was not raised, it appears
  2. The finally block was not executed, it appears

Of course, that "destroyed but it is pending" is also unpleasant to see.

Is there a bug in pytest-asyncio or am I missing something? I think this code should simply pass with no drama of any kind.

@zpz
Copy link
Author

zpz commented Jan 21, 2024

I understand this is related to event_loop shutdown. If I add this

@pytest.fixture(scope='function')
def event_loop(request):
    loop = asyncio.get_event_loop_policy().new_event_loop()
    try:
        yield loop
    finally:
        try:
            asyncio.runners._cancel_all_tasks(loop)
            loop.run_until_complete(loop.shutdown_asyncgens())
            loop.run_until_complete(loop.shutdown_default_executor())
        finally:
            loop.close()

The 3 issues I mentioned are all gone, but there's this warning

  /usr/local/lib/python3.10/dist-packages/pytest_asyncio/plugin.py:749: DeprecationWarning: The event_loop fixture provided by pytest-asyncio has been redefined in
  /home/docker-user/mpservice/tests/debug.py:6
  Replacing the event_loop fixture with a custom implementation is deprecated
  and will lead to errors in the future.
  If you want to request an asyncio event loop with a scope other than function
  scope, use the "scope" argument to the asyncio mark when marking the tests.
  If you want to return different types of event loops, use the event_loop_policy
  fixture.

Two related issues are

#222

#309

@zpz
Copy link
Author

zpz commented Jan 21, 2024

Specifically, this line in event_loop shutdown fixes the issue

loop.run_until_complete(loop.shutdown_asyncgens())

I don't know why pytest-asyncio decides not to do this like in the standard asyncio event_loop. #222 sounds like it has fixed these things but it appears it did not...

@zpz
Copy link
Author

zpz commented Jan 21, 2024

If instead of hacking the event_loop, changing the test function into this

@pytest.mark.asyncio
async def test_earlystop():
    data = gen()
    async for x in data:
        print(x)
        if x > 8:
            break
    assert True
    await data.aclose()

then all the issues are gone. But, of course, regular user should not need to do that await data.aclose()!

@seifertm seifertm added this to the v1.0 milestone Jan 28, 2024
@seifertm seifertm added the bug label Jan 28, 2024
@seifertm
Copy link
Contributor

I agree that async generators should be cleaned up properly. Pytest-asyncio is currently in the middle of transitioning away from the event_loop fixture. You're seeing the DeprecationWarning, because users are supposed to get rid of their custom event_loop implementations.

The current v0.23 release still has some issues that have to be ironed out (especially #706), in order to provide a proper migration path for users. Once everything is done, I'm sure this issue will be resolved as well.

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

No branches or pull requests

2 participants