On Fri, 25 Nov 2022 12:15:47 GMT, Daniel Fuchs <[email protected]> wrote:
>> The java/net/httpclient/SpecialHeadersTest.java test has been observed >> failing once, due to a selector thread still running after all operations >> had terminated. This change slightly alter the test logic to check the >> shared client only at the end of the test when all methods have been >> executed, and focussed the checks performed by a test method to the only >> clients that this method is using. This may or may not prevent the observed >> issue to ever reproducing, but should at least provide more information on >> where the selector manager thread is at the time it is observed running, >> should the test fail again: >> the logic of the HttpClient should have woken up the selector when the last >> operation finished, which should have ensured a prompt termination of the >> thread. >> In addition this change fixes a small race condition in the logic that >> attempts to decide if the selector is still alive: the selector should be >> considered alive until its run() method has been called and has exited (or >> Thread::start has thrown). > > Daniel Fuchs has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains five additional > commits since the last revision: > > - Merge branch 'master' into SpecialHeadersTest-8297200 > - Update test/jdk/java/net/httpclient/ReferenceTracker.java > > Co-authored-by: Andrey Turbanov <[email protected]> > - Fixed race condition in detecting if selector is still alive > - 8297200 > - 8297200 test/jdk/java/net/httpclient/SpecialHeadersTest.java line 379: > 377: if (error != null) { > 378: if (thrown != null) error.addSuppressed(thrown); > 379: throw error; If I am reading this code correctly, I think, we might end up losing (and not propagating) the original `thrown` if `error == null`. i.e. we might end up marking a test as passed even when an exception was thrown (and there were no more references left of the client). Perhaps add a else block: if (error != null) { ... } else if (thrown != null) { throw thrown; } Similar comment for the other places in this PR where this construct is being changed. ------------- PR: https://git.openjdk.org/jdk/pull/11294
