On Fri, 11 Mar 2022 10:07:21 GMT, Daniel Fuchs <dfu...@openjdk.org> wrote:
>> src/java.net.http/share/classes/jdk/internal/net/http/Http2ClientImpl.java
>> line 162:
>>
>>> 160:
>>> 161: String key = c.key();
>>> 162: synchronized(this) {
>>
>> Hello Daniel, it's not fully clear to me why this synchronized block is
>> needed. Of course, this synchronized block has existed even before this PR,
>> so this isn't a comment about the changes in the PR.
>> I see that in this synchronized block we are updating the `connections` in
>> one single `putIfAbsent` operation. The `connections` happens to be an
>> instance of `ConcurrentHashMap`. Now, in this PR, additionally we are also
>> including a `c.isOpen()` check in this synchronized block. However, the
>> `Http2Connection#isOpen()` method deals with a `volatile closed` member of
>> the `Http2Connection` and it uses an instance of `Http2Connection` (i.e. a
>> different object monitor) to synchronize on when updating the `closed`
>> member, like in the `Http2Connection#shutdown` method. So do you think this
>> synchronization on `Http2ClientImpl` here, while calling `c.isOpen()` is
>> necessary?
>
> I'm not completely sure of the purpose of the synchronized block here - but
> I'd rather not change this in this PR (and I'd rather not change it at all
> unless it proves to cause issues). As for the second check for isOpen() - I
> included it here because 1. the check is cheap, and 2. since there is a
> synchronized block - some time may have elapsed waiting for the lock - so
> rechecking here before putting the connection into the pool would save adding
> a dead connection to the pool.
Sounds fine to me. One last question - I suspect you didn't include the
additional `c.finalStream()` within this synchronized block because that call
is relatively expensive (with `finalStream()` being `synchronized`)?
-------------
PR: https://git.openjdk.java.net/jdk/pull/7776
