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

[Jaikiran Pai]

On Wed, 12 Oct 2022 14:23:13 GMT, Daniel Fuchs <dfuchs at openjdk.org> wrote:

>> When [JDK-8277969](https://bugs.openjdk.org/browse/JDK-8277969) was implemented, a list of outstanding response subscribers was added to `HttpClientImpl`. A body subscriber is added to the list after being created and is removed from the list when it is completed, either successfully or exceptionally.
>> 
>> It appears that in the case where the subscription is cancelled before the subscriber is completed, the subscriber might remain registered in the list forever, or at least until the HttpClient gets garbage collected. This can be easily reproduced using streaming subscribers, such as BodySubscriber::ofInputStream. In the case where the input stream is closed without having read all the bytes, Subscription::cancel will be called. Whether the subscriber gets unregistered or not at that point becomes racy.
>> 
>> Indeed, the reactive stream specification doesn't guarantee whether onComplete or onError will be called or not after a subscriber cancels its subscription. Any cleanup that would have been performed by onComplete/onError might therefore need to be performed when the subscription is cancelled too.
>
> Daniel Fuchs has updated the pull request incrementally with one additional commit since the last revision:
> 
>   More cleanup to the test

src/java.net.http/share/classes/jdk/internal/net/http/common/HttpBodySubscriberWrapper.java line 83:

> 81:         @Override
> 82:         public void cancel() {
> 83:             HttpBodySubscriberWrapper.this.cancel(subscription);

Hello Daniel, at first when I noticed this code, it looked odd to me that we aren't calling the underlying `subscription.cancel()`. I then noticed that the `HttpBodySubscriberWrapper.cancel()` actually does the cancellation of this underyling `subscription`. It's slightly odd to me that we are doing it there instead of here. Furthermore, the `HttpBodySubscriberWrapper` (which implements the `Flow` interface) has APIs like `onComplete()`, `onError()`. Perhaps it would be more consistent to have an `onCancel` in `HttpBodySubscriberWrapper` and then have the actual cancellation of the `subscription` done here in the `SubscriptionWrapper` and then call the `onCancel(Subscription)` on the `HttpBodySubscriberWrapper`? Something like:




class SubscriptionWrapper ... {
  @Override
  public void cancel() {
    try {
      subscription.cancel();
    } finally {
         HttpBodySubscriberWrapper.this.onCancel(subscription);
    }
  }
}



class HttpBodySubscriberWrapper {
    protected void onCancel(Subscription cancelledSubscription) {
        
    }
}

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.

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

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