Skip to content

Commit

Permalink
8296889: Race condition when cancelling a request
Browse files Browse the repository at this point in the history
Reviewed-by: jpai
  • Loading branch information
dfuch committed Nov 15, 2022
1 parent 87530e6 commit 7357a1a
Show file tree
Hide file tree
Showing 4 changed files with 25 additions and 6 deletions.
Expand Up @@ -208,6 +208,11 @@ static final class Http1ResponseBodySubscriber<U> extends HttpBodySubscriberWrap
this.exchange = exchange;
}

@Override
public void onSubscribed() {
exchange.registerResponseSubscriber(this);
}

@Override
protected void complete(Throwable t) {
try {
Expand Down Expand Up @@ -459,7 +464,6 @@ Http1ResponseBodySubscriber<T> createResponseSubscriber(BodyHandler<T> handler,
BodySubscriber<T> subscriber = handler.apply(response);
Http1ResponseBodySubscriber<T> bs =
new Http1ResponseBodySubscriber<T>(subscriber, this);
registerResponseSubscriber(bs);
return bs;
}

Expand Down
Expand Up @@ -344,7 +344,6 @@ CompletableFuture<T> readBodyAsync(HttpResponse.BodyHandler<T> handler,
Http2StreamResponseSubscriber<T> createResponseSubscriber(BodyHandler<T> handler, ResponseInfo response) {
Http2StreamResponseSubscriber<T> subscriber =
new Http2StreamResponseSubscriber<>(handler.apply(response));
registerResponseSubscriber(subscriber);
return subscriber;
}

Expand Down Expand Up @@ -1543,17 +1542,22 @@ final class Http2StreamResponseSubscriber<U> extends HttpBodySubscriberWrapper<U
super(subscriber);
}

@Override
public void onSubscribed() {
registerResponseSubscriber(this);
}

@Override
protected void complete(Throwable t) {
try {
Stream.this.unregisterResponseSubscriber(this);
unregisterResponseSubscriber(this);
} finally {
super.complete(t);
}
}
@Override
protected void onCancel() {
Stream.this.unregisterResponseSubscriber(this);
unregisterResponseSubscriber(this);
}
}

Expand Down
Expand Up @@ -127,6 +127,15 @@ private void propagateError(Throwable t) {
*/
protected void onCancel() { }

/**
* Called right after the userSubscriber::onSubscribe is called.
* @apiNote
* This method may be used by subclasses to perform cleanup
* related actions after a subscription has been succesfully
* accepted.
*/
protected void onSubscribed() { }

/**
* Complete the subscriber, either normally or exceptionally
* ensure that the subscriber is completed only once.
Expand Down Expand Up @@ -169,8 +178,9 @@ public CompletionStage<T> getBody() {
public void onSubscribe(Flow.Subscription subscription) {
// race condition with propagateError: we need to wait until
// subscription is finished before calling onError;
boolean onSubscribed;
synchronized (this) {
if (subscribed.compareAndSet(false, true)) {
if ((onSubscribed = subscribed.compareAndSet(false, true))) {
SubscriptionWrapper wrapped = new SubscriptionWrapper(subscription);
userSubscriber.onSubscribe(this.subscription = wrapped);
} else {
Expand All @@ -181,6 +191,7 @@ public void onSubscribe(Flow.Subscription subscription) {
assert completed.get();
}
}
if (onSubscribed) onSubscribed();
}

@Override
Expand Down
2 changes: 1 addition & 1 deletion test/jdk/java/net/httpclient/CancelRequestTest.java
Expand Up @@ -23,7 +23,7 @@

/*
* @test
* @bug 8245462 8229822 8254786
* @bug 8245462 8229822 8254786 8296889
* @summary Tests cancelling the request.
* @library /test/lib http2/server
* @key randomness
Expand Down

1 comment on commit 7357a1a

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign in to comment.