On Mon, 17 Oct 2022 14:51:23 GMT, Daniel Fuchs <[email protected]> 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
