falconry / falcon

The no-magic web data plane API and microservices framework for Python developers, with a focus on reliability, correctness, and performance at scale.
https://falcon.readthedocs.io/en/stable/
Apache License 2.0
9.52k stars 945 forks source link

ASGI WebSocket protocol 2.4: `send()` may raise `OSError` #2292

Closed vytas7 closed 1 month ago

vytas7 commented 2 months ago

In the ASGI HTTP & WebSocket protocol version 2.4, calling send() on a closed connection should raise an OSError.

See also Disconnected Client - send exception:

If send() is called on a closed connection the server should raise a server-specific subclass of IOError. This is not guaranteed, however, especially on older ASGI server implementations (it was introduced in spec version 2.4).

Applications may catch this exception and do cleanup work before re-raising it or returning with no exception.

Servers must be prepared to catch this exception if they raised it and should not log it as an error in their server logs.

Do we need to adjust our code? Or do we just bubble this up to the app server?

Note: this issue is aiming to address this only for WebSocket. The same issue in the usual HTTP protocol is tracked as #2323.

See also:

CaselIT commented 2 months ago

I agree that the primary impact is likely the websocket, since in the other cases the sent is confined to the app class, allowing the orerror to be propagated without much issues to the api server.

For websocket we likely need to close it if send raises an os error

vytas7 commented 2 months ago

I don't think we need to close the websocket at hand, because closing is performed via send() by sending an appropriate ASGI event (which is moot since it is exactly what raises OSError).

However, we'll need to make sure that we properly reraise this exception as WebSocketDisconnected. (Probably we could even use the raise WebSocketDisconnected(...) from io_ex form? :thinking:)

CaselIT commented 2 months ago

my closing I meant cleaning up the internal state, like the _BufferedReceiver. regarding the re-raise, seems like an appropriate behaviour

vytas7 commented 2 months ago

The last paragraph of Lost Connections also needs to be reworded:

Note also that some ASGI server implementations do not strictly follow the ASGI spec in this regard, and in fact will raise an error when the app attempts to send a message after the client disconnects. If testing reveals this to be the case for your ASGI server of choice, Falcon’s own receive queue can be safely disabled.

(emphasis mine)

:arrow_right: Conversely, these implementations do actually follow the spec version 2.4.

vytas7 commented 2 months ago

I split the HTTP part to another issue (#2323), and address only the WebSocket part for 4.0.