Mailing-List: contact commons-httpclient-dev-help@jakarta.apache.org;
 run by ezmlm
Message-ID: <3DECFDE5.9050509@tibco.com>
Date: Tue, 03 Dec 2002 13:54:29 -0500
From: Eric Johnson <eric@tibco.com>
Organization: TIBCO Extensibility
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US;
 rv:1.2.1) Gecko/20021130
MIME-Version: 1.0
To: Commons HttpClient Project <commons-httpclient-dev@jakarta.apache.org>
Subject: Re: [PATCH] close handled properly, round three
References: <31306E4D-06E4-11D7-A8F9-00306557E112@u.washington.edu>
In-Reply-To: <31306E4D-06E4-11D7-A8F9-00306557E112@u.washington.edu>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

Michael,

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

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"

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.

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?

-Eric.

Michael Becke wrote:

> Hi Eric,
>
> I have come across the following problem in HttpClient.executeMethod():
>
>         InputStream previousStream =  
> connection.getSingleResponseInputStream();
>         if ( previousStream != null) {
>             previousStream.close();
>         }
>
> Closing the stream will have the effect of releasing the 
> HttpConnection  for use by others ( take a look at  
> HttpMethodBase.ResponseAutoReleaseInputStream ).
>
> It took me some time to work through the myriad of possible scenarios  
> to determine how streams get closed.  I don't envy you having worked 
> on  this for so long:)  Is seems that HttpClient's use of stream and 
> how  they are managed has become overly complicated.  Perhaps there is 
> some  way we can merge these different classes/method into something 
> more  coherent.  Any thoughts?
>
> Mike
>
> On Tuesday, December 3, 2002, at 11:22 AM, Eric Johnson wrote:
>