Bug: declaring a RedisStore in the app triggers a 500 error in the 2nd set of parameters of a parametrized test about litestar HOT 13 CLOSED

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.

from litestar.

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.

from litestar.

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

from litestar.

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

from litestar.

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).

from litestar.

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.

from litestar.

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

from litestar.

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...

from litestar.

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

from litestar.

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.

from litestar.

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

from litestar.

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.

from litestar.

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

from litestar.