On Wed, 7 Sep 2022 06:36:33 GMT, Jaikiran Pai <[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.
>
> src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java
> line 200:
>
>> 198: debug.log("HTTP connection idle for too long");
>> 199: }
>> 200: HttpTimeoutException hte = new HttpTimeoutException("HTTP
>> connection idle, no active streams. Shutting down.");
>
> Hello Conor,
>
> The javadoc of `HttpTimeoutException` states "Thrown when a response is not
> received within a specified time period.", which isn't what we are using it
> for, here. Do you think we should instead just use a `Exception` (or some
> internal exception type) since this (as far as I can see) won't get
> propagated to the application?
At the point were this exception is generated, it shouldn't be propagated
anywhere, because there should be no open stream on that connection. However,
we could have some hidden race conditions (even if that's not supposed to
happen), where the connection would have been found in the pool by a new
request, where the connection gets closed as idle at the same time where the
request attempts to open a stream on the connection. In that case, the
exception may be propagated, in which case `HttpTimeoutException` (hmmm, or
should it actually be `HttpConnectTimeoutException`?) will be propagated to the
request - which should allow the stack (or at worst the caller?) to retry.
> src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java
> line 201:
>
>> 199: }
>> 200: HttpTimeoutException hte = new HttpTimeoutException("HTTP
>> connection idle, no active streams. Shutting down.");
>> 201: shutdown(hte);
>
> The implementation in `shutdown` has some code which logs errors/exception
> when `error` logging is enabled. Specifically:
>
>
> if (Log.errors()) {
> if (!(t instanceof EOFException) || isActive()) {
> Log.logError(t);
>
> The implementation of `Log.logError` logs the exception stacktrace as well as
> a message which says `ERROR:`. Should we add a specific check in the
> `shutdown` implementation to prevent logging exception stacktraces when a
> connection is being closed due to idle timeout and instead just log it as an
> informational message?
Good catch - yes we don't want the exception to be logged as an error if we're
closing an idle connection.
> src/java.net.http/share/classes/jdk/internal/net/http/HttpClientImpl.java
> line 1695:
>
>> 1693: private Duration getIdleConnectionTimeoutProp() {
>> 1694: // Http 2 in prop name somewhere
>> 1695: String s =
>> Utils.getNetProperty("jdk.httpclient.idleConnectionTimeout");
>
> Is this new system property expected/allowed to be configured even in
> `net.properties` file? The implementation of `Utils.getNetProperty` will
> check even the `net.properties` file for this value, so just checking if
> that's what we want. If we do want this to be configurable in
> `net.properties`, should we updated that file to have a commented section
> which this property and some documentation about it?
>
> Furthermore, I think we should read this value just once and cache the
> `Duration` as a static member of this class instead of reading this more than
> once.
Good point. TBD whether we want this in `net.properties`. We might - but then
it's true that if we do we should have some documentation there.
> test/jdk/java/net/httpclient/http2/IdleConnectionTimeoutTest.java line 94:
>
>> 92: assertEquals(hresp.statusCode(), 200);
>> 93: // Sleep for 4x the timeout value to ensure that it occurs
>> 94: Thread.sleep(800);
>
> Should this use the `Utils.adjustTimeout` to take into account the
> `TIMEOUT_FACTOR`?
I don't think so? We're not waiting on some computational thing here -
everybody should be sleeping ;-)
-------------
PR: https://git.openjdk.org/jdk/pull/10183
