RFR: 8308310: HttpClient: Avoid logging or locking from within synchronized blocks [v8]

[Jaikiran Pai]

On Tue, 23 May 2023 15:11:10 GMT, Daniel Fuchs <dfuchs at openjdk.org> 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 <turbanoff at gmail.com>
>  - Update src/java.net.http/share/classes/jdk/internal/net/http/ConnectionPool.java
>    
>    Co-authored-by: Andrey Turbanov <turbanoff at gmail.com>
>  - Update test/jdk/java/net/httpclient/AuthFilterCacheTest.java
>    
>    Co-authored-by: Andrey Turbanov <turbanoff at gmail.com>
>  - ... 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