On Tue, 23 May 2023 15:11:10 GMT, Daniel Fuchs <[email protected]> wrote:
>> Please find here a change that revisits usage of monitors in the HttpClient. >> >> With Virtual Threads now part of the platform it should be possible to pass >> a newVirtualThreadPerTaskExecutor to the HttpClient. Logging, when enabled, >> and when called from a synchronized block, can cause the carrier thread to >> get pinned in case of contention when printing through the underlying >> PrintStream. >> >> This change aims at avoiding situations where the carrier threads might get >> pinned. > > Daniel Fuchs has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 11 commits: > > - Merge branch 'master' into HttpClient-Logging-8308310 > - Merge branch 'master' into HttpClient-Logging-8308310 > - Fix whitespace > - make stateLock final > - Add debug traces to ExpectContinueTest.java > - failedRef should be final > - Align parameters > - Update > src/java.net.http/share/classes/jdk/internal/net/http/Http1Response.java > > Co-authored-by: Andrey Turbanov <[email protected]> > - Update > src/java.net.http/share/classes/jdk/internal/net/http/ConnectionPool.java > > Co-authored-by: Andrey Turbanov <[email protected]> > - Update test/jdk/java/net/httpclient/AuthFilterCacheTest.java > > Co-authored-by: Andrey Turbanov <[email protected]> > - ... and 1 more: https://git.openjdk.org/jdk/compare/c0c4d771...c5d2f1f2 src/java.net.http/share/classes/jdk/internal/net/http/Http1Response.java line 150: > 148: > 149: public void tryRelease() { > 150: if (STATE.compareAndSet(this, (byte)0x01, (byte)0x03)) { Nit - a space is missing between the cast and the value for the second and third parameters. src/java.net.http/share/classes/jdk/internal/net/http/Http1Response.java line 158: > 156: debug.log("ref count for %s already released", > client); > 157: } > 158: state |= 0x02; It looks like previously (before the changes in this PR), the `tryRelease` could incorrectly set this `state` to `0x02` even when the `state` was `0`. From what I see in this change, that seems to have been addressed now. src/java.net.http/share/classes/jdk/internal/net/http/Http2ClientImpl.java line 323: > 321: private void unlock() { > 322: lock.unlock(); > 323: } Any specific reason for creating private wrapper methods instead of just using `lock.lock()` and `lock.unlock()` at the call sites? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/14038#discussion_r1203894064 PR Review Comment: https://git.openjdk.org/jdk/pull/14038#discussion_r1203892337 PR Review Comment: https://git.openjdk.org/jdk/pull/14038#discussion_r1203896133
