On Tue, 18 Oct 2022 08:53:52 GMT, Conor Cleary <[email protected]> wrote:
>> src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java
>> line 730:
>>
>>> 728: if (idleConnectionTimeoutEvent != null) {
>>> 729: Log.logTrace("idleConnectionTimeout timeout fired,
>>> shutting down connection: {0}", t.getMessage());
>>> 730: } else if (!(t instanceof EOFException) || isActive()) {
>>
>> That doesn't seem right to me. If `idleConnectionTimeoutEvent` is not null
>> it means that we have armed the timeout, it doesn't mean that it has fired.
>> We're not shutting down the connection, the connection is simply idle, and
>> thus we should log exceptions.
>
> Good point, the setup here seems that an idleConnectionTimeoutEvent being not
> null could cause its log to be incorrectly logged (like you said, it doesn't
> mean it has fired.
>
> I'm thinking maybe here I could include an additional condition in the form
> of a check to see if the idleConnectionTimeoutEvent has fired or not as well
> as the null check.
With the latest changes, I'm proposing the additional check for the timeout
firing as well as the null check. Also logging with logError as supposed to
using logTrace as the idleConnectionTimeoutEvent produceces a
ConnectTimeoutException
-------------
PR: https://git.openjdk.org/jdk/pull/10183
