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

[Jaikiran Pai]

On Fri, 11 Mar 2022 10:12:46 GMT, Daniel Fuchs <dfuchs at openjdk.org> 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