RFR: 8299338: AssertionError in ResponseSubscribers$HttpResponseInputStream::onSubscribe [v2]

[Jaikiran Pai]

On Tue, 21 Feb 2023 16:28:52 GMT, Daniel Fuchs <dfuchs at openjdk.org> wrote:

>> This fix revisits an assertion that has been observed failing in ResponseSubscribers.HttpResponseInputStream.
>> 
>> The HttpResponseInputStream has the logic to wait until a buffer has been taken out of the queue before requesting a new one. Therefore there should at most be one byte buffer in the queue, except in the case of error (or asychronous close), where a LAST_LIST sentinel is inserted in the queue to unblock the consumer of the input stream, which might be blocked in queue.take().
>> 
>> The  HttpResponseInputStream thus asserts, when processing a subscription, that the remaining capacity of the queue should be greater than 1 (unless already closed), to ensure that there will be room for the LAST_LIST sentinel. However, in case of asynchronous shutdown of the executor, it's possible that the subscriber will be marked failed and the LAST_LIST sentinel inserted into the queue before/at the same time that the subscription is processed.
>> 
>> This fix proposes to relax the assertion to only fire if closed == false and failed == null and capacity <= 1 when processing the subscription
>
> Daniel Fuchs has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Call tryRegister() before markSubscribed()

Thank you for these changes, Daniel. This looks good to me.

I see that now if the tryRegister() doesn't actually register the subscriber and instead the onError() of the subscriber gets called, then in the propagateError() we first mark the subscriber subscribed and then call the onError on the underlying subscriber. So once tryRegister() returns, on the next line the call to markSubscribed() will return false and we then cancel the subscription. So this looks fine to me.

Happy to see this intermittent test failure fixed.

-------------

Marked as reviewed by jpai (Reviewer).

PR: https://git.openjdk.org/jdk/pull/12677