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

FastAPI integration Async Redis session dependency override, error during test RuntimeError: <Queue > is bound to a different event loop #292

Closed
christiansicari opened this issue Feb 29, 2024 · 18 comments · Fixed by #312
Labels
bug Something isn't working

Comments

@christiansicari
Copy link

christiansicari commented Feb 29, 2024

Describe the bug
I am currently developing a FastAPI app that gets a Redis Session from a Dependency Injection, and uses it to write some data on Redis.
I need to test the endpoint, therefore I created a pytext fixture which in turn:

  1. create a FakeAsyncRedis
  2. override the dependency in the endpoint
  3. return the same FakeAsyncRedis object

in the test basically I call the endpoint, and I read the key I expect to get set, but I get the following error
test_redis - RuntimeError: <Queue at 0x111538a10 maxsize=0 tasks=1> is bound to a different event loop

To Reproduce
My fixtures are:

@pytest.fixture(scope="session")
def event_loop():
    policy = asyncio.get_event_loop_policy()
    loop = policy.new_event_loop()
    yield loop
    loop.close()


@pytest.fixture
async def redis(event_loop: asyncio.AbstractEventLoop):
    redis_session = fakeredis.FakeAsyncRedis(decode_responses=True, encoding="utf-8", )

    async def _get_async_redis_session() -> AsyncIterator[fakeredis.FakeAsyncRedis]:
        yield redis_session

    for app in all_fast_apps:
        app.dependency_overrides[get_async_redis_session] = _get_async_redis_session
    return redis_session

This is my dependency used to fetch a Redis Connection:

from redis import asyncio as aioredis
async def get_async_redis_session() -> AsyncIterator[Redis]:
    async with aioredis.from_url(redisUrl, **redis_args) as session:
        yield session
    await session.close()

and this is the endpoint definition:

async def write_on_redis(
    request: Request,
    redis: Redis = Depends(get_async_redis_session)
) :
  await redis.set(name=key, value=value, ex=expire)

Finally the test where I get the error:

def test_write(redis):  # redis is the fixture we defined before

       url = "myapiendpoint"
        response = client.get(url)
       value = await redis.get(key) <-- this throws the error

Expected behavior
Get the key set before in the endpoint

Desktop (please complete the following information):

  • OS: [e.g. iOS]
  • fakeredis==2.21.1
  • redis==4.6.0

Upvote & Fund

  • We're using Polar.sh so you can upvote and help fund this issue.
  • We receive the funding once the issue is completed & confirmed by you.
  • Thank you in advance for helping prioritize & fund our backlog.
Fund with Polar
@christiansicari christiansicari added the bug Something isn't working label Feb 29, 2024
@cunla
Copy link
Owner

cunla commented Feb 29, 2024

Can you post the full stack trace?

I am not sure, but I think you don't need to include the event_loop when defining the redis fixture.

@pytest.fixture
async def redis():
    redis_session = fakeredis.FakeAsyncRedis(decode_responses=True, encoding="utf-8", )

    async def _get_async_redis_session() -> AsyncIterator[fakeredis.FakeAsyncRedis]:
        yield redis_session

    for app in all_fast_apps:
        app.dependency_overrides[get_async_redis_session] = _get_async_redis_session
    return redis_session

You can also see how the fixture is defined in the tests for this package (remove the logic for fake vs. real)

@cunla
Copy link
Owner

cunla commented Feb 29, 2024

I don't think this is related to FakeRedis..

See this thread:
https://stackoverflow.com/questions/53724665/using-queues-results-in-asyncio-exception-got-future-future-pending-attached

I suspect you are creating your FastAPI server and the fakeredis connection on different event-loops

@cunla
Copy link
Owner

cunla commented Feb 29, 2024

Here you are creating a new event-loop:

@pytest.fixture(scope="session")
def event_loop():
    policy = asyncio.get_event_loop_policy()
    loop = policy.new_event_loop()
    yield loop
    loop.close()

@cunla
Copy link
Owner

cunla commented Mar 1, 2024

Please update here.
If you are running into this, I am sure others do as well :)

@steve-mavens
Copy link

steve-mavens commented Mar 11, 2024

