On Mon, 3 Aug 2026 17:17:45 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/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. I've removed it as a field in the updated PR. It may be that the readLoop thread might need some attention to how we manage that, but it isn't necessary now or in this PR. > 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. Good catch about the daemon nature of the threads before this change. They should be daemon. I've updated the PR accordingly. > You might consider using the Thread.ofPlatform() fluent builder API. I resisted doing that so as to allow this test-only change to be easily backported to any older update releases. On the other hand, I don't think it would be too much of a hassle to use the old style APIs if there are backport conflicts. So if you suggest we should use these new APIs, I'll update accordingly. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/32173#discussion_r3712087112 PR Review Comment: https://git.openjdk.org/jdk/pull/32173#discussion_r3712072621
