CODEWITHSUNDEEP
javaasynchttpclient
user256717
user256717

Reputation:

AsyncHttpClient and timeouts

In AsyncHttpClient JDKFuture.get()

 public V  [More ...] get(long timeout, TimeUnit unit)  {
        V content = null;
        try {
            if (innerFuture != null) {
                content = innerFuture.get(timeout, unit);
            }
        } catch (TimeoutException t) {
            if (!contentProcessed.get() && timeout != -1 && 
              ((System.currentTimeMillis() -   touch.get()) <= responseTimeoutInMs)) {
                return get(timeout, unit);
            }

Why do we have 2 timeouts?

  1. timeout as param
  2. responseTimeoutInMs

Second timeout is hurting us, as the call doesn't comeout even after timeout expires. It keeps calling get() again recursively.

Will the connection get closed once responseTimeoutInMs is hit? We are trying to set it to lower than timeout.

Upvotes: 2

Views: 4995

Answers (2)

Stephane Landelle
Stephane Landelle

Reputation: 7038

Don't use the JDK provider, it's broken and is being dropped in AHC 2. Use the Netty one, and upgrade to a modern version.

Upvotes: 0

Holger
Holger

Reputation: 298539

I assume you are referring to the method I found in the net:

public V get(long timeout, TimeUnit unit) throws InterruptedException, ExecutionException, TimeoutException {
    V content = null;
    try {
        if (innerFuture != null) {
            content = innerFuture.get(timeout, unit);
        }
    } catch (TimeoutException t) {
        if (!contentProcessed.get() && timeout != -1 && ((System.currentTimeMillis() - touch.get()) <= responseTimeoutInMs)) {
            return get(timeout, unit);
        }

        if (exception.get() == null) {
            timedOut.set(true);
            throw new ExecutionException(new TimeoutException(String.format("No response received after %s", responseTimeoutInMs)));
        }
    } catch (CancellationException ce) {
    }

    if (exception.get() != null) {
        throw new ExecutionException(exception.get());
    }
    return content;
}

You can consider this class as being erroneous in several regards. The first mistake which jumps directly into the eye is the use of System.currentTimeMillis() instead of System.nanoTime(). System.currentTimeMillis() refers to the computers system clock which may be adjusted during the programs execution and hence can jump back and forth. Code dealing with timeouts should use System.nanoTime() which gives a value relative to the programs execution independent to the real world clock.

responseTimeoutInMs seems to be meant as a connection timeout but using it even when the timeout passed as parameter value has been expired is a violation of the Future contract. The correct behavior would be letting the get method time out even when the task represented by the Future may be still running.

But invoking the get method recursively is a double fault. Not only is the recursion dangerous as small timeout values may lead to a StackOverflowError; passing the same timeout again to itself means deferring the time out infinitely as every re-invocation will treat that value as relative to the current time.

Interestingly, even if the method gets to the point of timing out it will wrap the TimeoutException inside an ExecutionException reporting a completely wrong semantic to the caller.

I don’t believe that you will find someone on stackoverflow who can explain the rationale behind this implementation, if there is one at all. You will have to ask the supporter/authors of that code directly.

Upvotes: 3

Related Questions