emmett-framework
Feature request: raise an exception on send() if client is already disconnected
Currently, the response is simply "swallowed" if the client is disconnected in the meantime. It would be nice to let the app be aware of such cases and raise an exception in the send() method: https://asgi.readthedocs.io/en/latest/specs/www.html#disconnected-client-send-exception.
Granian logs a message [INFO] ASGI transport error: SendError { .. } on every failed send, and the log becomes cluttered, while we don't even know which request (and client/ip) is causing this. This is especially annoying when the response has lots of chunks, i.e. send() is called thousands of times.
Use case: both malicious and erroneous disconnections can occur, and this hurts the app if requests are either expensive or take some time to finish. If the app is aware, it will have a chance to at least log such issues, allowing for subsequent analysis to take appropriate action, or to stop sending a long response body if the client suddenly disconnects.
PS: Ideally, of course, it would be nice to somehow signal the app that the client is disconnected, giving it a chance to cancel an active request. Unfortunately, this is beyond the current specifications...
It would be nice to let the app be aware of such cases and raise an exception in the
send()method: https://asgi.readthedocs.io/en/latest/specs/www.html#disconnected-client-send-exception.
This would require to move from spec 2.3 to spec 2.4 (https://asgi.readthedocs.io/en/latest/specs/www.html#spec-versions), which is sort of a double-edged sword, as not many ASGI frameworks/apps out there support spec > 2.3. Thus, I'd say that requires some dedicated discussion, effectively valuating all pros and cons of that choice.
This is especially annoying when the response has lots of chunks, i.e.
send()is called thousands of times. Ideally, of course, it would be nice to somehow signal the app that the client is disconnected, giving it a chance to cancel an active request. Unfortunately, this is beyond the current specifications...
No it's not. A disconnect event (https://asgi.readthedocs.io/en/latest/specs/www.html#disconnect-receive-event) is sent in ASGI protocol following the spec since Granian version 2.1.0 (issue #286 closed in #524). Thus, in my perspective, if the app is calling send a thousand times after the client disconnected, is an app/framework issue not following the spec and reacting to the disconnect, rather than a Granian issue.
The feature could be conditional - this is the only change in 2.4, and since Granian is already aware of disconnection, raising an exception should not be a big deal (I hope).
The disconnect event requires a separate task which is permanently listening/running even after the body is received, and it should be running until the response is completely sent. This task has to be created explicitly every time when http.request event is received.
This also requires a bit of work to handle it correctly in two cases - when the body is processed immediately or when it is streamed.
Obviously, all this is doable - but it looks and feels a bit unclean and adds processing overhead, while exception on send() significantly simplifies things - I guess this is actually the reason why it was added in 2.4
Just tested - processing with listener for disconnect event and without it - the mere existence of this task (plus reading the disconnect event) reduces RPS by 35% (!) - from 21K on average to 15.5K, and with explicit cancellation of this task (when the body is completely sent) RPS is reduced by 20% (to 17.5K).
This probably won't matter much for relatively light loads - but may become noticeable in high-load cases.