On Thu, 27 Jul 2023 10:03:06 GMT, Jaikiran Pai <[email protected]> wrote:
>> Can I please get a review of this change which proposes to fix the issue >> noted in https://bugs.openjdk.org/browse/JDK-8312433? >> >> In JDK 20, we introduced a way to identify and close idle HTTP2 connections >> in the HttpClient implementation through >> https://bugs.openjdk.org/browse/JDK-8288717. Whenever a HTTP2 stream gets >> closed and if there are no other streams on the connection, we create a idle >> timeout event that times out after a (configurable) duration. After the >> event has been registered, if any more streams get opened on that >> connection, then the event is cancelled. If no streams are created during >> that period, then the event fires and we close the connection. >> We have a race condition in this implementation, as noticed in >> https://bugs.openjdk.org/browse/JDK-8312433. When a HTTP2 connection is >> created, we pool that connection. Any subsequent request against the same >> target server will reuse this pooled connection if it's still open. When >> user code issues a request, we checkout a connection from this pool (if >> relevant) and then use that connection to create a HTTP2 stream and issue >> the request. This sequence has a small window of time, where a pooled >> connection which has a idle timeout event scheduled (because it had no >> active streams) is handed out of the pool and before it can create a new >> HTTP2 stream, gets closed (concurrently by the idle connection timeout >> event). This results in the HTTP2 stream creation to fail with an exception >> that effectively fails the user initiated request. >> >> The commit in this PR addresses it by introducing an internal method >> `tryReserveForPoolCheckout()` on the `Http2Connection` which is responsible >> for co-ordinating with the idle connection management to ensure that if this >> connection does get handed out of the pool, then the idle connection event >> doesn't end up closing it. >> >> A new jtreg test has been added to test this issue. This test consistently >> reproduces the issue without this fix and passes with this fix. Ideally, a >> new test method could have been added to the existing >> `test/jdk/java/net/httpclient/http2/IdleConnectionTimeoutTest.java` but that >> test has multiple `@run` with different timeout values ranging all the way >> upto 30 seconds and it focuses more on the timeout value parsing and >> honouring that value. Introducing this test method in that existing test >> would end up testing this against all those values for no reason, plus would >> also end up increasing the duration of that test. So I decided to introd... > > Jaikiran Pai has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains five additional > commits since the last revision: > > - minor - add a code comment in the test method explaining the use of the > magic number > - merge latest from master branch > - Daniel's review - no need for volatile > - minor cleanup to simplify the code > - 8312433: HttpClient request fails due to connection being considered idle > and closed LGTM. Thanks! ------------- Marked as reviewed by djelinski (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15012#pullrequestreview-1549487433
