On Wed, 14 Aug 2024 12:31:17 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 with a new target base due to a 
> merge or a rebase. The pull request now contains five commits:
> 
>  - Implemented feedback and cleanup
>  - Merge master
>  - remove duplicate method
>  - implemented feedback
>  - Add timeout handling to expect-continue implementation

test/jdk/java/net/httpclient/ExpectContinueTest.java line 359:

> 357: 
> 358:     private void verifyRequest(String path, int expectedStatusCode, 
> HttpResponse<String> resp, boolean exceptionally, Throwable testThrowable) {
> 359:         if(!exceptionally){

Suggestion:

        if (!exceptionally){

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20525#discussion_r1720806853

Reply via email to