biasedbit / http-client Goto Github PK

When an exception is notified to DefaultHttpConnection, the exceptionCaught method invokes terminate, but terminate doesn't tell the listener that the connection is terminated, so it is never removed from the pool and when the maximum number of concurrent connections is reached, the (Abstract)HttpClient can't connect any more.
Perhaps the channelClosed method is supposed to do that, but it doesn't as it finds the this.terminate variable already set and quits soon.

My temporary fix is to call "this.listener.connectionTerminated(this);" at the very end of the terminate() method, but it would probably correct to deal with the channelClosed method or the currentRequest and its Future in a different way.

I had this happening all the time with the socket throwing a SocketException with a reasonMessage of "Connection reset".

Hey

The variable queuedRequests in http-client / src / main / java / com / biasedbit / http / client / AbstractHttpClient.java is never decremented in case of failures(cannot connect etc), because of same even if eventQueue is empty the requests may be rejected stating eventQueue is full.

Thanks & Regards
Jatinder

Hi Bruno, I have to report another bug. I'm still using the pre-refactoring version, but looking at your refactored code, I think the bug is still there. The problem is that when HTTP requests time out (the server-side actually never sends a response in my case), the DefaultHttpConnection (now DefaultConnection) is not terminated (and not available, but it shouldn't be available in this case), so it sits in the connection pool, inactive, forever.
A first fix could seem to be to invoke this.connection.terminate(cause) in DefaultHttpRequestFuture (now DefaultRequestFuture), in setFailure(Throwable cause) (and the overloaded method below, perhaps), but I'm not sure it would be correct in all cases and in some cases this.connection is null, so it has to be protected with an "if (connection != null)" or with "if (cause == HttpRequestFuture.TIMED_OUT)". The connection being null sounds a bit like an inconsistency within the code too.
So I resolved to temporarily make the Default(Http)Connection a listener of the Default(Http)RequestFuture and invoke currentRequestFinished() conditionally (if (future.getCause() == HttpRequestFuture.TIMED_OUT) and caring for when this.currentResponse is null).

What do you think would be the best solution?