Response.getWriter(), getOutputStream(), setContentLength() spec issues

[Mark Thomas <ma...@apache.org>]

The changes [1] for bug 50748 [2] (be aware the bug number changed) have
triggered some Servlet 3.0 TCK failures. I don't want to get into the
details of those tests, but I do want to discuss the root cause.

Consider the following scenario:
Servlet knows it will return exactly 100 bytes of content
Servlet calls setContentLength()
Servlet calls getOutputStream() or getWriter()
Servlet writes 100 bytes

In this case everything works as expected. Now add a filter to the mix
that adds additional content to the response

Filter calls getOutputStream() or getWriter()
Filter writes 20 bytes to the response
Servlet knows it will return exactly 100 bytes of content
Servlet calls setContentLength()
Servlet calls getOutputStream() or getWriter()
Servlet writes 100 bytes

There are a couple of potential issues in the second scenario:
- If the Filter called getOutputStream(), the Servlet must also call
getOutputStream()
- If the Filter called getWriter(), the Servlet must also call getWriter()
- The response contains 120 bytes but the content length header says 100
so the response is truncated by 20 bytes.

This final point is (essentially) what is causing the TCK failures. The
Servlet in question is the default Servlet.

Prior to the commit for bug 50748, scenario 2 above would have worked if
getWriter() had been used but would have failed if getOutputStream() has
been used.

One of the things I like about the fix for bug 50748 is that the
behaviour is now consistent regardless of whether getOutputStream() or
getWriter() is called.

I couldn't find much in the spec about what is correct in this case
although the spec does seem to imply that if a filter is going to modify
the response, it should wrap the response.

So, the questions we need to decide:

1. Is the fix for bug 50748 correct? I think it is.

2. Should Tomcat try and handle this situation (e.g. if any bytes have
been written by a filter, commit the response). This could be tricky to
get right when forwards are involved and the response is meant to have
been cleared. I'm leaning towards not doing this.

3. Is any filter that writes to the response without wrapping it so any
calls to setContentLength() can be intercepted and adjusted as necessary
broken? I think it is.

Thoughts? I'd like to reach some conclusions fairly speedily as this
might hold up a 7.0.9 release. If we conclude that the filter is at
fault in point 3, we'll need to raise a TCK challenge.

Mark

[1] http://svn.apache.org/viewvc?rev=1069131&view=rev
[2] http://issues.apache.org/bugzilla/show_bug.cgi?id=50748

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org

Re: Response.getWriter(), getOutputStream(), setContentLength() spec issues

[Konstantin Kolinko <kn...@gmail.com>]

2011/2/25 Mark Thomas <ma...@apache.org>:
> The changes [1] for bug 50748 [2] (be aware the bug number changed) have
> triggered some Servlet 3.0 TCK failures. I don't want to get into the
> details of those tests, but I do want to discuss the root cause.
>
> Consider the following scenario:
> Servlet knows it will return exactly 100 bytes of content
> Servlet calls setContentLength()
> Servlet calls getOutputStream() or getWriter()
> Servlet writes 100 bytes
>
> In this case everything works as expected. Now add a filter to the mix
> that adds additional content to the response
>
> Filter calls getOutputStream() or getWriter()
> Filter writes 20 bytes to the response
> Servlet knows it will return exactly 100 bytes of content
> Servlet calls setContentLength()
> Servlet calls getOutputStream() or getWriter()
> Servlet writes 100 bytes
>
> (...)
> The Servlet in question is the default Servlet.
>
>(..)
>
> So, the questions we need to decide:
>
> 1. Is the fix for bug 50748 correct? I think it is.

+1

> 2. Should Tomcat try and handle this situation (e.g. if any bytes have
> been written by a filter, commit the response).

-1.  The filter could call flush() by itself. Though I think relying
on flush() so that setContentLength were ignored is a weak workaround.

> 3. Is any filter that writes to the response without wrapping it so any
> calls to setContentLength() can be intercepted and adjusted as necessary
> broken? I think it is.


I think that any filter that prepends or appends data to a static file
(e.g. to add a license, an add, a message --  there are certainly are
sites that append their adds) must either

a) take care of
- setContentLength(),
- setHeader("content-length"), setIntHeader("content-length")
calls