Indeed I have also hit this problem. I couldn't figure out how to get it all working. You'd think that event_loop fixture should be unnecessary, but it's a workaround for a different issue where sometimes if you don't create your own, pytest_asyncio ends up closing the event loop at the "wrong" time, and you get warnings from destructors that things are not as they should be. I can't remember what happened when I tried making the redis fixture depend on the custom event_loop fixture to straighten out the order everything was created, but based on the code in this ticket, that's not sufficient.

Anyway in the end I worked around it using mock.patch (of a function called by my fastapi dependency function) instead of the dependency override. I'll certainly be interested in a better fix if anyone comes up with one, but in the mean time I'll stick to my general claim that Python's mutable module namespaces already are a de facto dependency injection system, and we can just about get along without adding another one ;-)

@christiansicari
Copy link
Author

Indeed I have also hit this problem. I couldn't figure out how to get it all working. You'd think that event_loop fixture should be unnecessary, but it's a workaround for a different issue where sometimes if you don't create your own, pytest_asyncio ends up closing the event loop at the "wrong" time, and you get warnings from destructors that things are not as they should be. I can't remember what happened when I tried making the redis fixture depend on the custom event_loop fixture to straighten out the order everything was created, but based on the code in this ticket, that's not sufficient.

Anyway in the end I worked around it using mock.patch (of a function called by my fastapi dependency function) instead of the dependency override. I'll certainly be interested in a better fix if anyone comes up with one, but in the meantime I'll stick to my general claim that Python's mutable module namespaces already are a de facto dependency injection system, and we can just about get along without adding another one ;-)

Hi Steve, do you mind publishing the snippet of your solution? it might be useful for other people (and for me :) )
I am currently mocking redis with a simple dict. It does its work, but it's not even cool to see

@steve-mavens
Copy link

steve-mavens commented Mar 12, 2024

@christiansicari there's a bit of redaction here since it's proprietary code, and I haven't included all the imports, but it's a bit like this:

from fakeredis.aioredis import FakeRedis
from httpx import AsyncClient
from .main import app  # this is the fastapi app object
from .main import RedisBackend # this is a class with a bunch of methods with the Redis code for the actions I perform

@pytest_asyncio.fixture(scope="function")
async def redis_conn() -> AsyncIterator[Redis[str]]:
    # Actually I have some code here to return fakeredis or real redis according to a pytest 
    # command-line option, but that's not really relevant. It's the reason I bother here with
    # the async context, though. There's no cleanup needed with FakeRedis alone.
    async with FakeRedis(decode_responses=True, version=(6,)) as redis_conn:
        await redis_conn.flushdb()
        yield redis_conn

@pytest_asyncio.fixture(scope="function")
async def client(redis_conn: Redis[str]) -> AsyncIterator[AsyncClient]:
    with mock.patch("main.create_backend", return_value=RedisBackend(redis_conn)):
        app.dependency_overrides[_some_other_injected_dependency] = _some_other_override
        async with AsyncClient(app=app, base_url="http://testserver") as client:
            yield client

In main.py, create_backend is not the function used in fastapi.Depends (I don't think patching that would work, since it's bound by value and not by name before the test code gets a chance to patch it). Rather, it's a function called by name in the function used in Depends.

Then I have a bunch of async tests that use the client fixture to make async http calls on the app object, and test its API that way. Separately I have async tests for the RedisBackend object, that use the redis_conn fixture directly. What I could never get working, due to the event loop error, was synchronous tests using fastapi.testclient.TestClient.

It might be that the code here is obvious, and my key insight was to give up on trying to write synchronous tests. The code you posted has def test_write ...await redis.get. I would expect that to never possibly work, just because you can't await in a sync function, but it's not clear to me whether your tests are actually sync or async.

@steve-mavens
Copy link

steve-mavens commented Mar 12, 2024

I should probably warn that this code is a bit scrappy. It doesn't bother undoing the modification to app.dependency_overrides, which happens to work with my code because every test that uses the app at all, uses it via this fixture and wants the same dependency override. It wouldn't work if I had different tests that used a different fixture and didn't want the override of that other dependency.

@cunla
Copy link
Owner

cunla commented Mar 12, 2024

@steve-mavens do you mind adding it in the docs? around here would probably be good.

