Bug: declaring a RedisStore in the app triggers a 500 error in the 2nd set of parameters of a parametrized test

[euri10]

Description

If you have a parametrized test that uses a function-scoped fixture that uses itself a session-scoped fixture and you declare a RedisStore in the app, the 2nd test fails with RuntimeError: Event loop is closed

The 2nd test passes in those cases:

1. all fixtures are session-based (just comment the client fixture)
2. there is no RedisStore in the app (or replace with a MemoryStore)

URL to code causing the issue

No response

MCVE

import msgspec
import pytest

from litestar import Litestar, get
from litestar.middleware.session.server_side import ServerSideSessionConfig
from litestar.stores.redis import RedisStore
from litestar.testing import AsyncTestClient


@get()
async def hget() -> int:
    return 1


class AppSettings(msgspec.Struct):
    debug: bool
    redis_url: str = "redis://localhost:6379"


def get_app(app_settings: AppSettings) -> Litestar:
    # setting up stores
    session_store = RedisStore.with_client(url=app_settings.redis_url)
    app = Litestar(route_handlers=[hget],
                   middleware=[
                       ServerSideSessionConfig().middleware,
                   ],
                   stores={"sessions": session_store, },  # comment this and 2nd test pass
                   debug=app_settings.debug
                   )
    return app


@pytest.fixture(scope="session")
def anyio_backend() -> str:
    return "asyncio"


@pytest.fixture(scope="session")
def app_settings_test():
    return AppSettings(debug=True)


@pytest.fixture(scope="session")
def app_test(app_settings_test: AppSettings):
    app = get_app(app_settings_test)
    yield app


@pytest.fixture  # add scope="session" and 2nd test pass
async def client(app_test: Litestar):
    async with AsyncTestClient(app=app_test) as c:
        yield c


@pytest.mark.anyio
@pytest.mark.parametrize("p1, p2",
                         [(1, 2), (3, 4)])
async def test_param(client: AsyncTestClient, p1: int, p2: int):
    response = await client.get("/")
    assert response.status_code == 200

Steps to reproduce

1. Go to '...'
2. Click on '....'
3. Scroll down to '....'
4. See error

Screenshots

"![SCREENSHOT_DESCRIPTION](SCREENSHOT_LINK.png)"

Logs

No response

Litestar Version

2.5.5

Platform

• Linux
• Mac
• Windows
• Other (Please specify in the description above)

---

Note

While we are open for sponsoring on GitHub Sponsors and
OpenCollective, we also utilize Polar.sh to engage in pledge-based sponsorship.

Check out all issues funded or available for funding on our Polar.sh dashboard

• If you would like to see an issue prioritized, make a pledge towards it!
• We receive the pledge once the issue is completed & verified
• This, along with engagement in the community, helps us know which features are a priority to our users.

[euri10]

thinking out loud after I read this, shouldn't there be some close/aclose methods for stores, or at least one for the RedisStore ?

[euri10]

this can be circumvented in the get_app factory replacing the RedisStore with a MemoryStore only for tests but it feels like a hack

[guacs]

thinking out loud after I read this, shouldn't there be some close/aclose methods for stores, or at least one for the RedisStore ?

I agree that we should add this to the stores. Then Litestar could close the stores on app shutdown (make it configurable as to whether the store should be closed or not).

[euri10]

thinking out loud after I read this, shouldn't there be some close/aclose methods for stores, or at least one for the RedisStore ?

I agree that we should add this to the stores. Then Litestar could close the stores on app shutdown (make it configurable as to whether the store should be closed or not).

I greped all python Redis store in github and nobody seems to have methods to close stores weirdly enough, so we're not alone :)
I'm not sure this is related to the issue at hand though, but from experience tests and unclosed ressources usually dont make good friends.

[provinzkraut]

thinking out loud after I read this, shouldn't there be some close/aclose methods for stores, or at least one for the RedisStore ?

The reason this does not exist is that the store can receive an "externally managed" Redis instance. The idea here was that whoever passes it in is responsible for managing its lifetime as well. While I still think this is the right thing to do, I can see how it could be an issue when the with_client classmethod is used, since then it's the store that's responsible for creating the client instance.

Maybe the solution here is to add a flag to the RedisStore that indicates whether we should handle the client's lifetime?

I'm not sure this is related to the issue at hand though, but from experience tests and unclosed ressources usually dont make good friends.

There are/were quite a few other issues surrounding the async Redis client and lifetimes, e.g. it trying to perform some action in __del__ that relied on an active async context, so I wouldn't entirely dismiss the possibility that this is also just the redis client being weird / buggy.

You could try though to manage its lifetime explicitly and see if that fixes the issue.

[euri10]

You could try though to manage its lifetime explicitly and see if that fixes the issue.

something like this does work indeed, I'm not too sure this is not overly complicated though it does the job, and my app factory needs to be async now, and I need to wrap it in an async context manager that creates / closes the redis client :

from contextlib import asynccontextmanager
from typing import Iterator

import msgspec
import pytest
from redis.asyncio import Redis, ConnectionPool

from litestar import Litestar, get
from litestar.middleware.session.server_side import ServerSideSessionConfig
from litestar.testing import AsyncTestClient


@get()
async def hget() -> int:
    return 1


class AppSettings(msgspec.Struct):
    debug: bool
    redis_url: str = "redis://localhost:6379"


