starlette icon indicating copy to clipboard operation
starlette copied to clipboard

Replace RuntimeError with WebSocketDisconnected

Open ruitcatarino opened this issue 1 year ago • 3 comments

Summary

Change the exception raised if we attempt to use a disconnected WebSocket from RuntimeError to WebSocketDisconnected. Discussed in https://github.com/encode/starlette/discussions/2762 Issue https://github.com/encode/starlette/issues/2766

Checklist

  • [x] I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • [x] 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.

I haven't changed the documentation, if there is a need for it I will gladly change it.

ruitcatarino avatar Nov 25 '24 14:11 ruitcatarino

The change suggested by @ruitcatarino looks meaningful to me. It would allow precise handling of situations like the one mentioned in https://github.com/encode/starlette/discussions/2543.

What stands in the way of accepting this change, can we somehow help?

dym-ok avatar Mar 19 '25 14:03 dym-ok

After the revival of this MR, which, to be honest, I have revisited sporadically in the hopes of getting an update, since my team and I at work are still affected by the original issue that led me to create this pull request, I realized that this newly created exception could also be used in the following cases: raise RuntimeError('WebSocket is not connected. Need to call "accept" first.') → raise WebSocketDisconnected('WebSocket is not connected. Need to call "accept" first.') in the methods receive_bytes, receive_text and receive_json.

ruitcatarino avatar Mar 19 '25 20:03 ruitcatarino

I have added the suggested changes I made in https://github.com/encode/starlette/pull/2767#issuecomment-2738015722 and added more tests. However, despite increasing the number of tests, the pipeline is still failing due to coverage. 🤔 Any ideas?

ruitcatarino avatar Mar 20 '25 09:03 ruitcatarino