On Wed, 7 Sep 2022 17:51:47 GMT, Daniel Fuchs <[email protected]> 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
