RFR: 8286194: ExecutorShutdown test fails intermittently [v2]

[Daniel Fuchs]

On Fri, 6 May 2022 05:16:15 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> Daniel Fuchs has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Added a comment to ReferenceTracker.java as suggested in the review
>
> src/java.net.http/share/classes/jdk/internal/net/http/common/SubscriberWrapper.java line 347:
> 
>> 345: 
>> 346:     void upstreamWindowUpdate() {
>> 347:         if (pushScheduler.isStopped()) return;
> 
> A similar question here. It looks to me that the contract with a `SequentialScheduler` is that the runnable task itself (or someone with access to the scheduler), is responsible for invoking the `runOrSchedule` method so that a next invocation of the task happens. When such a scheduler instance is already stopped, then the call to `runOrSchedule` is kind of a noop, with no next execution of the task taking place. In cases like here, where the task detects that the scheduler has already stopped, do you think the tasks should do some cleanup work like clearing up the `outputQ` of `ByteBuffers`?
> 
> The question really isn't directly related to the PR, but I have been reviewing some unrelated test failures where a large number of HttpClient instances get created, so in that context, I was wondering if there's anything we could do to help reduce any potential memory usage.

What happens here is a bit more subtle: if we don't return, the code is going to request more data from upstream, even though that data won't be processed, and that will fill up the queue, and potentially cause more data to be wrapped by the SSLEngine and sent down stream. I have observed that some activity was still ongoing in the SSLTube/SSLFlowDelegate after the underlying TCP channel had been closed, when there's no chance that this data will be ever sent, and the logs I was observing showed that returning at this point was effectively stopping that useless activity early. The fact that the channel is bidirectional and that the implementation is fully asynchronous means that several tasks may be executing in background threads at the time the TCP connection is closed. If the effect of these task is to simply request, process and submit data that will be later ignored (or cause more exceptions to be relayed) - then we should stop them early.

-------------

PR: https://git.openjdk.java.net/jdk/pull/8562