RFR: 8297149: REDO JDK-8296889: Race condition when cancelling a request
Daniel Fuchs
dfuchs at openjdk.org
Thu Nov 17 09:59:50 UTC 2022
On Thu, 17 Nov 2022 01:23:49 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:
>> Please find here a re-do fix for the race condition while cancelling request.
>> The previous fix failed because it registered the subscriber too late (after having called userSubsciber.onSubscribe()), which opened a window for the call to unregister to occur before the call to register.
>> This is fixed in this new iteration.
>
> src/java.net.http/share/classes/jdk/internal/net/http/common/HttpBodySubscriberWrapper.java line 138:
>
>> 136:
>> 137: /**
>> 138: * Called right after the userSubscriber::onSubscribe is called.
>
> Hello Daniel, I suspect this comment will need a change now, since the implementation in this PR now calls `onSubscribed` before the `userSubscriber::onSubscribe`.
Ah! Good catch.
> src/java.net.http/share/classes/jdk/internal/net/http/common/HttpBodySubscriberWrapper.java line 188:
>
>> 186: // race condition with propagateError: we need to wait until
>> 187: // subscription is finished before calling onError;
>> 188: subscriptionLock.lock();
>
> More of a question than a review comment - I see that the only place in this class where we were using `synchronized` is while dealing with the `subscribed`. The PR replaces the `synchronized` blocks with a `ReentrantLock`. Does that have an advantage in context of this code?
Well - I am not sure. The code within the block calls onSubscribe on the user subscriber. Theoretically, this should not block, but it might kick off a loop (via calling subscription::request) that may run in the current thread for a while. Using a lock ensures that the waiter will be well-behaved for virtual threads if that ever happens - and no carrier thread will be pinned because of this.
-------------
PR: https://git.openjdk.org/jdk/pull/11193
More information about the net-dev
mailing list