bokeh / bokeh-fastapi

BSD 3-Clause "New" or "Revised" License
9 stars 2 forks source link

Websocket state handling #19

Open tbronson opened 1 month ago

tbronson commented 1 month ago

Handling of detecting when the websockets close is bugged at the moment, sessions are never cleaned up because of it.

Here are a couple patches that fix the issue:

handler.py

...
async def _receive_loop(self) -> None:
        while True:
            try:
                ws_msg = await self._socket.receive()
                self._socket._raise_on_disconnect(ws_msg)
            except WebSocketDisconnect as e:
                log.info(
                    "WebSocket connection closed: code=%s, reason=%r", e.code, e.reason
                )
                self.application.client_lost(self.connection)
...

receive does not raise when the socket is closed normally, but starlette has a private helper function to do so

...
async def send_text(self, text: str) -> None:
        try:
            await self._socket.send_text(text)
        except WebSocketDisconnect as e:
            self.on_close(e.code, e.reason)

    async def send_bytes(self, bytestream: bytes) -> None:
        try:
            await self._socket.send_bytes(bytestream)
        except WebSocketDisconnect as e:
            self.on_close(e.code, e.reason)
...

send_text and send_bytes do raise, they just needed to be put in a try block

pmeier commented 1 month ago

Thanks for the report @tbronson. Could you when exactly this happens? If a client drops the connection without properly closing it?

tbronson commented 1 month ago

Yes, specifically I found this while testing Panel 1.5.0rc2 on Windows 10.

Launch any panel app with 'admin=True', open the admin panel in a tab, then close the tab and check server console. It will be spammed with unhandled write exceptions to closed websocket (from periodic_callbacks) and the sessions will never be cleaned up either.

tbronson commented 1 month ago

I should clarify:

The websocket.disconnect message does come in, it's just not handled completely, hence the _raise_on_disconnect call