On Tue, 18 Oct 2022 08:51:43 GMT, Jaikiran Pai <[email protected]> wrote:
>> Daniel Fuchs has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Update copyright years
>
> test/jdk/java/net/httpclient/SmallTimeout.java line 86:
>
>> 84: HttpClient client = HttpClient.newHttpClient();
>> 85: ReferenceTracker.INSTANCE.track(client);
>> 86: Reference<HttpClient> reference = new WeakReference<>(client);
>
> I don't understand the use of `WeakReference` here. I see that we have a
> `ReachabilityFence` for the `client` in the `finally` block of this test
> where we then null out `client`. So, if I'm understanding this right, this
> `WeakReference` is essentially a no-op. i.e. we probably don't need it
> because we are anyway holding on to the `client` for the lifetime of this
> test program?
The reference is passed to another thread but should remain alive until that
other thread has terminated - which is ensured by waiting for the executor to
shutdown. What happens here is that the test failed because the other threads
started in this block were keeping the reference alive. I'm using a reference
here because passing `client` to a lambda makes `client` final and that
prevents me from nulling out the `client` variable before calling `gc`. Using a
weak reference solves the issue (since I don't have to null it and it won't
keep the client alive).
> test/jdk/java/net/httpclient/SmallTimeout.java line 184:
>
>> 182:
>> 183: executor.shutdownNow();
>> 184: if (!executor.awaitTermination(500, TimeUnit.MILLISECONDS))
>> {
>
> Would this timeout value (whatever value we decide) introduce any potential
> intermittent failures, especially on CI systems? Would it perhaps be better
> to just call `executor.close()` after that call to `executor.shutdownNow()`,
> so that if the tasks really don't complete, then the jtreg test timeout will
> make it fail with a timeout and we don't have to decide what timeout is a
> good timeout?
Good point!
-------------
PR: https://git.openjdk.org/jdk/pull/10659
