On Mon, 12 Aug 2024 14:22:13 GMT, Darragh Clarke <dcla...@openjdk.org> wrote:
>> Currently `HttpClient` will timeout if a server doesn't respond to a request >> which includes `Expect: 100-Continue` >> >> Section 10.1.1 of [rfc >> 9110](https://datatracker.ietf.org/doc/html/rfc9110#name-expect) states that >> >> a client SHOULD NOT wait for an indefinite period before sending the content. >> >> >> >> This PR changes `HttpClient` to wait for a maximum of 5 seconds for a server >> response, this will be shorter if a timeout is set. If no response is >> received, the message will be sent regardless. >> This should bring `HttpClient` in line with how >> [HttpUrlConnection](https://github.com/DarraghClarke/jdk/blob/61386c199a3b29457c002ad31a23990b7f6f88fd/src/java.base/share/classes/sun/net/www/protocol/http/HttpURLConnection.java#L1305) >> treats expect continue timeouts. >> >> This is done using `orTimeout` in the `expectContinue` method , though there >> is some changes in `streams.java` where it was possible for race conditions >> to cause timeouts where `CompleteableFuture`s were removed from >> `response_cfs` prematurely or in some cases not removed at all. >> >> I've tested this against tiers 1-3 and it appears to be stable. > > Darragh Clarke has updated the pull request incrementally with one additional > commit since the last revision: > > remove duplicate method Thanks Jaikiran for chiming in. It pushed me to have another look at CompletableFuture where I noticed [completeOnTimeout](https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/util/concurrent/CompletableFuture.html#completeOnTimeout(T,long,java.util.concurrent.TimeUnit)) which should simplify this part hugely. src/java.net.http/share/classes/jdk/internal/net/http/Exchange.java line 484: > 482: cf = CompletableFuture.completedFuture(response); > 483: return cf; > 484: }).thenCompose(Function.identity()) Lines 465-484 could be simplified to a single line call: .completeOnTimeout(null, responseTimeout, TimedUnit.XXXX) since all we do in there is to replace the `TimeoutException` with a null response. ------------- Changes requested by dfuchs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20525#pullrequestreview-2237809090 PR Review Comment: https://git.openjdk.org/jdk/pull/20525#discussion_r1716718249
