Use only one request timeout mechanism in JdkClientHttpRequest

[cfredri4]

Previously, a timeout was set both on HttpRequest, and used on httpClient.sendAsync().get(). This leads to inconsistent behaviour depending on which timeout gets triggered first. This change changes so that timeout is only set on the HttpRequest.

[snicoll]

This was added in 6.1.3, see #31911

[cfredri4]

@snicoll thanks I was not aware this issue existed with HttpClient. But according to JDK-8258397 then the current implementation with additional timeout on CompletableFuture will not help because it fetches an InputStream?

A possibility for the caller is to make use of the CompletableFuture API (get/join will accept a timeout, or CF::orTimeout can be called).
IIRC - in that case, it will still be the responsibility of the caller to cancel the request. We might want to reexamine and possibility change that.
The disadvantage here is that some of our BodyHandlers (ofPublisher, ofInputStream) will return immediately - so the CF API won't help in this case.

It feels like there are two options; introducing a separate timeout where the input stream is consumed (this would impact other client types too, maybe in a good way of they have similar behavior?), or accepting the behavior that the existing timeout does not account for the body being received.

[rstoyanchev]

Indeed, the comment in JDK-8258397 refers to the BodyHandlers#ofInputStream which we use. Looking at ResponseSubscribers.HttpResponseInputStream, the getBody method there returns CompletableFuture.completedStage(this), i.e. an already completed future that can't be cancelled. We could presumably wrap HttpResponse.BodySubscribers.ofInputStream(), intercept CompletionStage<InputStream> getBody() to get access to the InputStream which would allow us to actually cancel by closing the InputStream after a timeout exception. That would be a potential workaround while the JDK issue is not resolved.

That said I'm not sure I follow the original comment about the inconsistency. @cfredri4 can you elaborate?

[cfredri4]

My comment about inconsistency was a hypothetical one. I just happened across the code and reacted to that the same timeout was set twice in two different ways.
Now it's clear that my worry was a non-issue but instead an entirely different problem has emerged. 😅

[cfredri4]

A suggestion; use the non-async send without timeout, capture the input stream before returning it, and schedule a separate timer to interrupt the sending thread+cancel input stream on timeout?
If reasonable I can prepare a PR.

[rstoyanchev]

I suppose we don't gain anything from the Future, and only using it as a timer. What scheduling did you have in mind to replace that with?

[cfredri4]

I haven't looked into it yet but initial thought is to use the common scheduler used for CompletableFuture timeouts/delayedExecutor. A guess is that it's the same or similar to what's used by HttpClient internally so it should be no overall change in behavior.

[rstoyanchev]

Sure, give that a try.

[cfredri4]

After looking some more into this.
The non-async send() just calls sendAsync() which in term returns a custom CompletableFuture which can be cancelled to cancel the request.
HttpClient keeps track of its own timeouts which are then ran by the selector thread.

We don't want to schedule multiple timeouts so that means no timeout on HttpRequest, or on the request future get(), and instead our custom timeout needs to cancel the request future if not already finished.
Other than that we then just need to cancel the timeout when the body is read completely.
Having a custom scheduler feels like overkill and the common scheduler should be ok enough since our timeout task will just trigger cancellation/close a stream (i.e. basically no work).
The CompletableFuture delayedExecutor does not cancel nicely (it doesn't remove the scheduled future from the delayed queue) but it can be done with completeOnTimeout instead.

I've pushed an initial draft proposal to my branch.
Can you review and provide feedback?

[rstoyanchev]

Thanks for the changes so far.

I think this is a direction we could work towards. I would like to suggest a little refactoring. Create an inner class, e.g. TimeoutHandler that manages the timeout future internally, and exposes a method to wrap the response InputStream. That would keep the executeInternal method simpler, and separate out the timeout concern, so that eventually when the JDK provides a fix, it's easier to see what code we no longer need.

It would be good to have some tests as well possibly in RestClientIntegrationTests.

A few more specific comments below.

[rstoyanchev]

Shouldn't this be checking if the task was cancelled successfully, i.e. that cancel() returns true?

[rstoyanchev]

Can we really be sure there is a body, I imagine the timeout could happen before the server responds with status and headers.

[rstoyanchev]

I think we could have constructors like these:

public JdkClientHttpResponse(HttpResponse<InputStream> response) {
    this(response, response.getInputStream());
}

public JdkClientHttpResponse(HttpResponse<InputStream> response, InputStream body) {

}

That would keep the logic mostly as it was before and avoid the full qualified java.net.http.HttpHeaders type.

[cfredri4]

Cool, I'll sort it out after summer vacation.

[rstoyanchev]

I've gone ahead and completed this, but if you have a chance, please take a look.