Add cleanup code to event_loop that mimics the behavior of asyncio.run #309

Askaholic · 2022-03-12T21:38:53Z

This is something I found useful and implemented in one of my own projects so I thought I'd make a PR in case the maintainers think this would be a good addition to the library itself. I'd be happy to modify it if only a subset of the changes is desired.

Some issues that I ran into with the current implementation:

Errors that occur in a task spawned during one test can leak into subsequent tests since simply calling loop.close() is not sufficient to shutdown the event loop. This can make it extremely difficult to track down which test is causing the errors, since the test that fails is not actually the one that has the problem.
When running with -W error under python 3.9 a similar problem happens with ResourceWarnings, where it becomes extremely difficult to figure out which test is actually opening the resources that are generating the warnings since the GC doesn't actually clean them up until some later test is already running.

Closes #200

Askaholic · 2022-03-12T21:40:41Z

pytest_asyncio/plugin.py

-    loop.close()
+    # Cleanup code copied from the implementation of asyncio.run()
+    try:
+        asyncio.runners._cancel_all_tasks(loop)


Calling private functions here is a bit sketch, but it's what I chose to do in my project instead of copy/pasting the entire implementation. Might want to go with the copy/paste approach though for this library.

Sorry for the late response… There are a couple of ideas flying around, all of which address this issue in some way. That makes it hard to review your PR, so I put it off for too long.

I agree that calling _cancel_all_tasks might not be a good idea. Although we test with different CPython versions, _cancel_all_tasks is an implementation detail that could change in a patch release. Since we only test different minor releases (i.e. 3.7, 3.8, …) a change of _cancel_all_tasks could slip through unnoticed.

It would be best to replicate the behaviour of _cancel_all_tasks in pytest-asyncio. We cant't do a 1:1 copy and paste, though, because the CPython code is protected by copyright and under a different license than pytest-asyncio.

I still stand by my last comment on the use of _cancel_all_tasks.

seifertm · 2022-03-17T08:42:20Z

Thanks for the contribution!

The linked issue is not the only request of that feature. There is also #222. We closed #222 in favour of #235, which intends to used aioloop-proxy to have a "virtualized" event loop.

The aioloop-proxy issue has not been started, as far as I know. I think it's a good idea to add clean up code for outstanding tasks independently of #235.

Could you add some tests that assert the correct behaviour of your code?

Askaholic · 2022-03-18T06:07:46Z

Great! I've read through the issues you linked and I think I understand the current state of this feature. Sounds like eventually a more sophisticated solution will be added, but for now this PR will be useful. I'll get the development environment set up and work on writing a few tests!

asvetlov · 2022-03-18T12:31:02Z

Regarding #235 (aioloop-proxy based solution).
I have a work-in-progress draft: #312
It is not finished yet, sorry. Now I'm working on improving asyncio itself, Python 3.11 feature freeze is coming.

pytest_asyncio/plugin.py

tests/test_event_loop_cleanup.py

seifertm · 2022-04-09T12:19:46Z

Thanks for uploading the latest version. I had some time to look into the issue that causes ResourceWarnings in the tests.

The culprit seems to be test_event_loop_scope.py::test_4. The test case requests the event_loop fixture and sets the current global loop to None. Requesting event_loop initializes a new loop. The code in pytest_fixture_post_finalizer tries to retrieve the current loop via asyncio.get_event_loop_policy().get_event_loop(), but that reference was set to None in the test case. The result is a stale event loop that triggers a ResourceWarning about an unclosed socket.

I was trying to work around the issue by using fixturedef.cached_result in pytest_fixture_post_finalizer to retrieve the loop provided by the event_loop fixture. Until now that wasn't successful, though. I'll keep digging…

…effects on other tests. Signed-off-by: Michael Seifert <[email protected]>

…ignature. This prevents gc.collect() from being unreachable. Signed-off-by: Michael Seifert <[email protected]>

…l_tasks. Signed-off-by: Michael Seifert <[email protected]>

codecov-commenter · 2022-09-13T19:51:06Z

Codecov Report

Base: 93.81% // Head: 93.23% // Decreases project coverage by -0.57% ⚠️

