Ask the deadline before arming, not the holder

Review round two on the absolute deadline.

The check that refuses a hop with nothing left to spend was in
sendNextRequest, which only continuations pass through. A first attempt
goes straight to sendRequest, and under ROUND_ROBIN the up-front resolve
asks for no timeout at all, so a slow resolver could eat the whole
budget before any holder existed and a pooled hit would then arm at zero
and write anyway. The check has moved into scheduleRequestTimeout, which
every attempt passes through, first or otherwise, and which is the last
point before the request is written. It returns whether the attempt may
go ahead; the four call sites stop when it may not.

Asking the holder was the wrong question anyway: the first attempt has
no holder to ask. The budget is now a static on TimeoutsHolder, taken
off the future, which every caller has in hand before an attempt of its
own exists. Long.MAX_VALUE stands for an exchange that is not bounded as
a whole, so a per-attempt timeout needs no special case at the call
site. Resolving the configured timeout moved there with it, so the
constructor and the budget no longer resolve it separately.

Three comments described the change rather than the code, which
AGENTS.md asks us not to and which will not read well in a year: the two
on the fields Redirect30xInterceptor was dropping, and the one on
DefaultRequest's package-private constructor. The comment on
requestTimeoutMillisTime claimed isDeadlinePassed depended on it staying
negative; only startReadTimeout does. And the javadoc for the budget had
been left sitting on the test accessor by an earlier edit.

One test asserted nothing. isDeadlinePassed answered on its first
conjunct for a per-attempt exchange, so the arithmetic it was meant to
cover never ran. It asserts on the deadline the holder computes instead:
that a hop is handed the configured timeout of its own however long the
exchange has already run.

Carrying the read timeout across a redirect changes behaviour outside
this flag - a short per-request read timeout was reverting to the config
default on every hop after the first and now does not - and is called
out in the pull request for the release notes.

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/DefaultRequest.java