@asynccontextmanager
async def make_session_store(app_settings: AppSettings) -> Iterator[None]:
    pool = ConnectionPool.from_url(app_settings.redis_url)
    client = Redis.from_pool(pool)
    try:
        yield
    finally:
        await client.aclose()


async def get_app(app_settings: AppSettings) -> Litestar:
    # setting up stores
    # session_store = RedisStore.with_client(url=app_settings.redis_url)
    # session_store = RedisStore(redis=Redis(), namespace="sessions")
    # session_store = MemoryStore()
    session_config = ServerSideSessionConfig()
    async with make_session_store(app_settings) as session_store:
        app = Litestar(route_handlers=[hget],
                       middleware=[
                           session_config.middleware,
                       ],
                       stores={"sessions": session_store},  # comment this and 2nd test pass
                       debug=app_settings.debug,
                       # lifespan=[partial(lifespan, app_settings=app_settings)]
                       )
        return app


@pytest.fixture(scope="session")
def anyio_backend() -> str:
    return "asyncio"


@pytest.fixture(scope="session")
def app_settings_test():
    return AppSettings(debug=True)


@pytest.fixture(scope="session")
async def app_test(app_settings_test: AppSettings):
    app = await get_app(app_settings_test)
    yield app


@pytest.fixture  # add scope="session" and 2nd test pass
async def client(app_test: Litestar):
    async with AsyncTestClient(app=app_test) as c:
        yield c


@pytest.mark.anyio(scope="session")
@pytest.mark.parametrize("p1, p2",
                         [(1, 2), (3, 4)])
async def test_param(client: AsyncTestClient, p1: int, p2: int):
    response = await client.get("/")
    assert response.status_code == 200

[euri10]

actually maybe if there is a way to lazily load the store, or use a lifespan-created redis client into the store that would be maybe cleaner, not sure...

[provinzkraut]

actually maybe if there is a way to lazily load the store, or use a lifespan-created redis client into the store that would be maybe cleaner, not sure...

I think you should be able to create the store in a sync function and then have a lifespan context manager on the app instance that takes care of its shutdown.

[euri10]

actually maybe if there is a way to lazily load the store, or use a lifespan-created redis client into the store that would be maybe cleaner, not sure...

I think you should be able to create the store in a sync function and then have a lifespan context manager on the app instance that takes care of its shutdown.

alright yeah this is much better this way indeed, I just have to access the private redis attribute from the store in the lifespan in order to close it but that solves everything and looks not ugly like the above :)

thanks for the patience )

While I still think this is the right thing to do, I can see how it could be an issue when the with_client classmethod is used, since then it's the store that's responsible for creating the client instance.

Do you think I should maybe try "solve" this adding a note in the docs ? This is not trivial and as you said maybe unexpected at first. lmk

from contextlib import asynccontextmanager
from functools import partial
from typing import Iterator

import msgspec
import pytest
from redis.asyncio import Redis

from litestar import Litestar, get
from litestar.middleware.session.server_side import ServerSideSessionConfig
from litestar.stores.redis import RedisStore
from litestar.testing import AsyncTestClient


@get()
async def hget() -> int:
    return 1


class AppSettings(msgspec.Struct):
    debug: bool
    redis_url: str = "redis://localhost:6379"


@asynccontextmanager
async def lifespan(app: Litestar) -> Iterator[None]:
    try:
        yield
    finally:
        print("lifespan cleanup")
        await app.stores.get("sessions")._redis.aclose()


def get_app(app_settings: AppSettings) -> Litestar:
    # setting up stores
    # session_store = RedisStore.with_client(url=app_settings.redis_url)
    session_store = RedisStore(redis=Redis(), namespace="sessions")
    # session_store = MemoryStore()
    session_config = ServerSideSessionConfig()
    app = Litestar(route_handlers=[hget],
                   middleware=[
                       session_config.middleware,
                   ],
                   stores={"sessions": session_store},  # comment this and 2nd test pass
                   debug=app_settings.debug,
                   lifespan=[partial(lifespan, app_settings=app_settings)]
                   )
    return app


@pytest.fixture(scope="session")
def anyio_backend() -> str:
    return "asyncio"


@pytest.fixture(scope="session")
def app_settings_test():
    return AppSettings(debug=True)


@pytest.fixture(scope="session")
def app_test(app_settings_test: AppSettings):
    app = get_app(app_settings_test)
    yield app


@pytest.fixture  # add scope="session" and 2nd test pass
async def client(app_test: Litestar):
    async with AsyncTestClient(app=app_test) as c:
        yield c


@pytest.mark.anyio(scope="session")
@pytest.mark.parametrize("p1, p2",
                         [(1, 2), (3, 4)])
async def test_param(client: AsyncTestClient, p1: int, p2: int):
    response = await client.get("/")
    assert response.status_code == 200

[provinzkraut]

Do you think I should maybe try "solve" this adding a note in the docs ? This is not trivial and as you said maybe unexpected at first. lmk

I think there are two things that need to be done:

1. Add a note to the docs about passing in a Redis instance and that you're expected to handle its lifetime
2. Properly handle the lifetime of internally created Redis instances. Adding lifespan or on_startup/on_shutdown methods to the stores seems to be the most sensible option here, with a flag for the Redis store to know if we should handle its shutdown.

[euri10]

sounds good to me, I'm keen on tackling both, 1st one will be simpler and faster obviously

[provinzkraut]

I've assigned you here. I'll leave it up to you if you want to address both in a single PR or two separate ones, both are fine with me.

[github-actions]

A fix for this issue has been released in v2.6.2