On Wed, 12 Oct 2022 14:23:13 GMT, Daniel Fuchs <[email protected]> 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
