tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mark Thomas <ma...@apache.org>
Subject Re: svn commit: r1598307 - in /tomcat/trunk/java/org/apache/coyote: Response.java http11/AbstractOutputBuffer.java
Date Mon, 02 Jun 2014 14:12:56 GMT
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


Mime
View raw message