Refuse a hop the deadline has no time for

Review round one on the absolute deadline.

Clamping a spent deadline to zero only delayed the abort. The next hop
still took a connection permit, took a connection and wrote the request,
and the timeout arrived a tick later - so a 307 put its body on the
redirect target while the caller was handed a TimeoutException that
reads as though nothing had been sent. sendNextRequest, which every
redirect, auth replay and retry funnels through, now fails the exchange
before the write when the deadline has passed. Worse than reported, as
it turns out: on a coarse wheel the exchange did not merely abort late,
it ran both hops and succeeded, the deadline exceeded and nobody told.

The anchor was getStart(), which is currentTimeMillis. A per-attempt
timeout can only be distorted by a clock step for the length of one hop;
an anchor spanning a whole exchange carries the step to every hop after
it, so a correction back would hand a later hop a budget it never had
and one forward would abort it on a healthy connection. The future now
records System.nanoTime() at submission and the budget is netted off
that, leaving wall clock only where main already had it, per hop.

The comment justifying the clamp said the task still cancels its
read-timeout sibling. There is no sibling at that point: the read
timeout is armed after the write. The reason is simply that a scheduler
has no use for a negative delay and the task has to run, running being
what fails the exchange.

DefaultRequest's widened constructor is package private. Public, it
would be pinned by revapi at twenty-seven arguments, the next
per-request option would make it twenty-eight, and the two parameter
lists would have to be kept in step by hand with a tail that is all
reference types for the compiler to confuse. RequestBuilderBase#build is
the only caller.

Two fields were being dropped by hand-copied lists, which is the same
fault this change exists to fix. Redirect30xInterceptor rebuilds the
next request from a hand-picked set and carried neither the read timeout
- reverting a per-request value to the config default on every hop after
the first - nor the deadline flag, which left the Request disagreeing
with the behaviour a filter or a signature calculator would read off it.
RequestBuilderBase's signature-calculator copy block dropped the read
timeout the same way.

The interface default returns false rather than reading the property, as
every other option on it does; the javadoc now says so, so that a custom
config setting the property and getting nothing is documented rather
than surprising.

Three of the tests asserted only that nothing was thrown, which a
dropped Location header or redirects turned off would have satisfied
having run a single hop. They assert the final status and which hop it
came from. The five were also all wall clock, so TimeoutsHolderTest
covers the anchor and the clamp directly: what a second holder makes of
the same exchange is the whole difference between the two modes.
AsyncHttpClientDefaultsTest was asserting no default for this option,
nor for the event-loop one merged alongside it, and now does both.

Claude Code on behalf of Pavel Ptashyts

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Author: pavel-ptashyts

client/src/main/java/org/asynchttpclient/AsyncHttpClientConfig.java

