RFR: 8302635: Race condition in HttpBodySubscriberWrapper when cancelling request [v2]

[Jaikiran Pai]

On Thu, 16 Feb 2023 12:47:56 GMT, Daniel Fuchs <dfuchs at openjdk.org> wrote:

>> The HttpBodySubscriberWrapper is a class that ensures that a subscriber will be subscribed to before it is completed. It also provides hooks to its two subclasses (one for HTTP/1, one for HTTP/2) that allows subclasses to register the susbscriber with the HttpClient at subscription time, and to unregister it when it is eventualy completed, or when the subscription is cancelled.
>> 
>> There is however a race condition that can happen when a subscription is cancelled: it can happen that unregister is called before register. The CancelRequestTest has been observed failing once or twice on personal jobs. Though the particular mechanics of this race is hard to understand, the logs of the tests have brought sufficient evidence that this is what was happening. 
>> 
>> The symptom is finding one subscriber still registered after completion of the exchange:
>> 
>> test CancelRequestTest.testGetSendAsync("https://localhost:42711/https1/x/same/interrupt", true, true): failure
>> java.lang.AssertionError: WARNING: tracker for HttpClientImpl(13) has outstanding operations:
>> Pending HTTP Requests: 0
>> Pending HTTP/1.1 operations: 0
>> Pending HTTP/2 streams: 0
>> Pending WebSocket operations: 0
>> Pending TCP connections: 0
>> Pending Subscribers: 1
>> Total pending operations: 0
>> Facade referenced: true
>> Selector alive: true
>> 
>> The proposed fix hoist special hooks for register/unregister in the superclass, merges all various volatile boolean states into a single int state, and protect the state changes  to subscribed/register/unregister by the same subscription lock.
>> If cancelling the subscription happens at around the same time that the subscriber is subscribed this ensures that the subscriber won't be removed from the map before it is added.
>
> Daniel Fuchs has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fixed boolean cancelled() {}

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

> 68:     final long id = IDS.incrementAndGet();
> 69:     final BodySubscriber<T> userSubscriber;
> 70:     volatile int state;

Hello Daniel, should we make this `private` and all the newly introduced state values, too? Or maybe you want to leave it package private to be consistent with some other fields here?

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

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