On Wed, 23 Aug 2023 07:05:48 GMT, Jaikiran Pai <[email protected]> wrote:
>> Currently `ResponsePublisher` occasionally fails due to unreleased resources. >> Updated test based on >> [AbstractThrowingPushPromises](https://github.com/openjdk/jdk/blob/b80001de0c0aeedeb412430660a4727fc26be98b/test/jdk/java/net/httpclient/AbstractThrowingPushPromises.java#L343) >> to ensure that non-shared clients get closed before further iterations run, >> this should limit the max number of clients that remain alive during the >> test. >> >> I ran tiers 1-3 and everything is passing > > test/jdk/java/net/httpclient/ResponsePublisher.java line 506: > >> 504: System.out.println(now() + "waiting for client to shutdown: " + >> tracker.getName()); >> 505: System.err.println(now() + "waiting for client to shutdown: " + >> tracker.getName()); >> 506: var error = TRACKER.check(tracker, 10000); > > Hello Darragh, 10 seconds looks a bit too large, even if it's the max time to > wait. Before this change the graceDelayms was 500 milli seconds. So this > appears to be a big jump. Perhaps we should start with something like 2 or 5 > seconds (an arbitrary number, I admit)? It shouldn't take 10 seconds for the client to shutdown, so it shouldn't matter that we use a large timeout. I we used HttpClient::close() we would wait forever (that is - we would wait for as long as it takes). I believe 500ms to wait for the GC to kick in was a bit optimistic for cases where the machine is overloaded by other tasks. Waiting for a client to be gc'ed or closed until we create the next one should help, as it will ensure there are no lingering resources waiting to be released. The advantage of using close() is that it is much cleaner and simpler. The disadvantage is that if it fails in timeout there, you have no clue about what prevented close() to finish. In opposition the tracker will provide diagnosis if the client fails to close in the imparted delay. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/15307#discussion_r1303276612