Coverage data is based on head (5024eb8) compared to base (5697df2).
Patch has no changes to coverable lines.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #309      +/-   ##
==========================================
- Coverage   93.81%   93.23%   -0.58%     
==========================================
  Files           3        3              
  Lines         275      281       +6     
  Branches       41       43       +2     
==========================================
+ Hits          258      262       +4     
- Misses         11       12       +1     
- Partials        6        7       +1

Impacted Files	Coverage Δ
plugin.py	`93.11% <0.00%> (-0.59%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

seifertm · 2022-09-13T19:53:20Z

I modified test_event_loop_scope.py::test_4 accordingly to avoid side-effects in other tests. The PR has been rebased and two typing issues have been addressed.

seifertm · 2022-09-13T20:02:17Z

pytest_asyncio/plugin.py

+    # as possible (these are triggered in various __del__ methods).
+    # Without this, resources opened in one test can fail other tests
+    # when the warning is generated.
+    gc.collect()


A bunch of people implement their own event_loop fixture, in order to control fixture scope. If we change the fixture definition, we'd have to inform them to adjust as well.

Asking users to remove loop.close() to benefit from the new cleanup code is an easy sell. Adding a call to gc.collect in the event_loop fixture not so much.

Is there away around the gc.collect call in the fixture code?

I suppose the call to gc.collect isn't directly related to task cleanup. I added this in my codebase because there were some tests that opened sockets and never closed them. Having the garbage collector run during the test teardown would make sure that the correct test failed when running pytest with -W error. But this is unrelated to pytest-asyncio so maybe it doesn't belong here.

elupus · 2022-11-27T09:56:30Z

I've run into the issue of tasks leaking over test bounderings when i tried to get home assistant updated to later pytest-asyncio: So far i have these fixes/hacks: I've not managed to get it to run reliable as of yet thou.

Cleaning up of lingering tasks:
home-assistant/core@6e9130d

Cleaning up of lingering timers:
home-assistant/core@4d1335f

elupus · 2022-11-27T10:04:02Z

I also just noticed that aiohttp loop fixture did trigger a GC call after test finishes: https://github.com/aio-libs/aiohttp/blob/905e0625777c20e4cf2d57fcfd4355827312f797/aiohttp/test_utils.py#L525

That likely explains some of my failures.

Some test will trigger warnings on garbage collect, these warnings spills over into next test. Some test trigger tasks that raise errors on shutdown, these spill over into next test. This is to mimic older pytest-aiohttp and it's behaviour on test cleanup. Discussions on similar changes for pytest-aiohttp are here: pytest-dev/pytest-asyncio#309

* Upgrade pytest-aiohttp * Make sure executors, tasks and timers are closed Some test will trigger warnings on garbage collect, these warnings spills over into next test. Some test trigger tasks that raise errors on shutdown, these spill over into next test. This is to mimic older pytest-aiohttp and it's behaviour on test cleanup. Discussions on similar changes for pytest-aiohttp are here: pytest-dev/pytest-asyncio#309 * Replace loop with event_loop * Make sure time is frozen for tests * Make sure the ConditionType is not async /home-assistant/homeassistant/helpers/template.py:2082: RuntimeWarning: coroutine 'AsyncMockMixin._execute_mock_call' was never awaited def wrapper(*args, **kwargs): Enable tracemalloc to get traceback where the object was allocated. See https://docs.pytest.org/en/stable/how-to/capture-warnings.html#resource-warnings for more info. * Increase litejet press tests with a factor 10 The times are simulated anyway, and we can't stop the normal event from occuring. * Use async handlers for aiohttp tests/components/motioneye/test_camera.py::test_get_still_image_from_camera tests/components/motioneye/test_camera.py::test_get_still_image_from_camera tests/components/motioneye/test_camera.py::test_get_stream_from_camera tests/components/motioneye/test_camera.py::test_get_stream_from_camera tests/components/motioneye/test_camera.py::test_camera_option_stream_url_template tests/components/motioneye/test_camera.py::test_camera_option_stream_url_template /Users/joakim/src/hass/home-assistant/venv/lib/python3.9/site-packages/aiohttp/web_urldispatcher.py:189: DeprecationWarning: Bare functions are deprecated, use async ones warnings.warn( * Switch to freezegun in modbus tests The tests allowed clock to tick in between steps * Make sure skybell object are fully mocked Old tests would trigger attempts to post to could services: ``` DEBUG:aioskybell:HTTP post https://cloud.myskybell.com/api/v3/login/ Request with headers: {'content-type': 'application/json', 'accept': '*/*', 'x-skybell-app-id': 'd2b542c7-a7e4-4e1e-b77d-2b76911c7c46', 'x-skybell-client-id': '1f36a3c0-6dee-4997-a6db-4e1c67338e57'} ``` * Fix sorting that broke after rebase

