On Thu, 10 Mar 2022 17:05:32 GMT, Daniel Fuchs <[email protected]> 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
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` to synchronize
on like in the `Http2Connection#shutdown` method. So do you think this
synchronization on `Http2ClientImpl` here, while calling `c.isOpen()` is
necessary?
-------------
PR: https://git.openjdk.java.net/jdk/pull/7776