@@ -159,6 +159,10 @@ default boolean isUseEventLoopTimeouts() {
      * Off by default because turning it on shortens exchanges that rely on the per-attempt behaviour. A caller
      * working to an end-to-end budget wants it on; {@link Request#getUseAbsoluteRequestDeadline()} sets it for a
      * single request.
+     * <p>
+     * As with every option on this interface, the {@code org.asynchttpclient.useAbsoluteRequestDeadline}
+     * property is read by {@link DefaultAsyncHttpClientConfig.Builder}, not here: an implementation of this
+     * interface that does not override this method gets {@code false} whatever the property says.
      *
      * @return {@code true} to treat the request timeout as a deadline for the whole exchange
      */

client/src/main/java/org/asynchttpclient/DefaultRequest.java

@@ -107,38 +107,42 @@ public DefaultRequest(String method,
     }
 
     /**
+     * Not public, and the parameter is trailing rather than beside {@code followRedirect}: the constructor above
+     * keeps the signature outside callers compile against, while this one stays free to grow. A public
+     * twenty-seven argument constructor would be pinned by revapi, and its parameter list would have to be kept
+     * in step with the one above by hand, with the compiler unable to help once the tail is all reference types.
+     * {@link RequestBuilderBase#build()} is the only caller.
+     *
      * @param useAbsoluteRequestDeadline whether {@code requestTimeout} bounds the whole exchange rather than
-     *                                   each attempt within it, or null to defer to the client config. Trailing
-     *                                   rather than beside {@code followRedirect} so the original signature
-     *                                   stays intact for callers that build a request without the builder.
+     *                                   each attempt within it, or null to defer to the client config
      */
-    public DefaultRequest(String method,
-                          Uri uri,
-                          @Nullable InetAddress address,
-                          @Nullable InetAddress localAddress,
-                          HttpHeaders headers,
-                          List<Cookie> cookies,
-                          byte @Nullable [] byteData,
-                          @Nullable List<byte[]> compositeByteData,
-                          @Nullable String stringData,
-                          @Nullable ByteBuffer byteBufferData,
-                          @Nullable ByteBuf byteBufData,
-                          @Nullable InputStream streamData,
-                          @Nullable BodyGenerator bodyGenerator,
-                          List<Param> formParams,
-                          List<Part> bodyParts,
-                          @Nullable String virtualHost,
-                          @Nullable ProxyServer proxyServer,
-                          @Nullable Realm realm,
-                          @Nullable File file,
-                          @Nullable Boolean followRedirect,
-                          @Nullable Duration requestTimeout,
-                          @Nullable Duration readTimeout,
-                          long rangeOffset,
-                          @Nullable Charset charset,
-                          ChannelPoolPartitioning channelPoolPartitioning,
-                          NameResolver<InetAddress> nameResolver,
-                          @Nullable Boolean useAbsoluteRequestDeadline) {
+    DefaultRequest(String method,
+                   Uri uri,
+                   @Nullable InetAddress address,
+                   @Nullable InetAddress localAddress,
+                   HttpHeaders headers,
+                   List<Cookie> cookies,
+                   byte @Nullable [] byteData,
+                   @Nullable List<byte[]> compositeByteData,
+                   @Nullable String stringData,
+                   @Nullable ByteBuffer byteBufferData,
+                   @Nullable ByteBuf byteBufData,
+                   @Nullable InputStream streamData,
+                   @Nullable BodyGenerator bodyGenerator,
+                   List<Param> formParams,
+                   List<Part> bodyParts,
+                   @Nullable String virtualHost,
+                   @Nullable ProxyServer proxyServer,
+                   @Nullable Realm realm,
+                   @Nullable File file,
+                   @Nullable Boolean followRedirect,
+                   @Nullable Duration requestTimeout,
+                   @Nullable Duration readTimeout,
+                   long rangeOffset,
+                   @Nullable Charset charset,
+                   ChannelPoolPartitioning channelPoolPartitioning,
+                   NameResolver<InetAddress> nameResolver,
+                   @Nullable Boolean useAbsoluteRequestDeadline) {
         this.method = method;
         this.uri = uri;
         this.address = address;

client/src/main/java/org/asynchttpclient/RequestBuilderBase.java

@@ -700,6 +700,7 @@ private RequestBuilderBase<?> executeSignatureCalculator() {
         rb.followRedirect = followRedirect;
         rb.useAbsoluteRequestDeadline = useAbsoluteRequestDeadline;
         rb.requestTimeout = requestTimeout;
+        rb.readTimeout = readTimeout;
         rb.rangeOffset = rangeOffset;
         rb.charset = charset;
         rb.channelPoolPartitioning = channelPoolPartitioning;

client/src/main/java/org/asynchttpclient/netty/NettyResponseFuture.java

@@ -92,6 +92,10 @@ public final class NettyResponseFuture<V> implements ListenableFuture<V> {
             .newUpdater(NettyResponseFuture.class, Object.class, "partitionKeyLock");
 
     private final long start = unpreciseMillisTime();
+    // Wall clock is what start reports, and a deadline anchored on it for the length of a whole exchange would
+    // be moved by any clock correction that lands mid-chain: a step back would hand a later hop a budget it
+    // never had, a step forward would abort it on a healthy connection. Elapsed time is measured from here.
+    private final long startNanos = System.nanoTime();
     private final ChannelPoolPartitioning connectionPoolPartitioning;
     private final ConnectionSemaphore connectionSemaphore;
     // Not final: a filter replay can retarget this future at a different origin, reached through a
@@ -601,6 +605,14 @@ public long getStart() {
         return start;
     }
 
+    /**
+     * When this exchange was submitted, on a monotonic clock, for measuring how much of a deadline spanning the
+     * whole exchange it has spent. Comparable only with other {@link System#nanoTime()} readings.
+     */
+    public long getStartNanos() {
+        return startNanos;
+    }
+
     public Object getPartitionKey() {
         Object override = partitionKeyOverride;
         if (override != null) {

client/src/main/java/org/asynchttpclient/netty/handler/intercept/Redirect30xInterceptor.java

@@ -139,7 +139,19 @@ public boolean exitAfterHandlingRedirect(Channel channel, NettyResponseFuture<?>
                         .setNameResolver(request.getNameResolver())
                         .setProxyServer(request.getProxyServer())
                         .setRealm(stripAuth ? null : request.getRealm())
-                        .setRequestTimeout(request.getRequestTimeout());
+                        .setRequestTimeout(request.getRequestTimeout())
+                        // Dropped here until now, so a per-request read timeout reverted to the config default
+                        // on every hop after the first.
+                        .setReadTimeout(request.getReadTimeout());
+
+                // Also dropped, which left the request disagreeing with the deadline the exchange was being
+                // held to: the future carries the flag, so a filter or a signature calculator reading the
+                // request saw per-attempt timeouts while the exchange was bounded as a whole. Only when it was
+                // set, since the setter takes a primitive and null means defer to the client config.
+                Boolean useAbsoluteRequestDeadline = request.getUseAbsoluteRequestDeadline();
+                if (useAbsoluteRequestDeadline != null) {
+                    requestBuilder.setUseAbsoluteRequestDeadline(useAbsoluteRequestDeadline);
+                }
 
                 if (stripAuth) {
                     future.setRealm(null);

client/src/main/java/org/asynchttpclient/netty/request/NettyRequestSender.java

@@ -81,6 +81,7 @@
 import org.asynchttpclient.proxy.ProxyType;
 import org.asynchttpclient.resolver.RequestHostnameResolver;
 import org.asynchttpclient.uri.Uri;
+import org.asynchttpclient.util.StringBuilderPool;
 import org.asynchttpclient.ws.WebSocketUpgradeHandler;
 import org.jetbrains.annotations.Nullable;
 
@@ -98,9 +99,11 @@
 import java.util.Locale;
 import java.util.Map;
 import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.function.Consumer;
 
+import static org.asynchttpclient.util.DateUtils.unpreciseMillisTime;
 import static io.netty.handler.codec.http.HttpHeaderNames.EXPECT;
 import static java.util.Collections.singletonList;
 import static java.util.Objects.requireNonNull;
@@ -1213,9 +1216,25 @@ public boolean applyIoExceptionFiltersAndReplayRequest(NettyResponseFuture<?> fu
     }
 
     public <T> void sendNextRequest(final Request request, final NettyResponseFuture<T> future) {
+        TimeoutsHolder timeoutsHolder = future.getTimeoutsHolder();
+        if (timeoutsHolder != null && timeoutsHolder.isDeadlinePassed()) {
+            // Arming the next hop's timeout at zero would abort it, but only after this call has taken a
+            // connection permit, taken a connection and written the request -- so a 307 would put the body on
+            // the wire and then hand the caller a TimeoutException that reads as if nothing was sent.
+            abort(future.channel(), future, new TimeoutException(deadlinePassedMessage(request, future)));
+            return;
+        }
         sendRequest(request, future.getAsyncHandler(), future);
     }
 
+    private static String deadlinePassedMessage(Request request, NettyResponseFuture<?> future) {
+        return StringBuilderPool.DEFAULT.stringBuilder()
+                .append("Request timeout to ").append(request.getUri().getHost())
+                .append(':').append(request.getUri().getExplicitPort())
+                .append(" after ").append(unpreciseMillisTime() - future.getStart())
+                .append(" ms, before the next hop was sent").toString();
+    }
+
     private static void validateWebSocketRequest(Request request, AsyncHandler<?> asyncHandler) {
         Uri uri = request.getUri();
         boolean isWs = uri.isWebSocket();

client/src/main/java/org/asynchttpclient/netty/timeout/TimeoutsHolder.java

@@ -90,12 +90,13 @@ public TimeoutsHolder(Timer nettyTimer, @Nullable EventExecutor eventExecutor, N
         requestTimeoutValue = requestTimeoutInMs;
         absoluteDeadline = nettyResponseFuture.isUseAbsoluteRequestDeadline();
         if (requestTimeoutInMs > -1) {
-            // A redirect, a retry or an auth replay builds a new holder for the same future. Anchoring the
-            // deadline here hands each of those hops a fresh budget, so a chain of n hops runs for n times the
-            // configured timeout; anchoring it on the future bounds the exchange as a whole instead. Which one
-            // applies is the caller's choice, per request or per client.
-            requestTimeoutMillisTime = (absoluteDeadline ? nettyResponseFuture.getStart() : unpreciseMillisTime())
-                    + requestTimeoutInMs;
+            // A redirect, a retry or an auth replay builds a new holder for the same future. Giving each of
+            // those hops the configured timeout lets a chain of n hops run for n times it; netting off what the
+            // exchange has already spent bounds it as a whole instead. Which one applies is the caller's
+            // choice, per request or per client. May be negative, and is deliberately left so: a deadline
+            // already behind us has to read as behind us, so that startReadTimeout does not arm a sibling and
+            // isDeadlinePassed can say the exchange is over.
+            requestTimeoutMillisTime = unpreciseMillisTime() + (absoluteDeadline ? remainingBudget() : requestTimeoutInMs);
             requestTimeoutTask = new RequestTimeoutTimerTask(nettyResponseFuture, requestSender, this, requestTimeoutInMs);
         } else {
             requestTimeoutMillisTime = -1L;
@@ -117,11 +118,35 @@ public void start() {
         if (requestTimeoutTask != null) {
             // Per attempt, the configured duration: this runs within microseconds of the constructor, so
             // reading the clock again would only expose the deadline to a step between the two reads. An
-            // absolute deadline is anchored before this holder existed, so there the remainder is the budget.
-            arm(requestTimeoutTask, absoluteDeadline ? remainingRequestTimeout() : requestTimeoutValue);
+            // absolute deadline was anchored before this holder existed, so there the remainder is the budget,
+            // floored at zero: a task armed at zero still runs, and running is how the exchange gets failed.
+            arm(requestTimeoutTask, absoluteDeadline ? Math.max(remainingBudget(), 0L) : requestTimeoutValue);
         }
     }
 
+    /**
+     * Whether the exchange has run out of time to start another hop with. Always {@code false} when the timeout
+     * is per attempt, where a hop is given the configured timeout of its own by definition.
+     *
+     * @see org.asynchttpclient.AsyncHttpClientConfig#isUseAbsoluteRequestDeadline()
+     */
+    public boolean isDeadlinePassed() {
+        return absoluteDeadline && requestTimeoutValue > -1 && remainingBudget() <= 0L;
+    }
+
+    /**
+     * What is left of a deadline that spans the whole exchange, which may be negative. Measured from the
+     * future's monotonic start rather than by comparing wall clocks across hops.
+     */
+    // Visible for testing: the instant this holder's request timeout is due, as a wall-clock reading.
+    long requestTimeoutMillisTime() {
+        return requestTimeoutMillisTime;
+    }
+
+    private long remainingBudget() {
+        return requestTimeoutValue - TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - nettyResponseFuture.getStartNanos());
+    }
+
     /**
      * Moves this exchange's timeouts onto {@code executor}, the loop of the channel it turned out to run on. The
      * connect path arms the request timeout before there is a channel -- deliberately, since it bounds address
@@ -186,8 +211,8 @@ private static void release(@Nullable TimeoutTimerTask task) {
     }
 
     private long remainingRequestTimeout() {
-        // A deadline already behind us is armed at zero rather than negative, so the task still runs and still
-        // cancels its read-timeout sibling, which is bookkeeping only it does.
+        // Floored at zero rather than passed on negative: a scheduler has no use for a negative delay, and the
+        // task has to run either way, since running is what fails the exchange.
         return Math.max(requestTimeoutMillisTime - unpreciseMillisTime(), 0L);
     }