RFR: 8302635: Race condition in HttpBodySubscriberWrapper when cancelling request [v2]

[Daniel Fuchs]

On Fri, 17 Feb 2023 13:12:47 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> Daniel Fuchs has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Fixed boolean cancelled() {}
>
> src/java.net.http/share/classes/jdk/internal/net/http/common/HttpBodySubscriberWrapper.java line 70:
> 
>> 68:     final long id = IDS.incrementAndGet();
>> 69:     final BodySubscriber<T> userSubscriber;
>> 70:     volatile int state;
> 
> Hello Daniel, should we make this `private` and all the newly introduced state values, too? Or maybe you want to leave it package private to be consistent with some other fields here?

Good idea.

> src/java.net.http/share/classes/jdk/internal/net/http/common/HttpBodySubscriberWrapper.java line 172:
> 
>> 170:             // and closing the connection.
>> 171:             userSubscriber.onError(t);
>> 172:         }
> 
> I haven't fully wrapped my head around the possible flow of the user subscription, but is there a possiblity where this call to `onError(...)` here results in an reentrant call to this `propagateError()`? For that matter, not just reentrant but perhaps from a different thread concurrently? The reason I ask is, should we call this onError just once? The `java.util.concurrent.Flow.Subscriber.onError(Throwable)` method says this:
>> Method invoked upon an unrecoverable error encountered by a
>> Publisher or Subscription, after which no other Subscriber
>> methods are invoked by the Subscription. 
> 
> So I'm wondering if we should maintain some state to only invoke it once?

This is the purpose of this class: complete() and propagateError() should ensure that onError() is only called once in the wrapped subscriber. The markCompleted() call should ensure that, even if there is a reentrant call.

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

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