On Thu, 16 Feb 2023 12:47:56 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:
>
> Fixed boolean cancelled() {}
src/java.net.http/share/classes/jdk/internal/net/http/common/HttpBodySubscriberWrapper.java
line 172:
> 170: // and closing the connection.
> 171: userSubscriber.onError(t);
> 172: }
I haven't fully wrapped my head around the possible flow of the user
subscription, but is there a possiblity where this call to `onError(...)` here
results in an reentrant call to this `propagateError()`? For that matter, not
just reentrant but perhaps from a different thread concurrently? The reason I
ask is, should we call this onError just once? The
`java.util.concurrent.Flow.Subscriber.onError(Throwable)` method says this:
> Method invoked upon an unrecoverable error encountered by a
> Publisher or Subscription, after which no other Subscriber
> methods are invoked by the Subscription.
So I'm wondering if we should maintain some state to only invoke it once?
-------------
PR: https://git.openjdk.org/jdk/pull/12587
