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]Re: HTTP Post and HTTP/100 (continue)
Date Tue, 25 Feb 2003 20:53:39 GMT
This shouldn't be a problem for a couple of reasons:

- HttpMethodBase.reponseConnection will get set to null by 
    responseStream.close()
- Calling HttpConnection.releaseConnection() multiple times on 
connections created by the MultiThreadedHttpConnectionManager is okay. 
these connections are wrapped in an adapter that allows for only a 
single release

Having said all of this I agree that we should probably put 
ensureConnectionRelease() in an else statement.  It is more readable. 
Also, I think I'll look at the 
MultiThreadedHttpConnectionManager.releaseConnection() a little more as 
there is potential for a NPE when it is called directly (instead of by 
the connection)

Sam, thanks for pointing this out.

Mike

Sam Maloney wrote:
> 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();
>>     }
>> 
>>     /**
> 
> 
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: commons-httpclient-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: commons-httpclient-dev-help@jakarta.apache.org
> 


Mime
View raw message