RFR: 8301243: java/net/httpclient/http2/IdleConnectionTimeoutTest.java intermittent failure

[Conor Cleary]

On Tue, 7 Feb 2023 16:08:53 GMT, Daniel Fuchs <dfuchs at 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