RFR: 8254786: java/net/httpclient/CancelRequestTest.java failing intermittently [v3]

[Jaikiran Pai]

On Fri, 11 Mar 2022 10:07:21 GMT, Daniel Fuchs <dfuchs at 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