spring-projects
Only set timeout one way in JdkClientHttpRequest
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 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.
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?
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. 😅
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.
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?
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.
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?
I've gone ahead and completed this, but if you have a chance, please take a look.