Connection is not terminated when request times out.
Closed this issue · 10 comments
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?
Actually, I had to switch from invoking currentRequestFinished() to invoking terminate(HttpRequestFuture.TIMED_OUT);. It was more proper for a few reasons.
Off the top of my head, this makes sense. I'm going to dig in and get it fixed for 1.1
Thank you. I finally settled for the 'ifs' in the DefaultHttpRequestFuture, as looking at all the other HttpRequestFuture.* Throwables seemed to be catered for in their own way (connection terminated when necessary), though I'm a bit scared about the double mutex locking (invoking terminate(...) on connection, which acquires the mutex of the connection, from inside 'synchronized(this)' in the future - should it go out...?). This is my commit: MarcoDalco@5a288e0#src/main/java/com/biasedbit/http/future/DefaultHttpRequestFuture.java
Makes sense. Cancellation works the same way, when the future is cancelled, the connection must be severed.
Connection being null would be an inconsistency for a time out (a request can only timeout after it has begun executing) but not for a cancellation (since a request can be cancelled at any time, even prior to it having begun executing). Does this make sense?
Yes, perfectly and it confirms that the order of lock acquisition is correct. Should I create a pull request? I've committed onto the same branch (patch-1), I'm new to git and I'm not sure what it would ask you to pull, i.e. if it's cumulative with my previous commit, but I'll see.
Since I've merged my 1.1 branch back to master, the pull request can no longer be applied to my repository. Just keep it on your version and thanks, again, for the heads up!
Go ahead and take a look at this pull request to see if it matches your changes.
There's a main problem: at the time of the timeout (DefaultRequestFuture.failedWithCause(Throwable cause)) the connection is not 'available', so connection.terminate(cause) isn't run. This at least on my version, as I haven't checked out yours and I don't have Groovy, which basically I don't know.
I'm going to check whether checking for 'connection.isDone()' rather thar 'cause == RequestFuture.TIMED_OUT' is OK in all cases: it's not in my version for example for EXECUTION_REJECTED, but I've seen that you changed that case so it invokes the other failure method and it makes sense to me that if the failure is not related to the request/response, the connection should be killed.
I'll check the whole pull request, if you want, though today it's a bit busy, here. Got a release, today.
You're absolutely right. Went for a quick fix without paying too much attention and overlooked this case...
Good luck with your release!