Closed flodiebold closed 5 months ago
Thanks for the report. This is related to an issue brought up during Ring 2 development, around how to handle exceptions from a respond
. The conclusion was that respond
should never throw - any exception should be caught and raised instead. So in Compojure-API, the try/catch block should be around coerce-response!
but not around respond
.
In terms of Ring, the EOF execption should be raised so that user code can react to it in some way, but after the error is raised, subsequent calls to respond
should, I think, be ignored. This requires some fixes to the Jetty adapter, in terms of catching the error, raising it correctly, and then ensuring that respond
is rendered a no-op for that request cycle.
Regarding the respond
function. I find the rules for a continuation from leonoel/task suitable:
[...] Its return value should be ignored, it must not throw and must not block the calling thread. [...]
You say:
In terms of Ring, the EOF execption should be raised so that user code can react to it in some way [...]
How do you expect to achieve that? The EOF Exception will be thrown inside the respond
function an adapter will use to call the handler. If that respond
function doesn't throw the exception, as the rule would require, I don't see how the user code could handle it.
In essence, the handler code hands over the response to the respond
function and never looks back, as it ignores the return value, don't expect the respond
function to throw and even should expect the respond
function to not block. Especially if the respond
function doesn't block on I/O, the handler function will return before the response is written to the client.
So, IMHO any exception that will be caught during writing of the StreamableResponseBody
has to somehow be handled by the adapter itself.
How do you expect to achieve that? The EOF Exception will be thrown inside the respond function an adapter will use to call the handler.
As I said in my previous comment:
The conclusion was that
respond
should never throw - any exception should be caught and raised instead.
This can be made more streamlined with the ring.util.async/raising function in the 2.0 branch.
As I said in my previous comment:
The conclusion was that
respond
should never throw - any exception should be caught and raised instead.This can be made more streamlined with the ring.util.async/raising function in the 2.0 branch.
If I understand you correctly, the implementation of the respond
method from the adapter should raise exceptions instead of throwing them. But then, how does the adapter become access the the raise
function? It will have access to it's own raise function only, which isn't a user provided raise function. In case the handler should be able to intercept the raising, the respond function would need a second argument, a raise
function.
IMHO it would be sufficient to just terminate the request handling gracefully, in case an EOF happens while writing the response. First the client doesn't receive data anymore and second, the handler can never assume that the client receives the response anyhow. So I see no need to inform the handler here.
The exception would be raised when attempting to write to the output stream, so if a developer wanted to handle it themselves, they could:
(defn handler [request respond raise]
(respond
{:status 200
:headers {}
:body (reify StreamableResponseBody
(-write-body-to-stream [_ _ output-stream]
(try
(with-open [writer (io/writer output-stream)]
(.write writer "Hello World"))
(catch EofException ex
(raise ex)))))})
But by default, yes, it'll just swallow the exception and move on.
In the example you show above, I don't think that raising the exception will work. The default raise of an adapter would try to send an error after the response was started already. An error handling middleware above would probably try to use respond a second time in order to send an error response.
An error handling middleware above would probably try to use respond a second time in order to send an error response.
Right, and as I said, subsequent responds would be ignored. If the stream is closed you can't respond to the client, you can only log and carry on (or pass a message via another connection).
This is an issue where I'm not 100% sure what part of the stack is really responsible for the problem, so I'll just start here.
We have an API using compojure-api, which is using async but in some cases returning an
InputStream
as body. If a caller requests that resource, but then closes the connection before the body has finished being written, it results in multiple errors being logged, which we would like to get rid of. So far as I can reconstruct, the following happens:StreamableResponseBody
implementation tries to write to the connection (here) and gets anorg.eclipse.jetty.io.EofException
.respond
calls, until finally being caught by this try/catch block in compojure-api, which callsraise
with it.raise
call finally reaches compojure-api's wrap-exceptions middleware, which tries torespond
with an error response.IllegalStateException: AsyncContext completed and/or Request lifecycle recycled
from Jetty.StreamEncoder
to write to it, which causes a secondEofException
.So... something is handling the exceptions wrongly here, but I'm not sure whether it's a) that middlewares should never call
raise
afterrespond
, or b) the servlet adapter code (or maybe the jetty adapter?) should catch theEofException
, or something completely different. If you think this is a bug in compojure-api, I'll reopen the ticket there of course, but it made more sense to me to open a ticket here first, because it seems to me part of the problem is that the contract for async handlers is a bit unclear (is callingraise
after having already calledrespond
allowed?).