On Fri, 6 May 2022 04:57:00 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/common/SSLFlowDelegate.java
> line 784:
>
>> 782:
>> 783: while (Utils.synchronizedRemaining(writeList) > 0 ||
>> hsTriggered() || needWrap()) {
>> 784: if (scheduler.isStopped()) return;
>
> If the `scheduler` is stopped then would this task instance be ever called
> again? If it won't be called again, then do you think we should perhaps drain
> the queued `writeList` to reduce any memory resource accumulation till the
> next GC cycle?
if the scheduler is closed the connection is being stopped and will be shortly
eligible for garbage collection, so I wouldn't bother with trying to clear the
queue.
> test/jdk/java/net/httpclient/ReferenceTracker.java line 95:
>
>> 93: }
>> 94:
>> 95: private static String toString(ThreadInfo info) {
>
> Should we perhaps add a comment to this method explaining why we are
> duplicating the implementation of `ThreadInfo#toString()` here?
>
> I can't find in commit logs or the documentation of the `ThreadInfo` class on
> why its `toString()` implementation just outputs only 8 stacktrace elements.
> Do you think we should remove that restriction or document it in a separate
> issue?
Good idea to add a comment. I have also wondered if we should add a new method
to ThreadInfo that would take a "maxdepth" integer. I don't know why the output
is limited to 8 element. Maybe we should log an enhancement request and let the
maintainers of ThreadInfo decide if they want to remove the limitation or
provide a new method, or do nothing.
-------------
PR: https://git.openjdk.java.net/jdk/pull/8562