@@ -107,11 +107,10 @@ 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.
+     * The full set of fields a request carries, called only by {@link RequestBuilderBase#build()}. Not public and
+     * not part of the API: the constructor above is what outside callers compile against, so this one is free to
+     * take another field without pinning a signature or asking the next reader to keep two parameter lists of
+     * reference types in step by eye.
      *
      * @param useAbsoluteRequestDeadline whether {@code requestTimeout} bounds the whole exchange rather than
      *                                   each attempt within it, or null to defer to the client config

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

@@ -140,14 +140,12 @@ public boolean exitAfterHandlingRedirect(Channel channel, NettyResponseFuture<?>
                         .setProxyServer(request.getProxyServer())
                         .setRealm(stripAuth ? null : request.getRealm())
                         .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.
+                // The exchange holds the deadline flag on its future, so a hop that does not carry it forward
+                // leaves the request saying something the exchange is not doing, which is what a filter or a
+                // signature calculator reads. Set only when the request has one: the setter takes a primitive,
+                // and null is how a request defers to the client config.
                 Boolean useAbsoluteRequestDeadline = request.getUseAbsoluteRequestDeadline();
                 if (useAbsoluteRequestDeadline != null) {
                     requestBuilder.setUseAbsoluteRequestDeadline(useAbsoluteRequestDeadline);

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

@@ -412,9 +412,10 @@ private <T> ListenableFuture<T> sendRequestWithOpenChannel(NettyResponseFuture<T
         future.attachChannel(channel, false);
 
         SocketAddress channelRemoteAddress = channel.remoteAddress();
-        if (channelRemoteAddress != null) {
+        if (channelRemoteAddress != null
+                && !scheduleRequestTimeout(future, (InetSocketAddress) channelRemoteAddress, channel)) {
             // otherwise, bad luck, the channel was closed, see bellow
-            scheduleRequestTimeout(future, (InetSocketAddress) channelRemoteAddress, channel);
+            return future;
         }
 
         if (LOGGER.isDebugEnabled()) {
@@ -525,7 +526,9 @@ private <T> ListenableFuture<T> sendRequestWithNewChannel(Request request, Proxy
                 abort(null, future, new UnknownHostException("No addresses resolved for " + request.getUri().getHost()));
                 return future;
             }
-            scheduleRequestTimeout(future, roundRobinAddresses.get(0));
+            if (!scheduleRequestTimeout(future, roundRobinAddresses.get(0))) {
+                return future;
+            }
             connectWithAddresses(request, proxy, future, asyncHandler, roundRobinAddresses);
             return future;
         }
@@ -595,16 +598,16 @@ private <T> Future<List<InetSocketAddress>> resolveAddresses(Request request, Pr
         if (proxy != null && !proxy.isIgnoredForHost(uri.getHost()) && proxy.getProxyType().isHttp()) {
             int port = ProxyType.HTTPS.equals(proxy.getProxyType()) || uri.isSecured() ? proxy.getSecuredPort() : proxy.getPort();
             InetSocketAddress unresolvedRemoteAddress = InetSocketAddress.createUnresolved(proxy.getHost(), port);
-            if (scheduleTimeout) {
-                scheduleRequestTimeout(future, unresolvedRemoteAddress);
+            if (scheduleTimeout && !scheduleRequestTimeout(future, unresolvedRemoteAddress)) {
+                return abortedResolution(future);
             }
             return resolveHostname(request, unresolvedRemoteAddress, asyncHandler);
         } else {
             int port = uri.getExplicitPort();
 
             InetSocketAddress unresolvedRemoteAddress = InetSocketAddress.createUnresolved(uri.getHost(), port);
-            if (scheduleTimeout) {
-                scheduleRequestTimeout(future, unresolvedRemoteAddress);
+            if (scheduleTimeout && !scheduleRequestTimeout(future, unresolvedRemoteAddress)) {
+                return abortedResolution(future);
             }
 
             if (request.getAddress() != null) {
@@ -1087,26 +1090,40 @@ private static void configureTransferAdapter(AsyncHandler<?> handler, HttpReques
         ((TransferCompletionHandler) handler).headers(h);
     }
 
-    private void scheduleRequestTimeout(NettyResponseFuture<?> nettyResponseFuture,
-                                        InetSocketAddress originalRemoteAddress) {
-        scheduleRequestTimeout(nettyResponseFuture, originalRemoteAddress, null);
+    private boolean scheduleRequestTimeout(NettyResponseFuture<?> nettyResponseFuture,
+                                           InetSocketAddress originalRemoteAddress) {
+        return scheduleRequestTimeout(nettyResponseFuture, originalRemoteAddress, null);
     }
 
     /**
+     * Arms the timeouts for the attempt about to be made, unless the exchange has no time left to make it in.
+     * Every attempt passes through here, whether it is the first or a redirect, an auth replay or a retry, and
+     * it is the last point before the request is written -- so it is where a deadline is worth one more look.
+     * Arming at zero instead would abort the attempt, but only after a connection permit had been taken, a
+     * connection taken and the request written: a 307 would put its body on the redirect target and then hand
+     * the caller a TimeoutException that reads as though nothing had been sent.
+     *
      * @param channel the channel the exchange will run on when it is already known, so the timeout can be armed
      *                on the loop that owns it. Null on the connect path: the timeout is armed before the channel
      *                exists, deliberately, so that it also bounds address resolution and the connect itself, and
      *                {@code TimeoutsHolder#rehomeOn} moves it onto the loop once there is one.
+     * @return whether the attempt may go ahead. When {@code false} the exchange has already been aborted.
      */
-    private void scheduleRequestTimeout(NettyResponseFuture<?> nettyResponseFuture,
-                                        InetSocketAddress originalRemoteAddress,
-                                        @Nullable Channel channel) {
+    private boolean scheduleRequestTimeout(NettyResponseFuture<?> nettyResponseFuture,
+                                           InetSocketAddress originalRemoteAddress,
+                                           @Nullable Channel channel) {
+        if (TimeoutsHolder.remainingBudget(config, nettyResponseFuture) <= 0L) {
+            abort(nettyResponseFuture.channel(), nettyResponseFuture,
+                    new TimeoutException(deadlinePassedMessage(nettyResponseFuture.getTargetRequest(), nettyResponseFuture)));
+            return false;
+        }
         nettyResponseFuture.touch();
         TimeoutsHolder timeoutsHolder = new TimeoutsHolder(nettyTimer, timeoutExecutor(channel), nettyResponseFuture,
                 this, config, originalRemoteAddress);
         // Arms the timeout as a part of installing the holder, which is why the pooled path attaches the
         // channel first: an expiry that lands immediately reaches the channel only through the future.
         nettyResponseFuture.setTimeoutsHolder(timeoutsHolder);
+        return true;
     }
 
     /**
@@ -1216,23 +1233,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);
     }
 
+    /**
+     * A resolution that will not be attempted, for an attempt the exchange has no time left to make. The
+     * exchange is aborted before this is returned, so the failure carried here only stops the listener from
+     * carrying on with a connect.
+     */
+    private static <T> Future<List<InetSocketAddress>> abortedResolution(NettyResponseFuture<T> future) {
+        return ImmediateEventExecutor.INSTANCE.newFailedFuture(
+                new TimeoutException(deadlinePassedMessage(future.getTargetRequest(), 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();
+                .append(" ms, before the request was sent").toString();
     }
 
     private static void validateWebSocketRequest(Request request, AsyncHandler<?> asyncHandler) {

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

@@ -82,21 +82,18 @@ public TimeoutsHolder(Timer nettyTimer, @Nullable EventExecutor eventExecutor, N
         final long readTimeoutInMs = targetRequest.getReadTimeout().toMillis();
         readTimeoutValue = readTimeoutInMs == 0 ? config.getReadTimeout().toMillis() : readTimeoutInMs;
 
-        long requestTimeoutInMs = targetRequest.getRequestTimeout().toMillis();
-        if (requestTimeoutInMs == 0) {
-            requestTimeoutInMs = config.getRequestTimeout().toMillis();
-        }
+        long requestTimeoutInMs = requestTimeout(config, targetRequest);
 
         requestTimeoutValue = requestTimeoutInMs;
         absoluteDeadline = nettyResponseFuture.isUseAbsoluteRequestDeadline();
         if (requestTimeoutInMs > -1) {
             // 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);
+            // choice, per request or per client. Left negative when the deadline is already behind us, which is
+            // what stops startReadTimeout arming a sibling for an exchange that is over.
+            requestTimeoutMillisTime = unpreciseMillisTime()
+                    + (absoluteDeadline ? remainingBudget(requestTimeoutInMs, nettyResponseFuture) : requestTimeoutInMs);
             requestTimeoutTask = new RequestTimeoutTimerTask(nettyResponseFuture, requestSender, this, requestTimeoutInMs);
         } else {
             requestTimeoutMillisTime = -1L;
@@ -120,33 +117,51 @@ public void start() {
             // reading the clock again would only expose the deadline to a step between the two reads. An
             // 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);
+            arm(requestTimeoutTask, absoluteDeadline
+                    ? Math.max(remainingBudget(requestTimeoutValue, nettyResponseFuture), 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.
+     * How much of a deadline spanning the whole exchange is left, in milliseconds, negative once it has passed.
+     * {@link Long#MAX_VALUE} when the timeout is per attempt or disabled, neither of which bounds an exchange as
+     * a whole: an attempt is then given the configured timeout of its own however long the exchange has run.
+     * <p>
+     * Measured from the future's monotonic start rather than by comparing wall clocks across hops, so a clock
+     * correction landing mid-chain cannot move the deadline. Static, and asked of the future rather than of a
+     * holder, because a caller deciding whether a request is still worth sending has the future in hand before
+     * any holder exists for the attempt it is about to make.
      *
      * @see org.asynchttpclient.AsyncHttpClientConfig#isUseAbsoluteRequestDeadline()
      */
-    public boolean isDeadlinePassed() {
-        return absoluteDeadline && requestTimeoutValue > -1 && remainingBudget() <= 0L;
+    public static long remainingBudget(AsyncHttpClientConfig config, NettyResponseFuture<?> nettyResponseFuture) {
+        if (!nettyResponseFuture.isUseAbsoluteRequestDeadline()) {
+            return Long.MAX_VALUE;
+        }
+        return remainingBudget(requestTimeout(config, nettyResponseFuture.getTargetRequest()), nettyResponseFuture);
+    }
+
+    private static long remainingBudget(long requestTimeoutInMs, NettyResponseFuture<?> nettyResponseFuture) {
+        if (requestTimeoutInMs <= -1) {
+            return Long.MAX_VALUE;
+        }
+        long spent = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - nettyResponseFuture.getStartNanos());
+        return requestTimeoutInMs - spent;
     }
 
     /**
-     * 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.
+     * The request timeout in force for {@code request}: its own, or the client's when it does not carry one.
      */
+    private static long requestTimeout(AsyncHttpClientConfig config, Request request) {
+        long requestTimeoutInMs = request.getRequestTimeout().toMillis();
+        return requestTimeoutInMs == 0 ? config.getRequestTimeout().toMillis() : requestTimeoutInMs;
+    }
+
     // 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

client/src/test/java/org/asynchttpclient/AbsoluteRequestDeadlineTest.java

@@ -185,12 +185,12 @@ void assertTimedOut() {
         }
 
         /**
-         * That the exchange was failed by the check before the next hop was written, rather than by the timeout
-         * arming at zero and expiring once it had been. The message is the only thing that tells the two apart.
+         * That the exchange was failed by the check before the request was written, rather than by a timeout
+         * armed at zero expiring once it had been. The message is the only thing that tells the two apart.
          */
         void assertTimedOutBeforeSending() {
             assertTimedOut();
-            assertTrue(cause.getMessage().contains("before the next hop was sent"),
+            assertTrue(cause.getMessage().contains("before the request was sent"),
                     "expected the deadline to be caught before the write, got " + cause.getMessage());
         }
     }

client/src/test/java/org/asynchttpclient/netty/timeout/TimeoutsHolderTest.java

@@ -27,7 +27,6 @@
 
 import java.time.Duration;
 
-import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
 /**
@@ -73,31 +72,39 @@ public void aPerAttemptTimeoutGivesTheSecondHopItsOwnBudget() throws Exception {
     }
 
     @RepeatedIfExceptionsTest(repeats = 5)
-    public void anExchangeThatOutranItsDeadlineSaysSo() throws Exception {
+    public void anExchangeThatOutranItsDeadlineHasNothingLeft() throws Exception {
         // A budget this small is spent by the time the sleep is over, so the next hop has nothing to run in.
         NettyResponseFuture<?> future = exchange(true);
         Thread.sleep(ELAPSED_MS);
 
-        assertTrue(holder(future, Duration.ofMillis(1)).isDeadlinePassed(),
-                "a spent deadline should report itself as passed");
+        assertTrue(TimeoutsHolder.remainingBudget(config(Duration.ofMillis(1)), future) <= 0,
+                "a spent deadline should leave nothing to send a further hop with");
     }
 
     @RepeatedIfExceptionsTest(repeats = 5)
-    public void aPerAttemptExchangeNeverRunsOutOfBudgetBetweenHops() throws Exception {
+    public void aPerAttemptExchangeIsNotBoundedAsAWhole() throws Exception {
+        // Asserted on the deadline the holder computes rather than on the budget: per attempt there is no
+        // exchange-wide budget to run out of, so the arithmetic is not what the answer rests on.
         NettyResponseFuture<?> future = exchange(false);
         Thread.sleep(ELAPSED_MS);
 
-        assertFalse(holder(future, Duration.ofMillis(1)).isDeadlinePassed(),
-                "a per-attempt timeout hands every hop a budget of its own, however long the exchange has run");
+        long deadline = deadlineOf(future, BUDGET);
+
+        assertTrue(deadline - System.currentTimeMillis() >= BUDGET.toMillis() - TOLERANCE_MS,
+                "a hop should be given the configured timeout of its own however long the exchange has run, got "
+                        + (deadline - System.currentTimeMillis()) + " ms");
     }
 
     private static long deadlineOf(NettyResponseFuture<?> future, Duration requestTimeout) {
         return holder(future, requestTimeout).requestTimeoutMillisTime();
     }
 
     private static TimeoutsHolder holder(NettyResponseFuture<?> future, Duration requestTimeout) {
-        return new TimeoutsHolder(null, future, null,
-                new DefaultAsyncHttpClientConfig.Builder().setRequestTimeout(requestTimeout).build(), null);
+        return new TimeoutsHolder(null, future, null, config(requestTimeout), null);
+    }
+
+    private static AsyncHttpClientConfig config(Duration requestTimeout) {
+        return new DefaultAsyncHttpClientConfig.Builder().setRequestTimeout(requestTimeout).build();
     }
 
     private static NettyResponseFuture<?> exchange(boolean useAbsoluteRequestDeadline) {