matzipan · 2023-06-09T13:48:46Z

So are the open threads the only things left for this to go forward? Or is there something else missing? I'll try to find some time for this.

Askaholic requested review from asvetlov, seifertm and Tinche as code owners March 12, 2022 21:38

Askaholic commented Mar 12, 2022

View reviewed changes

seifertm mentioned this pull request Mar 17, 2022

Deprecate event_loop fixture overrides #311

Closed

seifertm reviewed Mar 25, 2022

View reviewed changes

pytest_asyncio/plugin.py Outdated Show resolved Hide resolved

tests/test_event_loop_cleanup.py Show resolved Hide resolved

Askaholic and others added 7 commits September 13, 2022 21:46

Add cleanup code to event_loop that mimics the behavior of asyncio.run

8a63a65

Skip cleanup if the loop was closed in the test

effc2e5

Add test for verifying task cancellation

046154d

Move cleanup code to pytest_fixture_post_finalizer

a43a7b5

test: Close event_loop in test_event_loop_scope/test_4 to avoid side-…

b7f1a16

…effects on other tests. Signed-off-by: Michael Seifert <[email protected]>

fix: Yield loop from event_loop fixture to avoid violating function s…

03ee1b1

…ignature. This prevents gc.collect() from being unreachable. Signed-off-by: Michael Seifert <[email protected]>

refactor: Ignore mypy problem when calling asyncio.runners._cancel_al…

5024eb8

…l_tasks. Signed-off-by: Michael Seifert <[email protected]>

seifertm force-pushed the event-loop-task-cleanup branch from b4f4d1b to 5024eb8 Compare September 13, 2022 19:46

seifertm reviewed Sep 13, 2022

View reviewed changes

matthew-at-qamcom mentioned this pull request Oct 27, 2022

Cleaning up pending tasks #435

Closed

zpz mentioned this pull request Jan 21, 2024

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

Open

seifertm mentioned this pull request Jul 15, 2024

Task finalizers run outside of event loop #878

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cleanup code to event_loop that mimics the behavior of asyncio.run #309

Add cleanup code to event_loop that mimics the behavior of asyncio.run #309

Askaholic commented Mar 12, 2022

Askaholic Mar 12, 2022

seifertm Mar 25, 2022

seifertm Sep 13, 2022

seifertm commented Mar 17, 2022

Askaholic commented Mar 18, 2022

asvetlov commented Mar 18, 2022

seifertm commented Apr 9, 2022

codecov-commenter commented Sep 13, 2022

seifertm commented Sep 13, 2022

seifertm Sep 13, 2022

Askaholic Sep 18, 2022

elupus commented Nov 27, 2022

elupus commented Nov 27, 2022

matzipan commented Jun 9, 2023

Add cleanup code to event_loop that mimics the behavior of asyncio.run #309

Are you sure you want to change the base?

Add cleanup code to event_loop that mimics the behavior of asyncio.run #309

Conversation

Askaholic commented Mar 12, 2022

Askaholic Mar 12, 2022

Choose a reason for hiding this comment

seifertm Mar 25, 2022

Choose a reason for hiding this comment

seifertm Sep 13, 2022

Choose a reason for hiding this comment

seifertm commented Mar 17, 2022

Askaholic commented Mar 18, 2022

asvetlov commented Mar 18, 2022

seifertm commented Apr 9, 2022

codecov-commenter commented Sep 13, 2022

Codecov Report

seifertm commented Sep 13, 2022

seifertm Sep 13, 2022

Choose a reason for hiding this comment

Askaholic Sep 18, 2022

Choose a reason for hiding this comment

elupus commented Nov 27, 2022

elupus commented Nov 27, 2022

matzipan commented Jun 9, 2023