Deal gently with ConnectionClosedError fron websockets #762
Conversation
…vicorn into stefanw-server-lost-connection
|
|
||
| try: | ||
| await self.send(data) | ||
| except ConnectionClosedError as exc: |
There was a problem hiding this comment.
Catching the exception is certainly one way to deal with this. Just want to mention that the wsproto implementation checks for self.transport.is_closing():
https://github.com/encode/uvicorn/blob/master/uvicorn/protocols/websockets/wsproto_impl.py#L278
Maybe this is more agnostic to the underlying library. On the other hand it's circumventing the library and checks transport conditions directly. Unsure.
There was a problem hiding this comment.
yes, I dont like it too, I didn't see something satisfying enough from the faq here too.
https://websockets.readthedocs.io/en/stable/faq.html#what-does-connectionclosederror-code-1006-mean
|
|
||
| # Simulate a lost connection | ||
| # without receiving a close frame | ||
| self.send.__self__.connection_lost(None) |
There was a problem hiding this comment.
I'm still wondering if there's a better way to access the underlying protocol here.
There was a problem hiding this comment.
yes, you're right.
add to that the fact that once we call the protocol here, right after in the asgi_send mehtod at elif not self.closed_event.is_set(): we have that event not set and the State that is State.CLOSED which is dubious
There was a problem hiding this comment.
I used connection_lost because it was used in test_early_disconnect in the http tests. It's the method called on protocol from transport.close(), so seems appropriate. I was just wondering if there's a nicer way to access it instead of going through the method's __self__.
based on test nicely provided in #758
using pytest-reraise to avoid the bug shown in #759
fix #757
Would really appreciate some feedback on this, not sure I got this right