RFR: 8286194: ExecutorShutdown test fails intermittently

[Daniel Fuchs]

On Fri, 6 May 2022 04:57:00 GMT, Jaikiran Pai <jpai at 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