RFR: 8324209: Check implementation of Expect: 100-continue in the java.net.http.HttpClient [v3]

[Darragh Clarke]

On Wed, 14 Aug 2024 09:55:34 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> Darragh Clarke has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   remove duplicate method
>
> src/java.net.http/share/classes/jdk/internal/net/http/Exchange.java line 465:
> 
>> 463: 
>> 464:         return ex.getResponseAsync(parentExecutor)
>> 465:                 .orTimeout(responseTimeout, TimeUnit.SECONDS)
> 
> Hello Darragh, would it be wise to check for the `responseTimeout` to be greater than `0`? I see that we set it to the request timeout value above and if the request timeout is say 500 milli seconds, then `responseTimeout` will end up being `0`.
> `CompletableFuture.orTimeout()` doesn't specify how it deals with `0` or negative time value to the parameter of that method. So I think we might have to do some checks here to avoid passing `0`. Maybe we should just use nanos as a time unit?

Thats a good point, changing the timeunit to nanos is probably for the best.

I wonder in the case of timeout being equal to 0 what the best way to handle it is though? 
perhaps something like
```         
        long responseTimeout = TimeUnit.SECONDS.toNanos(5);
        if (request.timeout().isPresent()
                && request.timeout().get().getNano() > 0
                && request.timeout().get().getNano() < responseTimeout) {
            responseTimeout = request.timeout().get().getNano();
        }

> src/java.net.http/share/classes/jdk/internal/net/http/ExchangeImpl.java line 72:
> 
>> 70:     }
>> 71: 
>> 72:     final void setExpectTimeoutRaised(boolean timeoutRaised) {
> 
> Nit - I don't think we will ever be calling this method with a value of `false`. So maybe we should just make this a no-arg method which in its implementation will set the flag to true? If you prefer it in the current form, that's fine too.

I wouldn't be opposed to that, I can include it with the next commit

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

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