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-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
