On Wed, 16 Aug 2023 12:23:27 GMT, Darragh Clarke <[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 498: > 496: // we use the ReferenceTracker API rather than HttpClient::close > here, > 497: // because these tests inject faults by throwing inside callbacks, > which > 498: // is more likely to get HttpClient::close wedged until jtreg times > out. Is this comment and the decision to not use `HttpClient.close()` in this test relevant to this test case? In `AbstractThrowingPublishers`, which is where this comment initially resided like you note in the PR description, I think it was relevant. I haven't thoroughly looked at the tests in this `ResponsePublisher` test, so unsure if we can't/shouldn't use `HttpClient.close()` in these test methods. 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)? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/15307#discussion_r1302575954 PR Review Comment: https://git.openjdk.org/jdk/pull/15307#discussion_r1302573457
