You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sling.apache.org by Radu Cotescu <ra...@apache.org> on 2022/06/14 14:39:32 UTC

Sling's RequestDispatcher tries to flush a closed buffer (Was: Re: SlingRequestDispatcher and nested forwards)

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 <cz...@apache.org> 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
> cziegeler@apache.org

Re: Sling's RequestDispatcher tries to flush a closed buffer (Was: Re: SlingRequestDispatcher and nested forwards)

Posted by Carsten Ziegeler <cz...@apache.org>.
The internet is full of discussions whether a servlet should or must not 
call close() and there are more opinions than options ...

I think, a servlet should not close as this can cause upstream problems, 
for example in a filter. And as mentioned I ran into issues with using 
johnzon in a servlet and closing the stream (my app was not a sling app 
though), so I needed to add a wrapper with a nop close method. But on 
the other hand I don't want to get into a big debate about this either.

Let's just add that sanity check and then everyone is happy :)

Regards
Carsten

Am 15.06.2022 um 11:01 schrieb Radu Cotescu:
> This behaviour has been added via https://issues.apache.org/jira/browse/SLING-2724 <https://issues.apache.org/jira/browse/SLING-2724>, but I think that what happens with flush* is too drastic.
> 
>> On 15 Jun 2022, at 10:53, Radu Cotescu <ra...@apache.org> wrote:
>>
>> Sure there is. However, in our case the dispatcher’s [1] and writer’s flush [3] methods are the ones that fail, since they throw an exception if the stream was closed. I don’t think this is correct. Flush should send the content to the client and commit the response, but IMO should not throw if the response has already been committed.
> 
> 

-- 
Carsten Ziegeler
Adobe
cziegeler@apache.org

Re: Sling's RequestDispatcher tries to flush a closed buffer (Was: Re: SlingRequestDispatcher and nested forwards)

Posted by Radu Cotescu <ra...@apache.org>.
This behaviour has been added via https://issues.apache.org/jira/browse/SLING-2724 <https://issues.apache.org/jira/browse/SLING-2724>, but I think that what happens with flush* is too drastic.

> On 15 Jun 2022, at 10:53, Radu Cotescu <ra...@apache.org> wrote:
> 
> Sure there is. However, in our case the dispatcher’s [1] and writer’s flush [3] methods are the ones that fail, since they throw an exception if the stream was closed. I don’t think this is correct. Flush should send the content to the client and commit the response, but IMO should not throw if the response has already been committed.

Re: Sling's RequestDispatcher tries to flush a closed buffer (Was: Re: SlingRequestDispatcher and nested forwards)

Posted by Radu Cotescu <ra...@apache.org>.
Hi Carsten,


> On 14 Jun 2022, at 17:14, Carsten Ziegeler <cz...@apache.org> wrote:
> 
> Hi Radu,
> 
> there is a difference between flush and close. If some downstream code is flushing then the dispatcher can still call flushBuffer.

Sure there is. However, in our case the dispatcher’s [1] and writer’s flush [3] methods are the ones that fail, since they throw an exception if the stream was closed. I don’t think this is correct. Flush should send the content to the client and commit the response, but IMO should not throw if the response has already been committed. The Servlet 3.1 specification [4] also doesn’t say that this method should throw an exception (see section 5.1 of the linked document).

> 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.

I don’t think that servlets should not be allowed to close the response object. While they indeed haven’t opened the stream, the Servlet 3.1 specification [4] mentions this in section “5.7. Lifetime of the Response Object”:

"Each response object is valid only within the scope of a servlet’s service method, or within the scope of a filter’s doFilter method, unless the associated request object has asynchronous processing enabled for the component. If asynchronous processing on the associated request is started, then the request object remains valid until complete method on AsyncContext is called.”

There are other aspects here as well, besides thinking about who is the owner of the stream (the container or the response object): a servlet should be able to close the stream for the simple fact that it owns the response processing. In some cases you might want to make sure that nothing more can be appended to the response. Another aspect could be that the original stream was wrapped (which would be the case with the Johnzon Json Writer): the servlet container would close the wrapped stream, but the wrapper would still stay open in this case.

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

I think it would help. Jetty seems to do the same thing [5].

Thanks,
Radu


[4] - https://download.oracle.com/otn-pub/jcp/servlet-3_1-fr-spec/servlet-3_1-final.pdf <https://download.oracle.com/otn-pub/jcp/servlet-3_1-fr-spec/servlet-3_1-final.pdf>
[5] - https://github.com/eclipse/jetty.project/blob/jetty-11.0.x/jetty-server/src/main/java/org/eclipse/jetty/server/Dispatcher.java#L218 <https://github.com/eclipse/jetty.project/blob/jetty-11.0.x/jetty-server/src/main/java/org/eclipse/jetty/server/Dispatcher.java#L218>

Re: Sling's RequestDispatcher tries to flush a closed buffer (Was: Re: SlingRequestDispatcher and nested forwards)

Posted by Carsten Ziegeler <cz...@apache.org>.
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 <cz...@apache.org> 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
>> cziegeler@apache.org
> 
> 

-- 
Carsten Ziegeler
Adobe
cziegeler@apache.org