Mailing-List: contact commons-httpclient-dev-help@jakarta.apache.org;
 run by ezmlm
From: Sam Maloney <sam.maloney@filogix.com>
Organization: FiLogix, inc
To: "Commons HttpClient Project" <commons-httpclient-dev@jakarta.apache.org>
Subject: Re: [PATCH]Re: HTTP Post and HTTP/100 (continue)
Date: Tue, 25 Feb 2003 15:35:13 -0500
User-Agent: KMail/1.5
References: <9E035BE80785AA4EAA456959ECB4A30D035CA734@nt036.an.sopra>
 <3E5BC0F6.4080608@u.washington.edu>
In-Reply-To: <3E5BC0F6.4080608@u.washington.edu>
MIME-Version: 1.0
Content-Type: text/plain;
  charset="iso-8859-1"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline
Message-Id: <200302251535.13434.sam.maloney@filogix.com>

That patch looks good, but for one bug I believe it would cause (and I *may* 
be wrong here):

As far as I can tell, it would cause 
HttpConnectionManager->releaseConnection(conn) to be called twice for conn, 
assuming that there was a responseStream set.

In the case that MultiThreadedHttpConnectionManager->releaseConnection() gets 
called twice for the same conn, it would be bad. The implmentation of that 
method does not check in any way to see if the connection is allready 
'released', nor does it look like it would fail (meaning exception *before* 
the damage happens :). It looks like what would happen would be that the 
connection would be added twice to the LinkedList called freeConnections. The 
problem would then happen that two different threads would be able to get the 
same connection by calling getConnection, as getConnection simply does:
if(freeConnections.size() > 0)
return freeConnections.removeFirst();

Your patch is fine otherwise, so perhaps you should add, and put your patch 
inside,  an else clause to the if conditional that is allready there, ie:

public void releaseConnection() {
	if(responseStream != null){
		 try{
			responseStream.close()
		}catch(IOException e){
		}
-	}
+	} else {
+		ensureConnectionRelease();
+	}
}

Cheers,
Sam

On Tuesday 25 February 2003 14:16, Michael Becke wrote:
> > What is moreover a trouble is that the connection wasn't released after
> > the failed post. Indeed as I call method.releaseConnection() and that
> > response Inputstream is empty, I didn't see anything like
> > HttpConnectionManager.releaseConnection:  Release connection for
> > org.apache.commons.httpclient.HostConfiguration@b8332793
> > after that request, whereas every other have it.
>
> You are correct.  It seems if an error occurs in
> HttpMethodBase.execute() before the response stream is set the
> connection will not be released.  The attached patch should fix this.  I
> will go ahead and commit this one after alpha 3 if all agree.
>
> Mike
> Index: java/org/apache/commons/httpclient/HttpMethodBase.java
> ===================================================================
> RCS file:
> /home/cvspublic/jakarta-commons/httpclient/src/java/org/apache/commons/http
>client/HttpMethodBase.java,v retrieving revision 1.116
> diff -u -r1.116 HttpMethodBase.java
> --- java/org/apache/commons/httpclient/HttpMethodBase.java      25 Feb 2003
> 02:10:16 -0000      1.116 +++
> java/org/apache/commons/httpclient/HttpMethodBase.java      25 Feb 2003
> 19:11:54 -0000 @@ -1188,7 +1188,9 @@
>                  // attempting cleanup, don't care about exception.
>              }
>          }
> -
> +        // Make sure the connection has been released. If the response
> stream +        // has not been set, this is the only way to release the
> connection. +        ensureConnectionRelease();
>      }
>  
>      /**