Hi Radu,
there is a difference between flush and close. If some downstream code
is flushing then the dispatcher can still call flushBuffer. But if some
downstream code is closing, then flushing will of course not work. To be
it sounds like there is some code illegally closing the response. I
think that should be fixed instead. I remember that we needed to fix
this in other servlets using johnzon as well and avoid that the json
writer closes the output stream/writer.
flushBuffer is called to commit the response, which I think was required
to be complient with how forward works. We can of course as a sanity
check call response.isCommitted() but does this help here?
Regards
Carsten
Am 14.06.2022 um 16:39 schrieb Radu Cotescu:
Hi Carsten,
our own RequestDispatcher calls response.flushBuffer() at the end of the
forward method [1] without checking first if the response has already been
committed. By this time, the response can be already committed, since dispatch
is called before this flushBufer() call [2]. The
SlingHttpServletResponseImpl#flushBuffer can end up calling PrintWriter#flush
[3], which in case the writer was already closed (happens for example if you
wrap the writer with the JsonWriter from Johnzon, which is auto-closable), will
end up throwing the exception with the message provided by Bertrand if the
initial request was forwarded.
Is there any reason to call flushBuffer in the RequestDispatcher#forward? If
yes, can’t we first check if the response was committed first
(response.isComitted())?
Thanks,
Radu
[2] -
https://github.com/apache/sling-org-apache-sling-engine/blob/8580cf1d5862f2a3b2cbe01c929c4c43cb93d773/src/main/java/org/apache/sling/engine/impl/request/SlingRequestDispatcher.java#L146
<https://github.com/apache/sling-org-apache-sling-engine/blob/8580cf1d5862f2a3b2cbe01c929c4c43cb93d773/src/main/java/org/apache/sling/engine/impl/request/SlingRequestDispatcher.java#L146>
[3] -
https://github.com/apache/sling-org-apache-sling-engine/blob/4c7e4149e15567015b1e586b6df617a9af16a6de/src/main/java/org/apache/sling/engine/impl/SlingHttpServletResponseImpl.java#L227
<https://github.com/apache/sling-org-apache-sling-engine/blob/4c7e4149e15567015b1e586b6df617a9af16a6de/src/main/java/org/apache/sling/engine/impl/SlingHttpServletResponseImpl.java#L227>
On 5 Jun 2022, at 11:53, Carsten Ziegeler <[email protected]> wrote:
In general, I think several calls to flushBuffer are totally valid, so even if
anyone else called this already in a inner call from forward, this shouldn't be
a problem. A rather suspect that the problem is more with duplicate tries to
set the headers or something along those lines.
So I think we should figure out what really is going on before making any
changes. It seems there is a scenario which is causing this, so it would be
good to convert that into a test.
Regards
Carsten
Am 03.06.2022 um 18:18 schrieb Bertrand Delacretaz:
Hi,
A colleague mentioned getting "SlingRequestProcessorImpl Writer has
already been closed" error messages when using
SlingRequestDispatcher.forward(...), and looking at the code [1] I
wonder if we should change the request flushing behavior.
Unfortunately, unless I missed something that code is not covered by
units tests, but I suppose nested calls to
SlingRequestDispatcher.forward(...) would cause the problem:
SlingRequestDispatcher blindly calls response.flushBuffer(), which
causes logged errors if the response Writer is already closed. So if a
Servlet that's been forwarded to calls
SlingRequestDispatcher.forward(...) I suppose we get this problem.
I could write a test to verify all that of course, but maybe someone
already has a clear idea on this.
Shall we modify that code to check the request status before calling
flushBuffer() ? I don't think there's a direct way to check if the
Writer is closed already, but maybe there's another call on the
request that would check that?
-Bertrand
[1]
https://github.com/apache/sling-org-apache-sling-engine/blob/631a54f45abf5fd6d7c56dac43fd499db543bcd7/src/main/java/org/apache/sling/engine/impl/request/SlingRequestDispatcher.java#L151
--
Carsten Ziegeler
Adobe
[email protected]
--
Carsten Ziegeler
Adobe
[email protected]