b) use context.getRequestDispatcher(...).include(..)

I agree that if the filter does not do either a) or b) then the filter
is broken.


I have not checked yet whet HTTP spec says about content-length header
by cllients.  I wonder what happens when content generation with a
content-length header fails in the middle. (and e.g. the
ErrorReportValve appends additional content).    The enforcement of
the response length I think is there because otherwise it can break
clients (or proxies) and thus is legit.

Best regards,
Konstantin Kolinko

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org

Re: Response.getWriter(), getOutputStream(), setContentLength() spec issues

[Rainer Jung <ra...@kippdata.de>]

On 25.02.2011 13:01, Mark Thomas wrote:
> So, the questions we need to decide:
>
> 1. Is the fix for bug 50748 correct? I think it is.

+0

> 2. Should Tomcat try and handle this situation (e.g. if any bytes have
> been written by a filter, commit the response). This could be tricky to
> get right when forwards are involved and the response is meant to have
> been cleared. I'm leaning towards not doing this.

TC should not try to handle it.

> 3. Is any filter that writes to the response without wrapping it so any
> calls to setContentLength() can be intercepted and adjusted as necessary
> broken? I think it is.

It is.

Rainer

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org

Re: Response.getWriter(), getOutputStream(), setContentLength() spec issues

[Mark Thomas <ma...@apache.org>]

On 25/02/2011 19:38, Christopher Schultz wrote:
> Where does this fail the TCK? Do we have a broken Filter, or does the
> TCK test an unusual/stupid scenario?

I am afraid I can't be more specific about this, the TCK is protected by
an NDA. For that reason, I deliberately simplified my example to address
the root cause rather than what the TCK is actually testing. Specific
TCK discussions are held on the private mailing list.

BTW, as a committer if you want access to the TCK, you just need to sign
and submit the NDA.

>> Thoughts? I'd like to reach some conclusions fairly speedily as this
>> might hold up a 7.0.9 release. If we conclude that the filter is at
>> fault in point 3, we'll need to raise a TCK challenge.
> 
> I guess that answers my question.
> 
> We could have a "just-pass-the-damned-TCK" configuration flag (or maybe
> something less inflammatory) that would change this behavior to
> commit-on-write for Filters.

We could, but if the consensus is that the TCK is wrong (and so far it
seems to be ) I'd be happy releasing Tomcat with a "we pass the TCK
apart from an undisclosed number of tests we are currently challenging".

Mark

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org

Re: Response.getWriter(), getOutputStream(), setContentLength() spec issues

[Christopher Schultz <ch...@christopherschultz.net>]

Mark,

On 2/25/2011 7:01 AM, Mark Thomas wrote:
> So, the questions we need to decide:
> 
> 1. Is the fix for bug 50748 correct? I think it is.

+0

> 2. Should Tomcat try and handle this situation (e.g. if any bytes have
> been written by a filter, commit the response). This could be tricky to
> get right when forwards are involved and the response is meant to have
> been cleared. I'm leaning towards not doing this.

The Filter definitely needs to be able to set headers and append content
to the response entity. I'm not sure about a Filter having a legitimate
reason to write content /before/ the servlet does to the response
entity... I'm having trouble thinking of a use case for that. Given
that, having the Filter write anything to the response stream/writer
seems reasonable to commit the response.

On the other hand, why should a filter act any differently than a
servlet? What I mean is that the response won't be committed when a
servlet writes to the response until the response buffer has been
filled, or an explicit flush() has been called. Also, a servlet should
have the opportunity to reset the response if possible given the commit
state.

I think that makes me -1 on this one.

> 3. Is any filter that writes to the response without wrapping it so any
> calls to setContentLength() can be intercepted and adjusted as necessary
> broken? I think it is.

+1

Where does this fail the TCK? Do we have a broken Filter, or does the
TCK test an unusual/stupid scenario?

> Thoughts? I'd like to reach some conclusions fairly speedily as this
> might hold up a 7.0.9 release. If we conclude that the filter is at
> fault in point 3, we'll need to raise a TCK challenge.

I guess that answers my question.

We could have a "just-pass-the-damned-TCK" configuration flag (or maybe
something less inflammatory) that would change this behavior to
commit-on-write for Filters.

-chris