fix: close RedisStore client in case the with_client classmethod is used

[euri10]

Description

WIP, it doesn't pass the test I added from the mvce in the issue (this one could certainly be simplified) so my impleentation has obviously an issue.
I made the RedisStore an async context manager, I can see the _on_shutdon method being hit, but the _redis client is still not closed or it doesnt exists.

from your comment here:
https://discord.com/channels/919193495116337154/1152626430295933060/1207016578823102565

Closes

Fixes #3083

[euri10]

you're reviewing / holding my hand faster than I can type :)
but there's progress, the test I wrote from the mcve passes,
I'll check later on why it broke 46 existing ones 👍

[provinzkraut]

you're reviewing / holding mye hand faster than I can type :)

Sorry 😬

[euri10]

you're reviewing / holding mye hand faster than I can type :)

Sorry 😬

no worries that was fun, I almost wondered if you were not a LLM at sone point 🤣

[provinzkraut]

no worries that was fun, I almost wondered if you were not a LLM at sone point 🤣

I do wonder that too at times. Aren't we all just fancy LLMs at some level? 👀

[euri10]

no worries that was fun, I almost wondered if you were not a LLM at sone point 🤣

I do wonder that too at times. Aren't we all just fancy LLMs at some level? 👀

that kind of would be Nick Bolstrom's theory in "Are you living in a computer simulation?"

[euri10]

I might have introduced some flakiness :)
Will take a look tomorrow

[euri10]

I might have introduced some flakiness :) Will take a look tomorrow

ok in fact it's just the test I added doesnt integrate well in the suite, need to find a way to see how to keep it without issues

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.26%. Comparing base (1ff7f1e) to head (aac3fb5).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3111   +/-   ##
=======================================
  Coverage   98.26%   98.26%           
=======================================
  Files         320      320           
  Lines       14405    14417   +12     
  Branches     2317     2319    +2     
=======================================
+ Hits        14155    14167   +12     
  Misses        109      109           
  Partials      141      141           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[euri10]

ok I removed my initial test from the mcve that passes with the change made to the lifespan, reason is twofold.

one, as you can notice in the suite, all instances where with_client is used are patched, so if there is a test to be made, one has to follow the same rule or we may end up having unclosed ressources: I therefore think this test is problematic:

litestar/tests/unit/test_stores.py

Lines 242 to 245 in 323e23e

	@pytest.mark.xdist_group("redis")
	def test_redis_namespace_explicit_none(redis_client: Redis) -> None:
	assert RedisStore.with_client(url="redis://127.0.0.1", namespace=None).namespace is None
	assert RedisStore(redis=redis_client, namespace=None).namespace is None

because it doesnt patch the with_client and will leave things unclosed, the other line is fine

Also, I modified the redis_client fixture: https://github.com/litestar-org/litestar/pull/3111/files#diff-e52e4ddd58b7ef887ab03c04116e676f6280b824ab7469d5d3080e5cba4f2128L307-R315

the previous one is problematic because it uses a sync redis instance to flushall and never closes it, one can just use the async redis instance from the fixture and flushall in the finally for the same effect without the unintended consequences,

wdyt ?

[provinzkraut]

the previous one is problematic because it uses a sync redis instance to flushall and never closes it, one can just use the async redis instance from the fixture and flushall in the finally for the same effect without the unintended consequences,

wdyt ?

I think the proper fix would be to close the sync Redis instance. The reason we flush at the beginning and not the end is that if we're flushing at the end, and something goes wrong, we'd be affecting other tests. If we flush at the beginning, every test starts with an empty DB.

Also, I'm surprised that calling .flush() after .close() does not cause any issues.

[euri10]

the previous one is problematic because it uses a sync redis instance to flushall and never closes it, one can just use the async redis instance from the fixture and flushall in the finally for the same effect without the unintended consequences,
wdyt ?

I think the proper fix would be to close the sync Redis instance. The reason we flush at the beginning and not the end is that if we're flushing at the end, and something goes wrong, we'd be affecting other tests. If we flush at the beginning, every test starts with an empty DB.

Also, I'm surprised that calling .flush() after .close() does not cause any issues.

so @provinzkraut I sent a PR fixing the fixture think properly I think here: #3117

questions now on this one:

1. do we deal here with the only test using with_client that doesnt uses mocks.
2. should I add a test checking if the client is closed when using that classmethod, provided there is no is_closed method on the redis client, so I wouldn't know how to test it

[euri10]

I think it should be ready, I'm not pleased with the fixtures as-is to be fair, so I reverted my changes on this part but that shouldn't prevent the PR to be looked at ;)

[github-actions]

Documentation preview will be available shortly at https://litestar-org.github.io/litestar-docs-preview/3111