On Fri, 11 Mar 2022 05:28:36 GMT, Jaikiran Pai <j...@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