On Mon, 3 Aug 2026 15:44:42 GMT, Volkan Yazici <[email protected]> wrote:
>> Jaikiran Pai has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains eight additional
>> commits since the last revision:
>>
>> - prevent multi-threaded concurrent close resulting in closing more than
>> once
>> - readLoopThread needn't be a field
>> - merge latest from master branch
>> - merge latest from master branch
>> - missed copyright year updates on a couple more files
>> - copyright year
>> - test library improvement - add ability to get underlying test exchange
>> - Improve test library HTTP2 connection
>
> test/jdk/java/net/httpclient/lib/jdk/httpclient/test/lib/common/HttpServerAdapters.java
> line 565:
>
>> 563: throw new IllegalArgumentException("underlying exchange
>> "
>> 564: + this.exchange.getClass().getName() + " is not
>> of type: "
>> 565: + exchgType);
>
> You can also choose to simplify this as follows:
>
> Suggestion:
>
> exchgType.cast(exchange);
>
>
> This performs all the necessary checks, plus a `ClassCastException` with a
> good message.
My impression is that a `IllegalStateException` with an explicit message might
be more useful. But I don't have a strong preference to either, so if you and
others think `ClassCastException` would be better, I'll update accordingly.
> test/jdk/java/net/httpclient/lib/jdk/httpclient/test/lib/http2/Http2TestServerConnection.java
> line 346:
>
>> 344: return;
>> 345: }
>> 346: stopping = true;
>
> Is this thread-safe? Shouldn't this rather be `if
> (!stopped.compareAndSet(false, true)) { return; }`?
Good point, fixed in latest update to the PR.
> test/jdk/java/net/httpclient/lib/jdk/httpclient/test/lib/http2/Http2TestServerConnection.java
> line 1423:
>
>> 1421: private void log(final String msg) {
>> 1422: System.err.println(this.server.name + ": " + msg);
>> 1423: }
>
> You might consider replacing all `printf` et al. usages with invocations to a
>
>
> private static final Logger LOGGER =
> Utils.getDebugLogger(Http2TestServerConnection.class::getSimpleName)
>
>
> class field.
Did you literally mean `printf` or was this a general comment about replacing
the usage of `System.err` with the `Logger`? I can't spot any `printf` calls in
this class.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/32173#discussion_r3712121984
PR Review Comment: https://git.openjdk.org/jdk/pull/32173#discussion_r3712139492
PR Review Comment: https://git.openjdk.org/jdk/pull/32173#discussion_r3712136327
