pytest-dev / pytest-asyncio

Asyncio support for pytest
https://pytest-asyncio.readthedocs.io
Apache License 2.0
1.43k stars 152 forks source link

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

Open zpz opened 10 months ago

zpz commented 10 months ago

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 commented 10 months ago

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

https://github.com/pytest-dev/pytest-asyncio/issues/222

https://github.com/pytest-dev/pytest-asyncio/pull/309

zpz commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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.

zpz commented 1 month ago

My workaround above using aclose() no longer makes it clean. I didn't deep dive.