On Thu, 10 Mar 2022 17:05:32 GMT, Daniel Fuchs <dfu...@openjdk.org> wrote:
>> Please find enclosed a patch that solves an intermittent issue detected by
>> the CancelRequestTest.java
>>
>> If during an HTTP upgrade from HTTP/1.1 to HTTP/2, the request is cancelled
>> after the Http2Connection has been created, and the handshake has proceeded,
>> and the response headers to the upgrade have been received, but before the
>> HTTP/2 connection is offered to the HTTP/2 connection pool, the underlying
>> TCP connection might get closed at a time where it won't be noticed
>> immediately, resulting in putting a "dead" HTTP/2 connection in the pool.
>> The next request to the same server will then fail with
>> "ClosedChannelException".
>>
>> The fix is to check the state of the underlying TCP connection before
>> offering the HTTP/2 connection to the pool, and when retrieving it from the
>> pool, and disabling the "connectionAborter" (which is there to abort the
>> connection in case of connect timeout, or cancellation before connect is
>> done) before offering the connection to the pool as well.
>
> 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
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?
-------------
PR: https://git.openjdk.java.net/jdk/pull/7776
