On Fri, 17 Feb 2023 14:20:54 GMT, Daniel Fuchs <[email protected]> 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:
>
> Update
> src/java.net.http/share/classes/jdk/internal/net/http/common/HttpBodySubscriberWrapper.java
>
> make `state` private
The changes look good to me. `HttpBodySubscriberWrapper` and `Http1Exchange`
files will need a copyright year update, before integrating.
-------------
Marked as reviewed by jpai (Reviewer).
PR: https://git.openjdk.org/jdk/pull/12587
