On Wed, 14 Aug 2024 09:55:34 GMT, Jaikiran Pai <j...@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
