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

[Jaikiran Pai]

On Wed, 7 Sep 2022 17:51:47 GMT, Daniel Fuchs <dfuchs at openjdk.org> wrote:

>> 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.

I think `HttpConnectTimeoutException` sounds like a more relevant exception, even if its javadoc has reference to `... is not successfully established within a specified time period.`

>> 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 ;-)

You are right indeed - the `TIMEOUT_FACTOR` shouldn't play a role here.

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

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