encode / starlette

The little ASGI framework that shines. 🌟
https://www.starlette.io/
BSD 3-Clause "New" or "Revised" License
10.31k stars 948 forks source link

Modify application_state at _raise_on_disconnect #2751

Closed osangu closed 2 weeks ago

osangu commented 2 weeks ago

Summary

In my opinion, even when the websocket is closed by the client, the state should be changed to DISCONNECTED. This way, it can be handled more easily by users.

Checklist

Kludex commented 2 weeks ago

But we do have client_state...

Kludex commented 2 weeks ago

Also, why are you using those states in your application?

osangu commented 2 weeks ago

Oh.. I didn't know about client_state, so using like below code . I apologize for making PR with light knowledge and finally waste your time.

Once again, I'm really sorry.

@socket_router.websocket('/ws')
async def knowledge_chat(
        websocket: WebSocket
):
    already_disconnected = False
    try:
        await service(websocket)

    except fe.WebSocketDisconnect:
        already_disconnected = True

    finally:
        still_connected = websocket.application_state == WebSocketState.CONNECTED

        if not already_disconnected and still_connected:
            await fe_websocket.close()
Kludex commented 2 weeks ago

I don't recommend using those states. You should rely on the WebSocketDisconnect.

osangu commented 2 weeks ago

May I kindly ask if the reason you recommend manage through exception handling is to enable immediate processing of the connection? If that’s not the case, I would be truly grateful if you could explain the other reasons.

Kludex commented 2 weeks ago

The reason is that application_state and client_state are meant to be private, even if there's no underscore as prefix. 😅

osangu commented 2 weeks ago

I understood! Really appreciate for you kind comment and sorry again : ) Have a nice day!!

Kludex commented 2 weeks ago

No need to apologize, all good. :)

Thanks for closing the PR.