On Fri, 11 Mar 2022 10:12:46 GMT, Daniel Fuchs <[email protected]> wrote:
>> 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).
Thank you - that helped.
-------------
PR: https://git.openjdk.java.net/jdk/pull/7776
