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

[Daniel Fuchs]

On Fri, 11 Mar 2022 05:28:36 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> Daniel Fuchs has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Copyright years
>
> 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.

> src/java.net.http/share/classes/jdk/internal/net/http/Http2ClientImpl.java line 170:
> 
>> 168:             Http2Connection c1 = connections.putIfAbsent(key, c);
>> 169:             if (c1 != null) {
>> 170:                 c.setFinalStream();
> 
> This comment isn't related to the change in this PR, but is this a typo here? Should it instead be `c1.setFinalStream()`? Reading up on the `finalStream` javadoc in `Http2Connection` this flag indicates that no new streams should be opened on this connection and the connection should be closed once this final stream completes. Is that an accurate understanding?
> There's an additional comment on top of this `Http2ClientImpl#offerConnection` which states:
>>
>> Cache the given connection, if no connection to the same
> destination exists. If one exists, then we let the initial stream
> complete but allow it to close itself upon completion.
>    ...
> 
> So if I'm reading that last part correctly in that comment, what it seems to suggest is that we should be allow the initial connection (which is already in cache) to close on completion i.e. mark the `finalStream` flag on the previously cached connection, which in this case is `c1`. So I would expect this to be `c1.setFinalStream()` instead of `c.setFinalStream()`.
> 
> Am I misunderstanding this code and comments?

It's not a typo: we want only one Http2Connection of the same type to a given server in the pool (the key encodes the connection type). putIfAbsent returns the previous connection. The logic is to close the *new* connection after the first exchange completes. In other words: the first connection that gets into the pool wins. Keep in mind that the Http2Connection pool is very different to the HTTP/1.1 connection pool. Because HTTP/2 supports multiplexing, Http2Connection stay in the pool until they are closed - the pool contains *active* connections (whereas in HTTP/1.1 it contains *idle* connections).

-------------

PR: https://git.openjdk.java.net/jdk/pull/7776