You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by Konstantin Kolinko <kn...@gmail.com> on 2014/06/02 01:50:25 UTC
Re: svn commit: r1598307 - in /tomcat/trunk/java/org/apache/coyote:
Response.java http11/AbstractOutputBuffer.java
2014-05-29 22:47 GMT+04:00 Mark Thomas <ma...@apache.org>:
> On 29/05/2014 19:36, Mark Thomas wrote:
>> On 29/05/2014 16:27, Konstantin Kolinko wrote:
>>> 2014-05-29 18:39 GMT+04:00 <ma...@apache.org>:
>>>> Author: markt
>>>> Date: Thu May 29 14:39:25 2014
>>>> New Revision: 1598307
>>>>
>>>> URL: http://svn.apache.org/r1598307
>>>> Log:
>>>> Simplify logic
>>>> Call Response.recycle() from Response.reset() rather than in Response.reset():
>>>> - doing ~half of what recycle() does
>>>> - calling ActionCode.RESET which resets the OutputBuffer which in turn calls Response.recycle()
>>>
>>> There are several places where ActionCode.RESET is used.
>>
>> I thought I reviewed them but to be honest I have felt like I have been
>> going around in circles at times with the connector code so I'll take
>> another look.
>>
>>> Implementation of action(ActionCode.RESET) are different between
>>> AbstractHttp11Processor and AbstractAjpProcessor.
>>> I can track the above reasoning for HTTP only.
>>>
>>> I wonder why AJP behaves differently
>>
>> I half suspect there is no good reason. What has made cleaning up the
>> connector code trickier is that implementations started as copy and
>> paste and then diverged. That divergence went on for many years in some
>> cases. Pulling it back together is taking time.
>>
>>> and suspect that this change may break it.
>>
>> My instinct is that it shouldn't. The behaviour of the coyote Response
>> object should be independent of the Protocol in use. Equally, based on
>> experience, there is a fair chance that something will have broken. Like
>> I said above, I'll take another look.
>
> The difference is that for AJP we now call recycle rather than than
> doing ~half of what recycle does. The extra calls made are:
>
> commited = false;
> commitTime = -1;
>
> These 2 should be NO-OPs as the response has not been committed.
>
> errorException = null;
>
> This one is used to pass back an exception when flush is called so
> resetting it here should be fine.
>
> // Servlet 3.1 non-blocking write listener
> listener = null;
> fireListener = false;
> registeredForWrite = false;
>
> I think these are OK as we don't call RESET while we are using async.
> That said, if this is an issue we already had the issue for HTTP and at
> least now AJP and HTTP will be consistent even if they are wrong (which
> I don't think they are).
>
> // update counters
> contentWritten=0;
>
> This is a NO-OP as the response has not been committed.
>
> So in summary, I think this change is OK.
>
Ack, OK.
I re-reviewed with your arguments in mind and see that nothing has changed.
Several comments.
First
--------
One oddity here is, though. The success of this refactoring is based
on fact that ActionCode.RESET is used internally in coyote only, as
implementation and meaning of this 'RESET' code was changed.
Maybe do it the other way,
Remove "reset()" method from Response object and replace it with call
to action(ActionCode.RESET). See r1598246 for an inspiration.
It implies the following:
a) Restore response.recycle() in http11.AbstractOutputBuffer.
b) Implement ActionCode.RESET in AJP connector:
{ response.recycle(). }
Motivations:
1) AbstractOutputBuffer#nextRequest() already calls response.recycle().
So it already calls recycle() method in similar use case.
2) AbstractOutputBuffer#reset() throws ISE with message text.
The method in Response does not has text in its ISE.
3) ActionCode.RESET now will have original meaning and can be called
from outside of coyote.
Second
------------
> // Servlet 3.1 non-blocking write listener
> listener = null;
> fireListener = false;
> registeredForWrite = false;
(I think ServletResponse.reset() is not prohibited in async mode. It
is prohibited once response is committed, but startAsync alone does
not commit the response, nor does registering a WriteListener.)
Its javadoc says
http://docs.oracle.com/javaee/7/api/javax/servlet/ServletResponse.html#reset%28%29
[quote]
If getWriter() or getOutputStream() have been called before this
method, then the corrresponding returned Writer or OutputStream will
be staled and the behavior of using the stale object is undefined.
[/quote]
Thus it should be OK to clear the listener installed via that old OutputStream.
It a bit contradicts to the following
http://docs.oracle.com/javaee/7/api/javax/servlet/ServletOutputStream.html#setWriteListener%28javax.servlet.WriteListener%29
[quote]
IllegalStateException - if one of the following conditions is true
* the associated request is neither upgraded nor the async started
* setWriteListener is called more than once within the scope of the
same request.
[/quote]
I think the above "called more than once within ... the same request"
condition is reset by response.reset() call.
Third
--------
Looking at org.apache.catalina.connector.Response.reset()
I suspect that it has to recycle o.a.c.connector.OutputBuffer.conv converter.
Are there encodings that may leave bytes in converter's buffer
during char->bytes conversion?
Best regards,
Konstantin Kolinko
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1598307 - in /tomcat/trunk/java/org/apache/coyote:
Response.java http11/AbstractOutputBuffer.java
Posted by Mark Thomas <ma...@apache.org>.
On 02/06/2014 00:50, Konstantin Kolinko wrote:
<snip/>
> Ack, OK.
> I re-reviewed with your arguments in mind and see that nothing has changed.
>
> Several comments.
>
> First
> --------
> One oddity here is, though. The success of this refactoring is based
> on fact that ActionCode.RESET is used internally in coyote only, as
> implementation and meaning of this 'RESET' code was changed.
>
> Maybe do it the other way,
> Remove "reset()" method from Response object and replace it with call
> to action(ActionCode.RESET). See r1598246 for an inspiration.
> It implies the following:
> a) Restore response.recycle() in http11.AbstractOutputBuffer.
> b) Implement ActionCode.RESET in AJP connector:
> { response.recycle(). }
>
> Motivations:
> 1) AbstractOutputBuffer#nextRequest() already calls response.recycle().
> So it already calls recycle() method in similar use case.
> 2) AbstractOutputBuffer#reset() throws ISE with message text.
> The method in Response does not has text in its ISE.
> 3) ActionCode.RESET now will have original meaning and can be called
> from outside of coyote.
Hmm. It needs moving a check for a committed response into
AbstractAjpProcessor. Let me think about that.
At the back of my mind is a desire to get as close as possible to
non-endpoint specific implementations with all the endpoint specific
code in the Endpoint class and/or SocketWrapper.
> Second
> ------------
>
>> // Servlet 3.1 non-blocking write listener
>> listener = null;
>> fireListener = false;
>> registeredForWrite = false;
>
> (I think ServletResponse.reset() is not prohibited in async mode. It
> is prohibited once response is committed, but startAsync alone does
> not commit the response, nor does registering a WriteListener.)
>
> Its javadoc says
> http://docs.oracle.com/javaee/7/api/javax/servlet/ServletResponse.html#reset%28%29
>
> [quote]
> If getWriter() or getOutputStream() have been called before this
> method, then the corrresponding returned Writer or OutputStream will
> be staled and the behavior of using the stale object is undefined.
> [/quote]
>
> Thus it should be OK to clear the listener installed via that old OutputStream.
>
> It a bit contradicts to the following
> http://docs.oracle.com/javaee/7/api/javax/servlet/ServletOutputStream.html#setWriteListener%28javax.servlet.WriteListener%29
>
> [quote]
> IllegalStateException - if one of the following conditions is true
> * the associated request is neither upgraded nor the async started
> * setWriteListener is called more than once within the scope of the
> same request.
> [/quote]
>
> I think the above "called more than once within ... the same request"
> condition is reset by response.reset() call.
Agreed. This is no different to the Writer/OutputStream issue.
> Third
> --------
> Looking at org.apache.catalina.connector.Response.reset()
> I suspect that it has to recycle o.a.c.connector.OutputBuffer.conv converter.
>
> Are there encodings that may leave bytes in converter's buffer
> during char->bytes conversion?
Not to my knowledge so (for now) I think we are safe.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org