On Tue, 21 Feb 2023 16:28:52 GMT, Daniel Fuchs <[email protected]> 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
