On Tue, 7 Feb 2023 16:52:43 GMT, Conor Cleary <ccle...@openjdk.org> wrote:
>> 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.
I don't see how using the hash wouldn't cause the exact same issue. Also you
really need volatile here, since two different requests are going to be handled
by two different threads.
-------------
PR: https://git.openjdk.org/jdk/pull/12457
