On Mon, 6 Oct 2025 19:41:07 GMT, Volkan Yazici <[email protected]> wrote:
>> Currently `HttpRequest::timeout` only applies until the response headers are
>> received. Extend its scope to also cover the consumption of the response
>> body.
>>
>> ### Review guidelines
>>
>> 1. Read _"the fix"_ in `MultiExchange`
>> 2. Skim through the test server *handler* in `TimeoutResponseTestSupport`
>> 3. Review first `TimeoutResponseHeaderTest`, and then
>> `TimeoutResponseBodyTest` (Mind the multiple `@test` blocks!)
>
> 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.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/27469#discussion_r2409632252
