Hi again,
I found a bug that has existed in the CVS source from when I first checked out
(02/16), and still exists in CVS upto last update (10:30am this morning.)
Severity: HIGH, because it will lock up the application using HttpClient.
Reproduce: This bug is easy to reproduce (see below). all you need is a
connection refused, and your connection is locked forever.
Result of bug: Connections won't get released, and thread lock will happen.
Fix: simple (see below for proposed patch).
The generally accepted way of doing things (based on your example code) is:
class MyAdaptor {
public MyAdaptor {
client = new HttpClient(new MultiThreadedConnectionManager());
}
public sendRequest(URL send_to, InputStream ins){
PostMethod method = new PostMethod(send_to.toString());
method.setRequestBody(ins);
method.setRequestContentLength(PostMethod.CONTENT_LENGTH_CHUNKED);
client.executeMethod(method);
// check status code, get response, then...
method.releaseConnection();
}
}
Now, this all works perfectly, if everything works perfectly (ie: no
connection refused!).
If the url is wrong or the url is invalid as the remote host is down or the
server on the host is down, then a 'IOException: Connection refused' will
happen inside HttpClient->executeMethod().
Here is the code in question:
<SNIP>
HttpConnection connection = state.getHttpConnectionManager().getConnection(
methodConfiguration,
httpConnectionTimeout
);
method.setStrictMode(strictMode);
if (!connection.isOpen()) {
connection.setSoTimeout(soTimeout);
connection.setConnectionTimeout(connectionTimeout);
connection.open();
if (connection.isProxied() && connection.isSecure()) {
method = new ConnectMethod(method);
}
}
return method.execute(state, connection);
</SNIP>
Problem: the IOException happens on connection.open(), it is not caught (in
throw statement, this is okay and needed), but since it is not caught, the
next line: method = new ConnectMethod(method) doesn't happen!
That line connects the connection that has been checked out allready (the call
to manager->getConnection()) to the method.
Since the method doesn't know about the connection yet, even calling
method->releaseConnection() will do nothing (it will go: is conn null? yup?
then return and do nothing).
This leaves the connection in the checked out collection.
If this happens twice, the default amount of connections will be checked out,
but forgotten about, and then all calls to manager->getConnection(...) will
block forever.
FIX: Since HttpClient is the one who checked out the connection, it is upto
that class to handle releasing the connection in an error condition.
PATCH:
Index: HttpClient.java
===================================================================
RCS file:
/home/cvspublic/jakarta-commons/httpclient/src/java/org/apache/commons/httpclient/HttpClient.java,v
retrieving revision 1.68
diff -u -r1.68 HttpClient.java
--- HttpClient.java 28 Jan 2003 04:40:20 -0000 1.68
+++ HttpClient.java 20 Feb 2003 17:11:03 -0000
@@ -551,16 +551,27 @@
);
method.setStrictMode(strictMode);
-
- if (!connection.isOpen()) {
- connection.setSoTimeout(soTimeout);
- connection.setConnectionTimeout(connectionTimeout);
- connection.open();
- if (connection.isProxied() && connection.isSecure()) {
- method = new ConnectMethod(method);
+
+ try{
+ if (!connection.isOpen()) {
+ connection.setSoTimeout(soTimeout);
+ connection.setConnectionTimeout(connectionTimeout);
+ connection.open();
+ if (connection.isProxied() && connection.isSecure()) {
+ method = new ConnectMethod(method);
+ }
}
+ }catch(IOException e){// Ie: IOException: Connection refused.
+ // Make sure to release the connection, as although the user may
call
+ // Method->releaseConnection(), the method doesn't know about the
+ // connection yet.
+ connection.releaseConnection();
+ throw e;
+ }catch(RuntimeException e){
+ connection.releaseConnection();
+ throw e;
}
-
+
return method.execute(state, connection);
}
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
