On Fri, 25 Nov 2022 10:22:10 GMT, Daniel Fuchs <[email protected]> wrote:
>> The `ResponseSubscribers.HttpResponseInputStream` class has an assert that >> has been observed firing (rarely) when running the >> `java/net/httpclient/AsyncExecutorShutdown.java` test. >> >> After a thorough analysis of the code and the log failure I am convinced >> that the issue is due to a misplaced assert. >> If cancellation happens asynchronously while the subscriber is being >> subscribed with the lower layers of the stack, the `HttpResponseInputStream` >> might get closed and the `LAST_LIST` buffer might be offered after `closed >> == false` has been observed by the `onSubscribed` method, but before the >> assertion has been checked. The assertion assumes that the queue must be >> empty, but it might not if `close` has been called and the `LAST_LIST` >> buffer has been offered. >> >> Moving the assert from within the synchronized block, to ensure that the >> observed value of `closed` is consistent with the state of the queue, should >> fix it. I have tagged the bug as `noreg-hard`, I'm not sure it would be fair >> to say that `AsyncExecutorShutdown.java` can be used to verify the fix. > > 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 three additional > commits since the last revision: > > - Merge branch 'master' into AsyncExecutorShutdown-8297424 > - Revert assignement of pendingResponseSubscriber to null to avoid race > conditions with ternary operators > - 8297424 Thank you Daniel. The latest changes look good to me. ------------- Marked as reviewed by jpai (Reviewer). PR: https://git.openjdk.org/jdk/pull/11332
