On Thu, 12 Feb 2026 14:40:35 GMT, Volkan Yazici <[email protected]> wrote:
>> Jaikiran Pai has updated the pull request incrementally with one additional >> commit since the last revision: >> >> undo unintentional whitespace changes during merge conflict resolution > > test/jdk/java/net/httpclient/NullReturningBodyHandlerTest.java line 175: > >> 173: .build(); >> 174: // test for HTTP/2 upgrade when there are no already >> established connections >> 175: args.add(Arguments.of(new Request(h2HttpReq, Version.HTTP_2))); > > `test()` issues two requests one after another using `send()` and > `sendAsync()`. Effectively, `sendAsync()` will never observe an empty > connection pool. I don't think this is something bad, but it makes me think > if we add any value by trying with an empty connection pool. I see that there is a probability that `BodyHandler::apply` returning null will fail the HTTP/2 upgrade, and no connections will make it to the pool. Hence, both `send()` and `sendAsync()` will observe an empty pool. > test/jdk/java/net/httpclient/NullReturningBodyHandlerTest.java line 177: > >> 175: args.add(Arguments.of(new Request(h2HttpReq, Version.HTTP_2))); >> 176: // test for HTTP/2 upgrade when there is an established >> connection >> 177: args.add(Arguments.of(new Request(h2HttpReq, Version.HTTP_2, >> true))); > > The test case added 1 line above will already admit a HTTP/2 clear-text > connection to the pool. Why do we need `requiresWarmupHEADRequest=true` here? I see that there is a probability that `BodyHandler::apply` returning null will fail the HTTP/2 upgrade, and no connections will make it to the pool. Hence, a 2nd `args` entry with explicit `requiresWarmupHEADRequest=true` makes sense. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/29691#discussion_r2812203064 PR Review Comment: https://git.openjdk.org/jdk/pull/29691#discussion_r2812199370
