On Mon, 28 Nov 2022 15:05:43 GMT, Jaikiran Pai <[email protected]> wrote:
>> 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. Oh! Darn. Of course. Thanks for catching that! Fixed on last commit. ------------- PR: https://git.openjdk.org/jdk/pull/11294
