On Wed, 26 Jul 2023 06:06:13 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 incrementally with one additional > commit since the last revision: > > minor cleanup to simplify the code Hi Jaikiran, thanks for taking care of this. Changes look good, except for `volatile` usage that looks unnecessary. src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java line 200: > 198: > 199: // expected to be accessed/updated with "stateLock" being held > 200: private volatile boolean cancelled; `cancelled` doesn't need to be volatile if it's consistently protected by the same lock ------------- PR Review: https://git.openjdk.org/jdk/pull/15012#pullrequestreview-1549296544 PR Review Comment: https://git.openjdk.org/jdk/pull/15012#discussion_r1275926002
