RFR: 8350279: HttpClient: Add a new HttpResponse method to identify connections [v5]

[Volkan Yazici]

On Wed, 2 Apr 2025 11:08:38 GMT, Jaikiran Pai <jpai at openjdk.org> 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