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

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

Closed
vytas7 opened this issue Aug 23, 2024 · 5 comments · Fixed by #2324
Closed

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

vytas7 opened this issue Aug 23, 2024 · 5 comments · Fixed by #2324

Comments

@vytas7
Copy link
Member

vytas7 commented Aug 23, 2024

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:

@vytas7 vytas7 added this to the Version 4.0 milestone Aug 23, 2024
@vytas7 vytas7 added the needs contributor Comment on this issue if you'd like to volunteer to work on this. Thanks! label Aug 23, 2024
@vytas7 vytas7 changed the title ASGI HTTP protocol 2.4: calling send() may raise OSError ASGI HTTP protocol 2.4: send() may raise OSError Aug 23, 2024
@CaselIT
Copy link
Member

CaselIT commented Aug 23, 2024

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

vytas7 commented Aug 23, 2024

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? 🤔)

@CaselIT
Copy link
Member

CaselIT commented Aug 23, 2024

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

@vytas7
Copy link
Member Author

vytas7 commented Aug 27, 2024

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)

➡️ Conversely, these implementations do actually follow the spec version 2.4.

@vytas7 vytas7 changed the title ASGI HTTP protocol 2.4: send() may raise OSError ASGI WebSocket protocol 2.4: send() may raise OSError Sep 6, 2024
@vytas7
Copy link
Member Author

vytas7 commented Sep 6, 2024

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

@vytas7 vytas7 removed the needs contributor Comment on this issue if you'd like to volunteer to work on this. Thanks! label Sep 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants