On Fri, 11 Nov 2022 15:18:08 GMT, Daniel Fuchs <dfu...@openjdk.org> wrote:
> The CancelRequest test was observed failing again after > [JDK-8294916](https://bugs.openjdk.org/browse/JDK-8294916) was integrated: > there is a small race condition window in the code that unregisters the > request BodySubscriber when a request is cancelled: if the request > cancellation happens after the body subscriber is registered but before it is > subscribed it may not be unregistered. > > The solution is to register it only after it has been successfully subscribed. Hello Daniel, from what I understand, before the change in this PR, we used to add the subscriber to an internal collection in the HttpClient even before the subscription was done. That would then mean the callbacks that are related to a subscription wouldn't be invoked, in certain cases (for example the request cancellation). Since we do the unregistration in these callbacks, the registered subscriber would not be removed from the HttpClient's collection. Did I understand this right? If so, your change looks good to me. src/java.net.http/share/classes/jdk/internal/net/http/Http1Exchange.java line 213: > 211: @Override > 212: public void onSubscribed() { > 213: exchange.registerResponseSubscriber(this); The `registerResponseSubscriber` in addition to registering it with the client will also set a local `responseSubscriber` member. Do you think we should `null` it out in the callbacks where we call the `unregisterResponseSubscriber`? ------------- PR: https://git.openjdk.org/jdk/pull/11110
