On Fri, 4 Apr 2025 15:08:25 GMT, Jaikiran Pai <[email protected]> wrote:
>> Volkan Yazici has updated the pull request incrementally with five
>> additional commits since the last revision:
>>
>> - Remove timeout from `CountDownLatch::await` calls
>> - Replace `@AutoClose` with a corresponding `@AfterEach` method
>> - Remove IDE-specific `OptionalGetWithoutIsPresent` warning suppression
>> - Improve `HttpConnection::label` JavaDoc
>> - Start from 1 while labeling connections
>
> test/jdk/java/net/httpclient/HttpResponseConnectionLabelTest.java line 412:
>
>> 410: HttpResponse<String> response1 = client.send(pair.request,
>> BodyHandlers.ofString(CHARSET));
>> 411: LOGGER.log("Firing request 2...");
>> 412: HttpResponse<String> response2 = client.send(pair.request,
>> BodyHandlers.ofString(CHARSET));
>
> In theory, there's no guarantee that these sequential requests will be
> executed "immediately" one after the other. Internally, in the httpclient
> implementation, we use idle timeouts to close idle connections. So I'm
> wondering if it's realistic that there would ever be a case where in some
> setup (like the CI), these two execute so far apart from each other
> (`-Xcomp`?) that the connection might have timed out in the meantime and
> closed? Thus the second request ends up using a different connection and
> fails this test?
> Should we perhaps use `othervm` for this test and configure an extremely high
> idle timeout of connections, through the system properties, to avoid such
> intermittent failures?
Good point - our default idle timeout is 30s IIRC. There would need to be a
pause of 30s between the two calls to `client.send` however. Not impossible but
unlikely. We could either pass a higher timeout (with e.g.
`-Djdk.httpclient.keepalive.timeout=120`) preemptively, or wait until we see
the test fail...
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24154#discussion_r2029032560
