hc-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Michael Becke <be...@u.washington.edu>
Subject Re: [PATCH] close handled properly, round three
Date Tue, 03 Dec 2002 19:27:44 GMT

On Tuesday, December 3, 2002, at 01:54 PM, Eric Johnson wrote:

> Michael,
>
> By problem, do you mean something that is causing a failure?  Can you 
> outline the scenario?

The problem would occur when using HttpClient from multiple threads.  
Closing the previous inputStream would release the connection 
prematurely thereby allowing a connection to be used by more than one 
method.  For the connection factory to work the general contract of 
acquire, use, release needs to be kept.

>
> I think I see what you are getting at.  Your new 
> ResponseAutoReleaseInputStream certainly added complexity on top of 
> what I just added, so I agree we should iron out the details.  I think 
> these are the scenarios:
>
>    * Response with content-length header
>    * Response with content-transfer-encoding of chunked
>    * Response with neither of the above
>    * Response with either of the first two options, but with
>      "connection: close"

I agree, I created the same list:)  The only thing to add is that all 6 
of these are eventually wrapped by a ResponseAutoReleaseInputStream.  
Also, in the case where nothing is wrapping the inputStream from the 
socket, closing the stream will not have the desired effect (i.e. it 
will close the socket input stream).

> The code takes advantage of the fact that the last two cases are 
> similar (AutoCloseInputStream).
>
> I think we now handle all of these case correctly, thanks in part to 
> your new stream wrapper.  My merge with your code, however, I think 
> erred in one regard.  The execute method could look something like 
> this to patch the problem:
>
>        HttpConnection connection = 
> state.getHttpConnectionManager().getConnection(
>            methodConfiguration,
>            httpConnectionTimeout
>        );
>
>        method.setStrictMode(strictMode);
>              // close any preexisting response that may not have been 
> closed.  For
>        // previous responses with "Connection: close", this close the 
> open
>        // connection.  For a connection that remains open, this will 
> have the
>        // effect of consuming whatever remains of the previous 
> response.
>        InputStream previousStream = 
> connection.getSingleResponseInputStream();
>        if ( previousStream != null) {
>            previousStream.close();
>            // NEW -- here we get a new connection, as the preceding 
> close will have returned the connection to the
>            // connection pool.
>            connection = state.getHttpConnectionManager().getConnection(
>                methodConfiguration,
>                httpConnectionTimeout);
>            // END NEW
>        }
>
> This seems a little ugly to me.  An alternative might be to change the 
> HttpConnectionManager contract and implementation to return a 
> connection where the previous response has already been closed (in the 
> sense of being fully read, now effected by a close on the wrapped 
> stream) - effectively moving the code that I put in executeMethod() 
> into the respective connection managers.  If I understand the code 
> correctly, this is a no-op for the MultiThreadedHttpConnectionManager, 
> as a connection cannot be in the pool unless the previous response has 
> been completely read.  For the Simple variant, the code would need to 
> be added.

Agreed, very ugly.  Handling this on release might make more sense, as 
you mention.  Actually now that I think about it more, perhaps the 
ResponseAutoReleaseInputStream should be automatically added to the 
socket's stream, making it the first wrapping stream instead of the 
last.  If this were the case the code in ContentLengthInputStream, 
ChunkedInputStream and elsewhere that finishes reading the response 
could be called from HttpConnection.releaseConnection().

> As for fixing the issue of so many wrappers for input streams, as I 
> mentioned before, we could push the code in 
> ResponseAutoReleaseInputStream down into the specific stream wrappers. 
>  The AutoCloseInputStream essentially does this already, and it would 
> not be too hard to do the same thing with the other two stream 
> wrappers.
>
> What do others think?

I'm going to fiddle with your patch a little and send a possible 
solution in a few minutes.

Mike


Mime
View raw message