On Thu, 18 Aug 2022 06:41:49 GMT, Jaikiran Pai <j...@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
