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? Interesting. If requestTimeout is 500ms I guess it means we will send the body immediately without waiting. Possibly the best course of action here could be to use millis as the granularity. Given the latency of HTTP then anything below 1ms is probably equivalent to 0 (no wait). > src/java.net.http/share/classes/jdk/internal/net/http/Exchange.java line 493: > >> 491: exchImpl.setExpectTimeoutRaised(true); >> 492: CompletableFuture<Response> cf = >> 493: exchImpl.sendBodyAsync() > > When a timeout occurs due to `orTimeout()` step of a CompletableFuture > pipeline, the subsequent stages in that pipeline get invoked on a thread > that's internal to the `java.util.concurrent` code and isn't the one that's > part of the executor that's configured on our HttpClient. > Looking at `sendBodyAsync()` which gets invoked if there's a timeout, I > suspect it's OK if that method gets invoked through this internal thread. > Daniel, do you see any issues if this internal thread ends up calling the > `sendBodyAsync()` here? There should be no issue. The future returned to the caller of sendAsync has the appropriate calls to make sure user dependent actions are run in the FJP. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/20525#discussion_r1716713825 PR Review Comment: https://git.openjdk.org/jdk/pull/20525#discussion_r1716720696
