Open invisibleroads opened 2 months ago
We need a test for this.
Also, I think we can check if the transport is closing instead: if self.transport.is_closing()
.
@invisibleroads Are you still interested in working on this?
Hi Marcelo, I was travelling. Let me look at the test.
Would you prefer the is closing check? I feel the try catch would be safer because it is more general.
On Wed, Apr 24, 2024, 6:39 AM Marcelo Trylesinski @.***> wrote:
@invisibleroads https://github.com/invisibleroads Are you still interested in working on this?
— Reply to this email directly, view it on GitHub https://github.com/encode/uvicorn/pull/2312#issuecomment-2074647642, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACBDLAPIHWKGEFZX4B2SWDY66DWDAVCNFSM6AAAAABGQ6JJDCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANZUGY2DONRUGI . You are receiving this because you were mentioned.Message ID: @.***>
Hi Marcelo, I was travelling. Let me look at the test. Would you prefer the is closing check? I feel the try catch would be safer because it is more general. … On Wed, Apr 24, 2024, 6:39 AM Marcelo Trylesinski @.> wrote: @invisibleroads https://github.com/invisibleroads Are you still interested in working on this? — Reply to this email directly, view it on GitHub <#2312 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACBDLAPIHWKGEFZX4B2SWDY66DWDAVCNFSM6AAAAABGQ6JJDCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANZUGY2DONRUGI . You are receiving this because you were mentioned.Message ID: @.>
Yes, that's what I said on my last message.
We also need a test here.
@Kludex I tried to recreate the situation where the transport is closed when the server tries to send the 500 error, but I can't get the situation to recreate exactly and don't think mocking would work either.
async def test_asgi_shutdown_before_handshake(ws_protocol_cls: WSProtocol, http_protocol_cls: HTTPProtocol, unused_tcp_port: int):
"""
"""
async def app(scope: Scope, receive: ASGIReceiveCallable, send: ASGISendCallable):
# await asyncio.sleep(2)
# server = d['server']
list(server.server_state.connections)[0].transport.close()
# server.should_exit = True
# server.force_exit = True
await asyncio.sleep(2)
config = Config(
app=app,
ws=ws_protocol_cls,
http=http_protocol_cls,
lifespan="off",
port=unused_tcp_port,
)
async with run_server(config) as server:
async with websockets.client.connect(f"ws://127.0.0.1:{unused_tcp_port}"):
pass
# try:
# async with websockets.client.connect(f"ws://127.0.0.1:{unused_tcp_port}", open_timeout=1):
# pass
# except:
# pass
# await asyncio.sleep(3)
INFO: Started server process [37557]
INFO: Uvicorn running on http://127.0.0.1:55877 (Press CTRL+C to quit)
INFO: Shutting down
INFO: Waiting for background tasks to complete. (CTRL+C to force quit)
ERROR: ASGI callable returned without sending handshake.
INFO: connection closed
--------------------------------------------------------------- Captured log call ----------------------------------------------------------------
INFO uvicorn.error:server.py:82 Started server process [37557]
INFO uvicorn.error:server.py:214 Uvicorn running on http://127.0.0.1:55877 (Press CTRL+C to quit)
INFO uvicorn.error:server.py:258 Shutting down
INFO uvicorn.error:server.py:303 Waiting for background tasks to complete. (CTRL+C to force quit)
ERROR uvicorn.error:websockets_impl.py:257 ASGI callable returned without sending handshake.
INFO uvicorn.error:server.py:264 connection closed
Whatever I do, the connection keeps closing only after sending the 500 error in the test.
Updated the commit to check if transport.is_closing
We can talk about this at the PyCon sprints on Monday if you're going to be there.
I'll be there on the 16 👀
More fixes are needed... The connection_lost
on WS implementation has a logical issue.
I need to fix this PR, sorry.
You know more about the edge cases than I do. I wish I could help more here, but you are welcome to modify the pull request as much as is needed
Please see the relevant "discussion" https://github.com/encode/uvicorn/discussions/2311