On Tue, 7 Feb 2023 16:08:53 GMT, Daniel Fuchs <dfu...@openjdk.org> wrote:
>> ### Description ###
>> Intermittent failures of this test are observed on frequent `HttpClient`
>> test runs. The test checks that the same connection is not used twice for
>> two seperate requests if an Idle Connection Timeout occurs by verifying that
>> the client-side port does not use the same port. It also verifies that when
>> an Idle Connection Timeout does not occur, the same connection is used by
>> verifying that the port used in both requests is the same.
>>
>> The issue here is that there is no guarantee that the ports used will not be
>> the same for when an Idle Connection Timeout occurs and so the test
>> will/does fail intermittently.
>>
>> ### Summary of Changes & Justification ###
>> Instead of comparing the post numbers of the connections used for each
>> request in all test cases, the connections themselves are now compared with
>> calls to `hashCode()` like so. The connection instances themselves are
>> accessed by using a customised `ExchangeSupplier` for the `Http2TestServer`.
>
> test/jdk/java/net/httpclient/http2/IdleConnectionTimeoutTest.java line 179:
>
>> 177: int cmp = Integer.compare(firstConnectionHash,
>> secondConnectionHash);
>> 178:
>> 179: if (cmp < 0 | cmp > 0) {
>
> In other words, `if (cmp != 0)` ? ;-)
>
> It may be simpler and less error prone to save the connection (as an Object)
> in the handler.
>
> volatile Object previousConnection = null;
>
>
> Then the condition become:
>
> if (previousConnection == null) {
> previousConnection = exch.getTestConnection();
> exch.sendResponseHeaders(200, 0);
> } else {
> var secondConnection = exch.getTestConnection();
>
> if (previousConnection != secondConnection) {
> ....
I could be wrong but I think I might have tried that before. What you're
calling `previousConnection` in the sample code you've given gets set to `null`
in the time between the first and second requests during the tests where a
timeout fires. Which does serve to test that the connections are different to
be fair but maybe for some reason `previousConnection` could be null that is
unrelated to it being closed by the test server. For example
`createConnection()` might close early due to a Reset Stream or Protocol Error.
I know that's very edge case but its the reason I compared using the hash code.
I'm open to changing it though of course! Might be less error prone as you say.
Also, very fair on the `if (cmp != 0)` suggestion 😅 I'll definitely shorten it
to that if I stick with the comparison as it is right now.
-------------
PR: https://git.openjdk.org/jdk/pull/12457
