On Thu, 18 Aug 2022 06:52:41 GMT, Jaikiran Pai <[email protected]> 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/SpecialHeadersTest.java line 195:
>
>> 193: out.printf(now() + "Task %s failed: %s%n", id, t);
>> 194: err.printf(now() + "Task %s failed: %s%n", id, t);
>> 195: FAILURES.putIfAbsent("Task " + id, t);
>
> Hello Daniel, is the `putIfAbsent` intentional here? I wouldn't expect the
> task id to repeat here (or would there be that many tasks that the `long` id
> value overflows and repeats?).
It's copy-pasted from other tests that use the same technique. Mainly we want
to make sure we get the first error. I agree that `putIfAbsent` might not been
necessary but it doesn't hurt either: it makes it clear that the value
associated with the key will not be overridden.
> test/jdk/java/net/httpclient/SpecialHeadersTest.java line 284:
>
>> 282: // will be an upgrade
>> 283: if (shared != null) {
>> 284: TRACKER.track(shared);
>
> Is it intentional that we track a shared client when it is being reset,
> instead of tracking it when we create it (a few lines later)? This is unlike
> a non-shared instance which we track when we create one.
Yes - because the tracker will complain if any of the tracked client is still
alive when check() is called. So the idea is to start tracking the shared
client when we're no longer using it.
-------------
PR: https://git.openjdk.org/jdk/pull/9908
