On Mon, 24 Oct 2022 16:44:15 GMT, Conor Cleary <[email protected]> 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
