Teardown of the event_loop fixture no longer replaces the event loop policy

[seifertm]

My understanding is that everyone is happy, if the event loop policy remains unaltered. I think I found a way to get rid of the code that resets the event loop policy.

Problem

Here is my understanding of the problem:
We need to make sure that each test case runs in an isolated fashion. Therefore, the event_loop fixture provides a fresh event loop and is automatically applied to all marked tests. When the fixture goes out of scope, it closes the event loop. Subsequent test cases would run into the following error:

RuntimeError: Event loop is closed 

This is prevented a dedicated fixture teardown prodecure. The teardown sets the event loop policy to None, which also disposes the existing, closed event loop. Subsequent calls to asyncio.get_event_loop() will return a fresh loop. The problem is that setting the event loop policy to None resets the policy to the system default, which may be undesired.

Solution

Instead of resetting the event loop policy, the same effect can be achieved by requesting a new loop to replace the existing, closed event loop:

@pytest.hookimpl(trylast=True) def pytest_fixture_post_finalizer(fixturedef, request): """Called after fixture teardown""" if fixturedef.argname == "event_loop": policy = asyncio.get_event_loop_policy() new_loop = policy.new_event_loop() # Replace existing event loop policy.set_event_loop(new_loop) # Ensure subsequent calls to get_event_loop() succeed 

Considerations

What I dislike about this solution is that a new loop is created during the teardown of the event_loop fixture. It feels wrong to do it here. The correct place would be to create a new loop during fixture setup. In fact, the fixture does create another loop so we effectively create two new event loops for each test run. This points to an issue with the order in which the fixtures are evaluated.

What are your thoughts on this approach?

Resolves #168
Resolves #188

[seifertm]

@wouterdb @bryevdv @Tinche Would this work for you?

[wouterdb]

I think this will work for us, thank you.

[bryevdv]

I'm not really knowledgeable enough to comment, but it seems plausible and we'll certainly try updating versions when it's available.

[seifertm]

@Tinche What are your thoughts on this?

[seifertm]

ping @Tinche :)

[seifertm]

I acknowledge that I created this PR without a previous design discussion. However, I do think this change fixes a real world problem and that the new fixture finalizer is cleaner than the existing code.

@Tinche If you can make the time for it, I'd be happy to adjust and rebase this PR or try a different approach based on your input on the matter.

[asvetlov]

LGTM.
A not-started event loop is a cheap object, it has only a pipe for self-wakeup plus a few empty data containers.

[seifertm]

Thanks for the feedback! I just rebased to master to fix a conflict in the changelog. It turns out the PR is now causing issues on Python 3.8. I'll have another look later today.

[seifertm]

I did the following changes to the PR:

• Adjusted formatting to satisfy black and flake8
• During tear down of the event_loop fixture: Explicitly close() the current event loop before requesting a new_event_loop() from the loop policy. This fixes an issue on Python 3.8 where random tests would fail due to a ResourceWarning. In my case, the warning was about unclosed Unix sockets.

All green now :)

[Tinche]

GJ, merging in.

If you're working on event loop teardown, there's an issue laying around here somewhere about shutting down the loop more robustly (#222), think we could incorporate that too?

[asvetlov]

Regarding #222 -- I think #235 can fix the proper teardown along with many other things.
I hope I can find time for PR soon.
If anybody can help me -- you are welcome, sure!

[asvetlov]

Please don't get me wrong -- I'm not opposed to step-by-step improvements.
I just see a generic solution for very many challenges; I wrote aioloop-proxy keeping in mind both pytest-asyncio and unittest.AsyncioTestCase.