RFR: 8288717: Add a means to close idle connections in HTTP/2 connection pool [v4]

[Daniel Fuchs]

On Mon, 24 Oct 2022 16:44:15 GMT, Conor Cleary <ccleary at openjdk.org> wrote:

>> **Issue**
>> When using HTTP/2 with the HttpClient, it can often be necessary to close an idle Http2 Connection before a server sends a GOAWAY frame. For example, a server or cloud based tool could close a TCP connection silently when it is idle for too long resulting in ConnectionResetException being thrown by the HttpClient.
>> 
>> **Proposed Solution**
>> A new system property, `jdk.httpclient.idleConnectionTimeout`, was added and is used to specify in Milliseconds how long an idle connection (idle connections are those which have no currently active streams) for the HttpClient before the connection is closed.
>
> Conor Cleary has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8288717: Updated test and property to use seconds

Changes requested by dfuchs (Reviewer).

src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java line 736:

> 734:         }
> 735:         if (Log.errors()) {
> 736:             if (idleConnectionTimeoutEvent != null && idleConnectionTimeoutEvent.isFired()) {

The reference for idleConnectionTimeoutEvent should be captured in the synchronized block above.


        IdleConnectionTimeoutEvent idleConnectionTimeoutEvent;
        synchronized (this) {
            if (closed == true) return;
            closed = true;
            idleConnectionTimeoutEvent = this.idleConnectionTimeoutEvent;
        }
        if (Log.errors()) {
            if (idleConnectionTimeoutEvent != null && idleConnectionTimeoutEvent.isFired()) { ... }

src/java.net.http/share/classes/jdk/internal/net/http/HttpClientImpl.java line 1703:

> 1701:         }
> 1702:         return null;
> 1703:     }

If the property is not present or doesn't parse to an Integer value then the default value should be used. `Utils.getIntegerNetProperty` - which ConnectionPool uses, does that. Possibly we should also define what happens is the value is 0 or negative: we should probably be using the default value in that case too.

The property should also be read only once - like in ConnectionPool. I believe the KEEP_ALIVE constant that is currently in ConnectionPool should be moved here. Then its value can then be shared by this code & ConnectionPool. Not sure whether we'd need an `Optional<Duration> idleConnectionTimeout() ` method any more then. The IdleConnectionTimeout could just be initialized with `Duration.ofSeconds(HttpClientImpl.KEEP_ALIVE)`. Or possibly we could have a `Duration idleConnectionTimeout()` method.

It would be good to refactor ConnectionPool and Http2Connection to use the same code for getting the HttpTimeout value - and if we expose a method that returns a Duration then maybe ConnectionPool should use that too.

-------------

PR: https://git.openjdk.org/jdk/pull/10183