On Fri, 4 Apr 2025 14:48:16 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 190:
>
>> 188: private static HttpTestServer createServer(Version version,
>> SSLContext sslContext) {
>> 189: try {
>> 190: return HttpTestServer.create(version, sslContext,
>> ForkJoinPool.commonPool());
>
> The HTTP/2 test server in the JDK shuts down the `Executor` that is passed to
> that server (like this one), when the server is stopped. I had a look at the
> specification and implementation of `ForkJoinPool.commonPool()` - it is
> specified to not close/shutdown the common pool. So I think it is OK to use
> the `commonPool()` as the `Executor` for these servers.
>
> Although, since this test is agentvm test, we never known what state the
> commonPool() executor is in due to some previously run test(s). Maybe we
> should consider having a test specific Executor in this test, like we do in
> some other tests within the httpclient tests.
Another possibility is to run in /othervm mode - then we don't care...
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24154#discussion_r2028979965
