On Tue, 7 Oct 2025 07:16:01 GMT, Michael McMahon <[email protected]> wrote:
>> src/java.net.http/share/classes/jdk/internal/net/http/MultiExchange.java
>> line 537:
>>
>>> 535: if (currentreq.timeout().isPresent()) {
>>> 536: // Retried/Forwarded requests should reset the timer, if
>>> present
>>> 537: cancelTimer();
>>
>> In `master` (i.e., without this change), retried (due to some failure) and
>> forwarded (due to 3XX responses with a `Location` header) requests will
>> reset the timer at each retry and forwarding, respectively. This PR
>> preserves this behavior, while additionally extending the request timeout's
>> scope to cover _the complete retrieval_ of the response body too. But I
>> would like to open this to discussion:
>>
>> 1. Shall we preserve the existing behavior and reset the timer on
>> retries/forwarding?
>> 2. Shall we ensure an `HttpClient::send` never exceeds the configured
>> timeout even when there are retries/forwarding?
>> 4. Something else?
>>
>> Note that the 1<sup>st</sup> option (i.e., the current behavior) implies
>> that, if I configure a client to take max. 30 seconds for a request, I can
>> be unfortunate enough to learn at production that it can actually take up to
>> `5*30=150` seconds, where 5 is the default retry limit. That said, this
>> behavior matches with Apache HTTP Client's
>> [setResponseTimeout](https://hc.apache.org/httpcomponents-client-5.5.x/current/httpclient5/apidocs/org/apache/hc/client5/http/config/RequestConfig.Builder.html#setResponseTimeout-org.apache.hc.core5.util.Timeout-).
>
> It would be very unusual for a server to wait 29 seconds before redirecting.
> I think you can assume that redirects will come in a reasonably timely
> fashion. The idea behind the timeout is really to allow for potentially large
> response bodies being downloaded and that's only going to happen once. So, a
> more likely scenario would be 1 or 2 seconds per redirect and then however
> long it takes to download the resource. So, in my opinion, the current
> behavior is okay.
For the record, @djelinski and @dfuch also (internally) shared that the current
behavior is okay.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/27469#discussion_r2414379565
