On Wed, 23 Nov 2022 19:38:50 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. Hello Daniel, the change where we move the assert into the synchronized block looks fine to me. Looking at the code I think the check to see that the `buffers` can accomodate at least 2 elements (when `onSubscribe` is called) looks correct to me because the `buffers` will be initialized in the constructor of `HttpResponseInputStream` with a capacity of at least 2. So the assert itself looks correct. This PR also has changes in the `Stream` class. Some of those changes are typo related fixes, so that's fine. However, it also has other functional changes. Is that intentional and related? ------------- PR: https://git.openjdk.org/jdk/pull/11332
