Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle case when websocket client disconnects before websocket connection is accepted #2312

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

invisibleroads
Copy link

Please see the relevant "discussion"
#2311

@Kludex
Copy link
Sponsor Member

Kludex commented Apr 22, 2024

We need a test for this.

Also, I think we can check if the transport is closing instead: if self.transport.is_closing().

@Kludex
Copy link
Sponsor Member

Kludex commented Apr 24, 2024

@invisibleroads Are you still interested in working on this?

@invisibleroads
Copy link
Author

invisibleroads commented Apr 24, 2024 via email

@Kludex
Copy link
Sponsor Member

Kludex commented Apr 28, 2024

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.

@invisibleroads
Copy link
Author

invisibleroads commented May 6, 2024

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

@invisibleroads
Copy link
Author

Updated the commit to check if transport.is_closing

@invisibleroads
Copy link
Author

We can talk about this at the PyCon sprints on Monday if you're going to be there.

@Kludex
Copy link
Sponsor Member

Kludex commented May 8, 2024

I'll be there on the 16 👀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants