On Fri, 6 May 2022 04:49:53 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> Hi, please find here a patch that solves a rare intermittent test failure
>> observed in the test `java/net/httpclient/ExecutorShutdown.java`
>>
>> A race condition coupled with some too eager synchronization was causing a
>> deadlock between an Http2Connection close, a thread trying to shutdown the
>> HttpClient due to a RejectedTaskException, and the SelectorManager thread
>> trying to exit.
>>
>> The fix comprises several cleanup - in particular:
>>
>> - `Http2Connection`: no need to try to send a `GOAWAY` frame if the
>> underlying TCP connection is already closed
>> - `SSLFlowDelegate`/`SubscriberWrapper`: no need to trigger code that will
>> request more data from upstream if the sequential scheduler that is supposed
>> to handle that data once it arrives is already closed
>> - `Http1Exchange`/`Http1Request`: proper cancellation of subscription if an
>> exception is raised before `onSubscribe()` has been called
>> - `HttpClientImpl`: avoid calling callbacks from within synchronized blocks
>> when not necessary
>> - `ReferenceTracker`: better thread dumps in case where the selector is
>> still alive at the end of the test (remove the limit that limited the stack
>> traces to 8 element max by no longer relying on `ThreadInfo::toString`)
>
> src/java.net.http/share/classes/jdk/internal/net/http/HttpClientImpl.java
> line 1039:
>
>> 1037: e.abort(selectorClosedException());
>> 1038: } else {
>> 1039: selector.wakeup();
>
> Hello Daniel, before this PR, except for the `wakeupSelector()` method on
> `SelectorManager`, all other methods which were operating on the `selector`
> field were `synchronized` on the `SelectorManager` instance. After this PR,
> that continues to be true except for this specific line where the operation
> on the `selector` field is outside of a `synchronized` block. Would that be
> OK?
And this is what (or at least a part of what) was causing the issue. We need to
add the event to the `registrations` list within a synchronized block because
that's a plain ArrayList whose access is controlled by synchronizing on `this`.
However the event should not be invoked within the synchronized and block, and
if you look at the calling method (eventUpdated) you will see that this is what
we are doing there too (raising the event without synchronizing).
-------------
PR: https://git.openjdk.java.net/jdk/pull/8562
