RFR: 8294916: Cancelling a request must eventually cause its response body subscriber to be unregistered [v4]

[Jaikiran Pai]

On Mon, 17 Oct 2022 14:51:23 GMT, Daniel Fuchs <dfuchs at openjdk.org> wrote:

>> src/java.net.http/share/classes/jdk/internal/net/http/common/HttpBodySubscriberWrapper.java line 166:
>> 
>>> 164:             if (subscribed.compareAndSet(false, true)) {
>>> 165:                 SubscriptionWrapper wrapped = new SubscriptionWrapper(subscription);
>>> 166:                 userSubscriber.onSubscribe(this.subscription = wrapped);
>> 
>> To make it simpler to read, perhaps change it to:
>> 
>> this.subscription = new SubscriptionWrapper(subscription);
>> userSubscriber.onSubscribe(this.subscription);
>> 
>> 
>> It's OK with me if you prefer to have it in the current form.
>
> I would prefer to leave it like that because this.subscription is volatile.

I overlooked the fact that this was a `volatile` field. So this isn't just a stylistic preference and the current form you have here adds value. So yes, looks fine to me. 
I additionally checked the generated bytecode to be sure that this does indeed do just one access to the volatile field (I wasn't sure what it would look like when you are writing to it as well as passing it as a method parameter).

>> src/java.net.http/share/classes/jdk/internal/net/http/common/HttpBodySubscriberWrapper.java line 181:
>> 
>>> 179:         if (completed.get()) {
>>> 180:             if (subscription != null) {
>>> 181:                 subscription.subscription.cancel();
>> 
>> Are we intentionally accessing and cancelling the underlying subscription here, instead of the wrapper? Do we intentionally want to bypass the on-cancellation logic in this flow?
>
> Yes. completed.get() is true here which means that we're receiving onNext after onError or onComplete have been called. Which also means that the stream is guaranteed to be unregistered (at some point). Receiving onNext after onError or onComplete have been called should not happen according to reactive stream specification. But if it does, we really want to make sure the underlying subscription is cancelled. Though I agree it's belt and braces.

Thank you for that explanation. Looks good to me.

> I'm not sure I want to change it ;-)

I wasn't expecting this to be changed :) At this point, it's now become a habit to pay extra attention to the value we pass for this method.

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

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