> 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 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 13 additional commits since the 
last revision:

 - create the writeLoopThread when the connection is created
 - don't remove the unused reset method
 - merge latest from master branch
 - merge latest from master branch
 - use Utils.getDebugLogger() instead of writing directly to System.err
 - 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
 - ... and 3 more: https://git.openjdk.org/jdk/compare/80e3fab1...b753d786

-------------

Changes:
  - all: https://git.openjdk.org/jdk/pull/32173/files
  - new: https://git.openjdk.org/jdk/pull/32173/files/fec974bc..b753d786

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=32173&range=03
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=32173&range=02-03

  Stats: 5723 lines in 103 files changed: 3492 ins; 2054 del; 177 mod
  Patch: https://git.openjdk.org/jdk/pull/32173.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/32173/head:pull/32173

PR: https://git.openjdk.org/jdk/pull/32173

Reply via email to