On Tue, 25 Jul 2023 08:50:58 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 introduce a > new test w... This pull request has now been integrated. Changeset: 486c7844 Author: Jaikiran Pai <[email protected]> URL: https://git.openjdk.org/jdk/commit/486c7844f902728ce580c3994f58e3e497834952 Stats: 320 lines in 3 files changed: 277 ins; 12 del; 31 mod 8312433: HttpClient request fails due to connection being considered idle and closed Reviewed-by: djelinski ------------- PR: https://git.openjdk.org/jdk/pull/15012
