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 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