@steve-mavens
Copy link

I can add something about how to intersect fastapi and async fakeredis :-) I'm making some assumptions about how the fastapi app is set up, and I don't know how typical my code is. I'm not deeply involved in the fastapi community to know what's most common: I just got the dependencies I needed working and then mostly forgot about them.

@cunla
Copy link
Owner

cunla commented Mar 31, 2024

Do you mind reviewing this section in the documentation and see whether it makes sense. If so, close this issue please

@steve-mavens
Copy link

I don't think it's complete. The code you've used doesn't explain what create_backend is, so I don't think there's enough information there for a fastapi user to adapt the code snippet to their app. The reason I haven't done anything on this yet is that it's a fair bit of work to produce a minimal example, figure out under what circumstances the "different event loop" problem actually bites, and therefore what the best fix is to advise. What worked for me is the combination of avoiding app.dependency_overrides and writing all the tests as async.

@cunla
Copy link
Owner

cunla commented Apr 2, 2024

Well, I'll keep this open, if any of you can spend some time to improve the docs it would be great. I haven't worked with FastAPI so I am not in a position to write it.

@steve-mavens
Copy link

steve-mavens commented May 14, 2024

I've just updated one of my projects to the latest pytest-asyncio (released March 19th), and at least for my usage the need to have a session-scoped event loop has gone away. I can just remove it and I don't get the "bound to a different event loop" errors any more.

The reason I even made the experiment of removing it is that pytest-asyncio has deprecated user-redefining the event_loop fixture, and I wanted to make that warning go away :-). I haven't investigated what the actual change is in pytest-asyncio that improved the cleanup such that it's no longer needed.

It might be that this is only fixed because all my other fixtures are function-scoped as well, but as a brief experiment I did also get the tests working by setting some of my fixtures to scope="session", and also using pytest.mark.asyncio(scope="session"). So I'm cautiously optimistic that it could also work now if there were some reason the fakeredis fixture did need to be session-scoped (being used as a fastapi dependency override is exactly such a reason).

@christiansicari would it be possible for you to check this for your usage? That is:

  • update to pytest-asyncio v.0.23.6
  • if you used my hack then change the fixture back to setting the fastapi dependency override
  • set scope="session" on the fixture
  • set @pytest.mark.asyncio(scope="session") on all the the tests that use this fixture.

Then see whether the event loop errors have gone.

Hopefully tomorrow or the day after I will finish everything I'm doing on this project I've changed today, and move on to my project from which I took the code snippet above. Then I can test the fastapi dependency override for myself.

@sjjessop
Copy link
Contributor

sjjessop commented Jun 7, 2024

@cunla I have created a standalone FastAPI app that uses Redis, and written a test for it to demonstrate the dependency override. Then I've copied that code into your docs and the PR is [edit: 311, which I have closed]

This does not need the eventloop fixture like in @christiansicari 's original question. I believe this to be due to improvements in pytest-asyncio since this question was asked, since my project used to need the same thing and no longer does.

@sjjessop
Copy link
Contributor

sjjessop commented Jun 7, 2024

Something is very wrong with that PR. For me the page now just shows a massive flickering discord URL. I have no idea how, and I'm now going to check from another machine. Could be some kind of compromise. Anyway, here is an alternative PR that should be equivalent: #312

@sjjessop
Copy link
Contributor

sjjessop commented Jun 8, 2024

The issue is that a user named "simulified" posted a comment to the PR with content that (when Javascript is enabled) replaces the whole content of the page with an image. I've reported the user to github, but I don't know if that will have any effect. I have also blocked the user, although that doesn't prevent their comments appearing on this repo's PRs. When I load the page with javascript disabled, I can see the malicious comment, but github has decided in its wisdom that the "..." menu on comments should only work with Javascript enabled. So, I can't even try to delete the comment. I don't believe it would work anyway, since I think I need to have write access to the repo to delete comments even on PRs I raised.

The other PR is OK for now, but this feels like the kind of exploit that could hit every repo on github within hours until they deal with it...

@cunla
Copy link
Owner

cunla commented Jun 8, 2024

Thanks for working on this. I'll review the PR later today.
I also reported simulified - and even I couldn't delete it (as the repo admin :) )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants