On Tue, 1 Sep 2026 07:10:02 GMT, Volkan Yazici <[email protected]> wrote:

>> Fix the reuse of timed out idle HTTP Client connections when the eviction 
>> gets delayed.
>> 
>> The added test has certain assumptions to reproduce the problem. That said, 
>> it consistently, almost always, fails if the associated fix is missing. Put 
>> another way, the test can pass without the fix.
>> 
>> ---------
>> - [X] I confirm that I make this contribution in accordance with the 
>> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai).
>
> Volkan Yazici has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Improve comments

The code part looks good now. I believe the test has a few issues regarding 
testing with HTTP/3.

test/jdk/java/net/httpclient/IdleConnectionTimeoutReuseTest.java line 84:

> 82:  *          As of date, connection eviction is triggered by the selector 
> of
> 83:  *          `HttpClientImpl`, not by the QUIC selector. Being prudent, we 
> fix
> 84:  *          both to virtual threads.

Hmmm... if we're not on windows this should be the default - so theoretically 
no need to override it.
I have no objection to keeping these though.

test/jdk/java/net/httpclient/IdleConnectionTimeoutReuseTest.java line 214:

> 212:                     secure,
> 213:                     version,
> 214:                     () -> HttpTestServer.create(version, secure ? 
> SSL_CONTEXT : null),

Depending on the version (e.g if it's HTTP/3) you might want to create a server 
that is HTTP/3 only. It will avoid testing HTTP/2 when you think you are 
testing HTTP/3.

test/jdk/java/net/httpclient/IdleConnectionTimeoutReuseTest.java line 245:

> 243:                     .host(server.getAddress().getAddress())
> 244:                     .port(server.getAddress().getPort())
> 245:                     .path(handlerPath)

If the version is HTTP/3 you also might want to configure the request to use 
HTTP/3 only. As it stands you are probably not testing HTTP/3 fully.

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

PR Review: https://git.openjdk.org/jdk/pull/32545#pullrequestreview-5077112585
PR Review Comment: https://git.openjdk.org/jdk/pull/32545#discussion_r3903369622
PR Review Comment: https://git.openjdk.org/jdk/pull/32545#discussion_r3903427395
PR Review Comment: https://git.openjdk.org/jdk/pull/32545#discussion_r3903443147

Reply via email to