RFR: 8208693: HttpClient: Extend the request timeout's scope to cover the response body

Daniel Fuchs dfuchs at openjdk.org
Wed Oct 22 12:33:15 UTC 2025


On Wed, 24 Sep 2025 13:24:35 GMT, Volkan Yazici <vyazici at openjdk.org> 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 414:

> 412:                                 return handleNoBody(r, exch);
> 413:                         }
> 414:                         return exch.readBodyAsync(responseHandler, this::cancelTimer)

Could we remove the `preTerminationCallback` parameter, and instead have the various subclass of `HttpBodySubscriberWrapper` supply `this.multi::cancelTimer()` to their super class?

If I'm not mistaken all concrete subclasses of `HttpBodySubscriberWrapper` have access to the multi exchange.

That should reduce the amount of changes significantly.

Another possibility is to introduce a noarg protected abstract method in `HttpBodySubscriberWrapper` and have all subclasses implement it.
We could call it for instance `protected abstract void onTerminationNotified();` and explain that it can be used to cancel a timer if needed.

It might need to be called on cancel as well?
If not a comment explaining why might be good to have. If it's not called by cancel then maybe the method suggested above could be changed to `onCompletionNotified`.

src/java.net.http/share/classes/jdk/internal/net/http/common/HttpBodySubscriberWrapper.java line 412:

> 410:         if (preTerminationCallback != null) {
> 411:             preTerminationCallback.run();
> 412:         }

Did you consider adding this to complete() instead? There would then be a single place where this code would appear.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/27469#discussion_r2451899504
PR Review Comment: https://git.openjdk.org/jdk/pull/27469#discussion_r2451927047


More information about the net-dev mailing list