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

[Jaikiran Pai]

On Thu, 10 Mar 2022 17:05:32 GMT, Daniel Fuchs <dfuchs at 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 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