RFR: 8292381: java/net/httpclient/SpecialHeadersTest.java fails with "ERROR: Shutting down connection: HTTP/2 client stopped"

[Daniel Fuchs]

On Thu, 18 Aug 2022 06:41:49 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> Please find here a change that improves SpecialHeadersTest. This test creates a large amount of ephemeral clients and has been observed running out of heap space in our CI once. This change updates the test to wait for the previous HttpClient to be eligible for garbage collection before it creates a new one. It also verifies that no outstanding operation are still running on the client by the time the client is released.
>
> test/jdk/java/net/httpclient/HttpServerAdapters.java line 113:
> 
>> 111:             if (this == o) return true;
>> 112:             if (!(o instanceof HttpTestRequestHeaders headers)) return false;
>> 113:             return Objects.equals(entrySet(), headers.entrySet());
> 
> This code confused me a bit initially. Each of the subclasses of `HttpTestRequestHeaders` has a `headers` private member and I misread this line to be comparing the headers entrySet with itself. Would it be better if we renamed this local variable (in the instanceof line above) to `otherHeaders` to avoid any confusion?

Why doesn't this comment appears when I click on the "Files Changed" tab? Strange.
Anyway - yes - good point - will change `headers` to `other` - I agree it's confusing.

> test/jdk/java/net/httpclient/SpecialHeadersTest.java line 370:
> 
>> 368:                     System.gc();
>> 369:                     var error = TRACKER.check(500);
>> 370:                     if (error != null) throw error;
> 
> Given slowness on CI systems, would it be better to use the jtreg timeout factor here (and other similar places in other test methods) to adjust the `500ms` timeout? Or better still, perhaps just log a warning instead of throwing an error? After all this test's main goal is to verify the request headers and in that context it should be OK if some HttpClient(s) aren't gc collected by the time the test completes?

I haven't observed any issue yet with using a `500ms`. Here or in other tests. And I do want to throw an error if a client doesn't cleanly exits. So if we get a failure here - we should look at what prevented the client from exiting - and if it looks like it might be that the GC didn't get enough cycle then we would raise the timeout.

> test/jdk/java/net/httpclient/SpecialHeadersTest.java line 385:
> 
>> 383:         final URI uri = URI.create(uriString);
>> 384: 
>> 385:         HttpClient client = newHttpClient("testHomeMadeIllegalHeader", sameClient);
> 
> Previously before this change, this test method wasn't using the `sameClient` param while creating the new client and instead was creating it always. I don't know if that was intentional (in fact it wasn't even using the `headerNameAndValue` param being passed to it). Is this change to use the `sameClient` param while creating this client here, intentional?

yes - before that the method would be running twice with a brand new client. I believe that was an oversight. I don't think that sameClient=true|false matters much here, which probably explains why the sameClient parameters was ignored. We could create a special provider that doesn't pass sameClient, but since this method should be quite short I decided to simply change it to take the value of `sameClient` into account.

-------------

PR: https://git.openjdk.org/jdk/pull/9908