On Mon, 3 Aug 2026 12:59:39 GMT, Jaikiran Pai <[email protected]> wrote:
> Can I please get a review of this change to the httpclient test library which
> cleans up the `Http2TestServerConnection` as well as updates the
> `HttpTestExchange` to provide a way to get hold of the underlying exchange?
>
> Apart from the logging clean up, the changes mainly include a new method on
> `HttpTestExchange` which allows access to the underlying exchange. Imagine a
> handler in the test code of the form:
>
>
> server = HttpServerAdapters.HttpTestServer.create(HTTP_2, sslCtx);
> server.addHandler(new Handler(), "/");
> ...
> class Handler implements HttpServerAdapters.HttpTestHandler {
> @Override
> public void handle(final HttpTestExchange exchg) throws IOException {
> final Http2TestExchangeImpl exchgImpl =
> exchg.getUnderlyingExchange(Http2TestExchangeImpl.class);
> ...
>
> Having this ability is convenient because the test no longer is forced to
> explicitly construct the `Http2TestServer` nor the handler is forced to
> implement the `Http2Handler` to get access to the `Http2TestExchange`.
>
> As for the changes in `Http2TestServerConnection`, it cleans up the way we
> `close()` the connection to make sure it's a bit more graceful and allows for
> the accumulated frames to be written out before we close the socket.
>
> A test repeat of more than a 100 against the test/jdk/java/net/httpclient as
> well as complete tier testing with these changes continues to pass without
> any failures.
>
> These changes are mainly needed for better testing of some upcoming bug
> fixes. I could have proposed these changes as part of those specific bug
> fixes, but I expect these test library changes to start getting used in
> additional tests as we go along. Having this test library update as a
> separate JBS issue should allow for backporting this easily, if necessary
> when some test that uses this gets backported.
>
> ---------
> - [x] I confirm that I make this contribution in accordance with the [OpenJDK
> Interim AI Policy](https://openjdk.org/legal/ai).
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.
test/jdk/java/net/httpclient/lib/jdk/httpclient/test/lib/http2/Http2TestServerConnection.java
line 198:
> 196: this.pushStreams = new HashSet<>();
> 197: is = socket.getInputStream();
> 198: os = socket.getOutputStream();
Why did we remove the `Buffered*` wrappers?
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; }`?
test/jdk/java/net/httpclient/lib/jdk/httpclient/test/lib/http2/Http2TestServerConnection.java
line 510:
> 508: }
> 509:
> 510: this.readLoopThread = new Thread(this::readLoop, "readLoop");
Why do we assign the read loop thread to an instance field? I see no usages of
it.
test/jdk/java/net/httpclient/lib/jdk/httpclient/test/lib/http2/Http2TestServerConnection.java
line 514:
> 512:
> 513: this.writeLoopThread = new Thread(this::writeLoop, "writeLoop");
> 514: this.writeLoopThread.start();
Old `ConnectionThread` was `daemon`, the new ones are not. Is this change
intentional?
You might consider using the `Thread.ofPlatform()` fluent builder API.
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.
test/jdk/java/net/httpclient/lib/jdk/httpclient/test/lib/http2/Queue.java line
75:
> 73: q.add(obj);
> 74: // notify anyone waiting for items in the queue
> 75: notifyAll();
`wait()` and `notifyAll()` are very sharp tools to cut yourself. Nice catch and
good correction.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/32173#discussion_r3705491407
PR Review Comment: https://git.openjdk.org/jdk/pull/32173#discussion_r3706236729
PR Review Comment: https://git.openjdk.org/jdk/pull/32173#discussion_r3706191453
PR Review Comment: https://git.openjdk.org/jdk/pull/32173#discussion_r3706225458
PR Review Comment: https://git.openjdk.org/jdk/pull/32173#discussion_r3706215723
PR Review Comment: https://git.openjdk.org/jdk/pull/32173#discussion_r3706040199
PR Review Comment: https://git.openjdk.org/jdk/pull/32173#discussion_r3706156556
