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

Error of "attached to a different loop" appears in 0.11.0 but not in 0.10.0 #154

Closed
krizex opened this issue Apr 21, 2020 · 28 comments · Fixed by #156
Closed

Error of "attached to a different loop" appears in 0.11.0 but not in 0.10.0 #154

krizex opened this issue Apr 21, 2020 · 28 comments · Fixed by #156

Comments

@krizex
Copy link

krizex commented Apr 21, 2020

My testcase get the error of "attached to a different loop" in 0.11.0 but works fine in 0.10.0

The detail is I have the following code in my testcase:

asyncio.get_event_loop().run_in_executor(...)

Is there any related change in 0.11.0?

A simple testcase for reproducing:

import asyncio
import pytest

@pytest.fixture(scope='function')
async def loop():
    return asyncio.get_event_loop()

def foo():
    return 0

@pytest.mark.asyncio
async def test_async(loop):
    await loop.run_in_executor(None, foo)
@simonfagerholm
Copy link
Contributor

@krizex Can you provide the shortest possible example that reproduces this?

@krizex
Copy link
Author

krizex commented Apr 21, 2020

@simonfagerholm Thanks for you quick response! Updated the testcase in my original post, please check.

elemoine pushed a commit to procrastinate-org/procrastinate that referenced this issue Apr 21, 2020
tests/integration/test_aiopg_connector.py::test_execute_query_simultaneous fails
with pytest-asyncio 0.11.0.

See pytest-dev/pytest-asyncio#154.
@elemoine
Copy link

We're getting hit by the same issue. We need to stick to 0.10.0 for now.

elemoine pushed a commit to procrastinate-org/procrastinate that referenced this issue Apr 21, 2020
tests/integration/test_aiopg_connector.py::test_execute_query_simultaneous fails
with pytest-asyncio 0.11.0.

See pytest-dev/pytest-asyncio#154.
@Gr1N
Copy link

Gr1N commented Apr 21, 2020

Got the same issue with 0.11.0...

@simonfagerholm
Copy link
Contributor

I tested a little bit, it seems the event_loop fixture is not started before the fixture foo in your example. That means that when event_loop is started it will create a new loop to run the test.
The behaviour seems to have changed a bit in the plugin hooks, I suspect pytest_fixture_setup might be the culprit.

By including event_loop as a dependency of foo the example test works.

Also the loop provided by event_loop can be used directly like this:

import pytest

def foo():
    return 0

@pytest.mark.asyncio
async def test_async(event_loop):
    await event_loop.run_in_executor(None, foo)

Would that work as a work around?

@krizex
Copy link
Author

krizex commented Apr 21, 2020

The example testcase is just for reproducing the issue. My real use case is not that simple and use asyncio.get_event_loop in some low level module so I could not leverage the event_loop fixture.

At this moment I will stick to v0.10.0 until the issue been fixed.

@simonfagerholm
Copy link
Contributor

Is it possible to make your fixture dependant on the event_loop, i.e add event_loop as an argument to the fixture?

@Gr1N
Copy link

Gr1N commented Apr 21, 2020

It will be not only about tests to rewrite. Because of that you somehow need to pass event_loop fixture in your production code...

@simonfagerholm
Copy link
Contributor

No, as long as the event_loop fixture is started before the production code is executed get_event_loop should return the loop from event_loop

@krizex
Copy link
Author

krizex commented Apr 21, 2020

No, the fixture is constructed by calling some low level module and the low level module call asyncio.get_event_loop() to get the event loop. To leverage the event_loop we have to introduce a new param in the code path to pass it to the low level module which is not pratical...

BTW, it is a backward compatibility issue so we have to fix it in the library side.

@krizex
Copy link
Author

krizex commented Apr 21, 2020

No, as long as the event_loop fixture is started before the production code is executed get_event_loop should return the loop from event_loop

As you said, why not setup the event_loop fixture by default so this issue can be resolved?

@simonfagerholm
Copy link
Contributor

I agree that it should be fixed, I am just suggesting a work around that if it solves the issue can be implemented.
If it doesn't work my diagnosis is incorrect and further investigation is needed. As I don't have your code causing this problem I cannot try if the diagnosis is correct. Both of my work arounds work in your example.

Also you are free to create a fix and a PR for this issue, if you think you have a solution.

I'm not a maintainer of this project, just your friendly neighborhood sp...programmer, that took some time out of my schedule to help you with a problem

@krizex
Copy link
Author

krizex commented Apr 21, 2020

@simonfagerholm Thanks for your suggestion and I can confirm that adding event_loop to the fixture which code path finally calls asyncio.get_event_loop could fix the problem. I have verified in my testcase so I think your suspicion is correct!

@simonfagerholm
Copy link
Contributor

@krizex Great, thanks for the help with confirming! I will see if I can create a fix in the hooks later today

@krizex
Copy link
Author

krizex commented Apr 21, 2020

To apply your suggestion to the example code, just add the event_loop to the loop fixture. Like the following code segment works:

import asyncio
import pytest

@pytest.fixture(scope='function')
async def loop(event_loop):
    return asyncio.get_event_loop()

def foo():
    return 0

@pytest.mark.asyncio
async def test_async(loop):
    await loop.run_in_executor(None, foo)

@SteveJones
Copy link

As an aside, I think I'm seeing this issue cause an odd problem with aiohttp. This code:

import aiohttp
from pytest import mark, fixture


@fixture
async def session():
    async with aiohttp.ClientSession() as s:
        yield s


@mark.asyncio
async def test_session(session):
    async with session.get("http://www.example.org/"):
        pass

Causes this error

RuntimeError: Timeout context manager should be used inside a task

