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]
