- 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 [EMAIL PROTECTED] 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: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
--------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
