On Tue, 4 Aug 2026 12:19:30 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).
>
> Jaikiran Pai has updated the pull request incrementally with one additional
> commit since the last revision:
>
> use Utils.getDebugLogger() instead of writing directly to System.err
test/jdk/java/net/httpclient/lib/jdk/httpclient/test/lib/http2/BodyOutputStream.java
line 189:
> 187: }
> 188: sendReset(resetErrorCode);
> 189: }
Maybe we should keep this method even if it's not used at the moment. Possibly
`Http2TestExchangeImpl::resetStream` should call this instead of `sendReset`.
test/jdk/java/net/httpclient/lib/jdk/httpclient/test/lib/http2/Http2TestServerConnection.java
line 367:
> 365: // await termination of the writeLoop to make sure any
> accumulated
> 366: // frames are written out before the socket is closed
> 367: this.writeLoopThread.join();
May `writeLoopThread` be null here? What if close() is called from within the
writeLoopThread?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/32173#discussion_r3719215883
PR Review Comment: https://git.openjdk.org/jdk/pull/32173#discussion_r3719288731
