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

Modify application_state at _raise_on_disconnect #2751

Closed
wants to merge 1 commit into from

Conversation

osangu
Copy link

@osangu osangu commented Nov 14, 2024

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

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.

@Kludex
Copy link
Member

Kludex commented Nov 14, 2024

But we do have client_state...

@Kludex
Copy link
Member

Kludex commented Nov 14, 2024

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

@osangu
Copy link
Author

osangu commented Nov 14, 2024

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()

@osangu osangu closed this Nov 14, 2024
@Kludex
Copy link
Member

Kludex commented Nov 14, 2024

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

@osangu
Copy link
Author

osangu commented Nov 14, 2024

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
Copy link
Member

Kludex commented Nov 14, 2024

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

@osangu
Copy link
Author

osangu commented Nov 14, 2024

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

@Kludex
Copy link
Member

Kludex commented Nov 14, 2024

No need to apologize, all good. :)

Thanks for closing the PR.

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.

2 participants