Remove pytest asyncio > anyio + fixx some unclosed ressources #3928
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… on a diff param alternative test_cors_simple_response and test_openapi_swagger, all the time its a unclosed socket on redis port 6397
…ems to have a delayed unclosed socket
… tests, very weird, feels like redis-py has issues on closing stuff or we do it wrong
github-actions
bot
added
area/dependencies
euri10
commented
Jan 6, 2025
| @@ -102,7 +102,7 @@ async def on_startup(self) -> None: | |||
| pass | |||
|
|
|||
| async def on_shutdown(self) -> None: | |||
| await self._pub_sub.reset() | |||
| await self._pub_sub.aclose() | |||
| def redis_store(redis_client: AsyncRedis) -> RedisStore: | ||
| return RedisStore(redis=redis_client) | ||
| def redis_store(redis_client: Redis) -> RedisStore: | ||
| return RedisStore(redis=redis_client, handle_client_shutdown=True) |
There was a problem hiding this comment.
we need to explicitely handle shutdown in test or we're left with dangling ressources
Comment on lines
+333
to
+336
| async def redis_client(redis_service: None, docker_ip: str): | ||
| async with Redis(host=docker_ip, port=6397) as client: | ||
| yield client | ||
| await client.flushall() |
There was a problem hiding this comment.
overall this feels more natural and the try / except of the original fixture is just a way to mask unclosed ressources
| @@ -77,7 +77,6 @@ def test_plugin_dependency_signature_namespace(memory_backend: MemoryChannelsBac | |||
| assert app.signature_namespace["ChannelsPlugin"] is ChannelsPlugin | |||
|
|
|||
|
|
|||
| @pytest.mark.flaky(reruns=5) | |||
There was a problem hiding this comment.
I removed all the flaky instances : it will definitely play bad with unclosed ressources and makes it hard to track down things
Comment on lines
+113
to
+171
| # @pytest.mark.parametrize("socket_send_mode", ["text", "binary"]) | ||
| # @pytest.mark.parametrize("handler_base_path", [None, "/ws"]) | ||
| # async def test_create_ws_route_handlers( | ||
| # channels_backend: ChannelsBackend, handler_base_path: str | None, socket_send_mode: WebSocketMode | ||
| # ) -> None: | ||
| # channels_plugin = ChannelsPlugin( | ||
| # backend=channels_backend, | ||
| # create_ws_route_handlers=True, | ||
| # channels=["something"], | ||
| # ws_handler_base_path=handler_base_path or "/", | ||
| # ws_send_mode=socket_send_mode, | ||
| # ) | ||
| # app = Litestar(plugins=[channels_plugin]) | ||
| # | ||
| # with TestClient(app) as client, client.websocket_connect(f"{handler_base_path or ''}/something") as ws: | ||
| # channels_plugin.publish(["foo"], "something") | ||
| # assert ws.receive_json(mode=socket_send_mode, timeout=2) == ["foo"] | ||
| # | ||
| # | ||
| # async def test_ws_route_handlers_receive_arbitrary_message(channels_backend: ChannelsBackend) -> None: | ||
| # """The websocket handlers await `WebSocket.receive()` to detect disconnection and stop the subscription. | ||
| # | ||
| # This test ensures that the subscription is only stopped in the case of receiving a `websocket.disconnect` message. | ||
| # """ | ||
| # channels_plugin = ChannelsPlugin( | ||
| # backend=channels_backend, | ||
| # create_ws_route_handlers=True, | ||
| # channels=["something"], | ||
| # ) | ||
| # app = Litestar(plugins=[channels_plugin]) | ||
| # | ||
| # with TestClient(app) as client, client.websocket_connect("/something") as ws: | ||
| # channels_plugin.publish(["foo"], "something") | ||
| # # send some arbitrary message | ||
| # ws.send("bar") | ||
| # # the subscription should still be alive | ||
| # assert ws.receive_json(timeout=2) == ["foo"] | ||
|
|
||
|
|
||
| # async def test_create_ws_route_handlers_arbitrary_channels_allowed(channels_backend: ChannelsBackend) -> None: | ||
| # channels_plugin = ChannelsPlugin( | ||
| # backend=channels_backend, | ||
| # arbitrary_channels_allowed=True, | ||
| # create_ws_route_handlers=True, | ||
| # ws_handler_base_path="/ws", | ||
| # ) | ||
| # | ||
| # app = Litestar(plugins=[channels_plugin]) | ||
| # | ||
| # with TestClient(app) as client: | ||
| # with client.websocket_connect("/ws/foo") as ws: | ||
| # channels_plugin.publish("something", "foo") | ||
| # assert ws.receive_text(timeout=2) == "something" | ||
| # | ||
| # time.sleep(0.1) | ||
| # | ||
| # with client.websocket_connect("/ws/bar") as ws: | ||
| # channels_plugin.publish("something else", "bar") | ||
| # assert ws.receive_text(timeout=2) == "something else" |
There was a problem hiding this comment.
those 2 tests are the only one where it feels like we have an issue closing things, I didnt manage to understand the issue but uncommenting those 2 and you'll have dangling sockets in subsequent tests
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
wanted to give it a try again with 2fb6f6b done
it fails on 4 tests left as is as they dont impact the rest, and on 3 that are impacting the rest so I commented them, that should be the focus
the 4 tests that have no impact on the rest so I left them failing are:
I'm not too sure about those 2 are failing to be fair
those 2 are failing with RuntimeError('Already running asyncio in this thread') which seems normal to me and it looks like the test itself should probably needs to be rewritten,
The 3 commented which impacts the rest are:
I'm not too sure in that case that we shouldnt handle the Plugin shutdown manually or soething, just a guess.
I'll leave some notes on the changes if someone wanna pick that or try fix the rest, that should be a good start,