On Fri, 11 Mar 2022 05:28:36 GMT, Jaikiran Pai <[email protected]> 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
