On Wed, 2 Apr 2025 11:08:38 GMT, Jaikiran Pai <[email protected]> wrote:
>> Volkan Yazici has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Apply review suggestions
>
> src/java.net.http/share/classes/jdk/internal/net/http/HttpConnection.java
> line 114:
>
>> 112:
>> 113: private static String nextLabel() {
>> 114: return "" + LABEL_COUNTER.getAndIncrement();
>
> The first label that this will generate will be `0`. It might be OK, but I
> think it would be better if we start it as `1`. So maybe consider using
> `LABEL_COUNTER.incrementAndGet()` instead?
Implemented in d64a901cf508ed7feeb0290733c61628d2ef9837.
> test/jdk/java/net/httpclient/HttpResponseConnectionLabelTest.java line 68:
>
>> 66: import static org.junit.jupiter.api.Assertions.assertTrue;
>> 67:
>> 68: @SuppressWarnings("OptionalGetWithoutIsPresent")
>
> I haven't seen this `OptionalGetWithoutIsPresent` warning before and looking
> into the JDK repo I don't see it being used or recognized any place else. Is
> this meant for some specific IDEs?
Removed in bcf106b27063e886167a29ecc92a3073a4334c55.
> test/jdk/java/net/httpclient/HttpResponseConnectionLabelTest.java line 83:
>
>> 81:
>> 82: // Start with a fresh client having no connections in the pool
>> 83: @AutoClose
>
> I hadn't come across this JUnit Jupiter annotation before. Reading through
> https://junit.org/junit5/docs/current/user-guide/#writing-tests-built-in-extensions-AutoClose
> it appears to be a convenience mechanism for managing closing of the
> resources at the "right time". That doc states:
>
>> Specifically, if the test class is configured with
>> @TestInstance(Lifecycle.PER_METHOD) semantics, a non-static @AutoClose field
>> will be closed after the execution of each test method, test factory method,
>> or test template method. However, if the test class is configured with
>> @TestInstance(Lifecycle.PER_CLASS) semantics, a non-static @AutoClose field
>> will not be closed until the current test class instance is no longer needed
>
> Do you know what test class lifecycle is configured for test classes like
> this one when launched through jtreg?
>
> Typically, we just use very basic JUnit constructs in the tests in the JDK
> repo to make it simpler to follow these tests, even if it means a bit more
> verbose code like having a method annotated with `@BeforeEach`/`@AfterEach`
> to do construction/clean up like this.
Replaced with `@AfterEach` in e32a48409478764ff5ab921c61be49f8ecffcff6.
> test/jdk/java/net/httpclient/HttpResponseConnectionLabelTest.java line 156:
>
>> 154: "Server[%s] is waiting for the
>> latch... (connectionKey=%s, responseBody=%s)",
>> 155: serverId, connectionKey,
>> responseBody);
>> 156:
>> assertTrue(serverResponseLatchRef[0].await(2, TimeUnit.SECONDS));
>
> We should avoid using any kind of timeouts here and in other places where we
> call this `await(...)` in this test. Given how varied the test execution
> environments are and our past experience with such timeouts, there's no right
> timeout to choose from. Instead, we should just do a `await()` without the
> timeout, so that if for whatever reason the latch isn't counted down, then
> the jtreg test execution timeout (which is controlled and configured
> externally to the test) will kick in and jtreg will do the necessary work of
> failing the test and also gathering detailed diagnostics through any failure
> handlers that are configured in that test execution environment.
Removed timeouts in 5b418fd7a9f4cfc3da940611ae3ba9d43e57aa91.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24154#discussion_r2028746855
PR Review Comment: https://git.openjdk.org/jdk/pull/24154#discussion_r2028746699
PR Review Comment: https://git.openjdk.org/jdk/pull/24154#discussion_r2028746544
PR Review Comment: https://git.openjdk.org/jdk/pull/24154#discussion_r2028746361