In 0.11 but not in 0.10. Seems to caused by the event loops being different - i.e. I think this is the same issue.

@layday
Copy link

layday commented Apr 21, 2020

It is the same issue, the task is not attached to the loop the timeout context manager is run from.

layday added a commit to layday/instawow that referenced this issue Apr 21, 2020
@Tinche
Copy link
Member

Tinche commented Apr 21, 2020

Just going through these issues real quick.

Note that at a certain point, the asyncio ecosystem moved away from passing loops explicitly between components. (Maybe around Python 3.7 when get_running_loop was introduced?)

The event loop is basically a global variable. (It's a little more complex than that, but let's go with it.) If you're using the event_loop fixture, you're asking pytest-asyncio to take care of the loop. If you're running your tests using pytest.mark.asyncio, you're asking pytest to run your test using the event loop from the event_loop fixture, so these tests depend on the event_loop fixture too.

When you have a fixture like

@fixture
async def session():
    async with aiohttp.ClientSession() as s:
        yield s

the problem is - it depends on the event loop too, it's just hidden from you since it's basically a global variable, like we said. The order in which pytest initializes unrelated fixtures is, as far as I know, unspecified (so from the perspective of end users, basically random).

The correct fix is to let pytest know the session fixture here depends on the event_loop fixture, which it does.

I guess the question for us is can we save this boilerplate for our users.

@SteveJones
Copy link

One problem with depending on the event_loop fixture is that while it works for that reduced test case it fails for my real test case (it just hangs) and I don't understand why yet.

@newAM
Copy link

newAM commented Apr 21, 2020

I had random hangs upon updating too. I just added event_loop to my fixtures and everything started working again.

@Tinche
Copy link
Member

Tinche commented Apr 21, 2020

We should probably document this somewhere.

@simonfagerholm
Copy link
Contributor

@Tinche I think I agree with you, going by ZEN 2: "Explicit is better than implicit" the fixture should explicitly declare that it depends on the event_loop-fixture. However the need for this declaration is not explicit as it is right now, possibly rectified by updated documentation.

What I don't currently understand is why the event_loop fixture is not started before the other fixtures when they don't declare it explicitly. pytest states in the documentation that the order of fixtures is scope-dependant (higher to lower), "argument order"-dependant, but still honoring dependencies and executing auto-use first.
So given that the event_loop-fixture is added as the first argument of the test case, shouldn't it be started first?
The only reasons I can see it wouldn't are either:

  • incorrect scope of async-fixtures (not function)
  • autouse async fixtures
  • event_loop in test case arguments, but after other async-fixtures

But the example has none of these.

@simonfagerholm
Copy link
Contributor

Wow, I just realised: pytest-asyncio appends the event_loop in the fixture list, but I think it should be inserted first.`

if 'asyncio' in item.keywords and 'event_loop' not in item.fixturenames:
    # inject an event loop fixture for all async tests
    item.fixturenames.append('event_loop')

Tested very quickly and it seems to solve it for the example.

@Tinche What do you think about changing this?

I also think that if you specify it yourself we maybe should change the order so it's still first or give a warning.

@simonfagerholm
Copy link
Contributor

It seems this was introduced in #122, where the event_loop is no longer automatically added to fixtures. see 1db3ea9 line 106:

if 'event_loop' not in fixturedef.argnames:
    fixturedef.argnames += ('event_loop', )

Ironically this could mean that #124 is already solved, while causing problems for the people in this thread.
I think it would be better to do it like I suggested above, i.e. changing:

if 'asyncio' in item.keywords and 'event_loop' not in item.fixturenames:
    # inject an event loop fixture for all async tests
    item.fixturenames.append('event_loop')

into:

if 'asyncio' in item.keywords:
    # inject an event loop fixture for all async tests
    if 'event_loop' in item.fixturenames:
        item.fixturenames.remove('event_loop')
    item.fixturenames.insert(0, 'event_loop')

@Tinche
Copy link
Member

Tinche commented Apr 22, 2020

@simonfagerholm Interesting, good research! This looks backwards compatible to me, can you put together a PR?

@simonfagerholm
Copy link
Contributor

@Tinche Sure, I'll do it tonight

@simonfagerholm
Copy link
Contributor

simonfagerholm commented Apr 22, 2020

I started doing the changes, but this test throws me for a loop (sorry for the pun ;)):

class TestUnexistingLoop:
    @pytest.fixture
    def remove_loop(self):
        old_loop = asyncio.get_event_loop()
        asyncio.set_event_loop(None)
        yield
        asyncio.set_event_loop(old_loop)

    @pytest.mark.asyncio
    async def test_asyncio_marker_without_loop(self, remove_loop):
        """Test the asyncio pytest marker in a Test class."""
        ret = await async_coro()
        assert ret == 'ok'

Of course this fails with the new implementation, as the event_loop-fixture starts before the fixture that removes the loop. What I don't fully understand is the use case of this behaviour.

It was added in #64, but a lot has changed since then so I'll continue digging.

Discussion about the problem is continued in PR #156

@Lenka42
Copy link

Lenka42 commented Apr 23, 2020

@simonfagerholm I confirm, your changes help in our case 👍

nolar added a commit to nolar/kopf-fork-from-zalando-incubator that referenced this issue Apr 27, 2020
marmarek added a commit to marmarek/qubes-core-qrexec that referenced this issue May 2, 2020
pytest-asyncio 0.11.0 uses different loops for fixtures and the actual
test and also closes event loop in some cases. Downgrade to 0.10.0 until
it's fixed.

pytest-dev/pytest-asyncio#154
pytest-dev/pytest-asyncio#157
Tinche pushed a commit that referenced this issue May 3, 2020
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 a pull request may close this issue.

9 participants