> 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 introduce a > new test w...
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 ------------- Changes: - all: https://git.openjdk.org/jdk/pull/15012/files - new: https://git.openjdk.org/jdk/pull/15012/files/05b992e6..c74ad782 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15012&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15012&range=01-02 Stats: 1985 lines in 59 files changed: 1147 ins; 321 del; 517 mod Patch: https://git.openjdk.org/jdk/pull/15012.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15012/head:pull/15012 PR: https://git.openjdk.org/jdk/pull/15012
