RFR: 8314136: Test java/net/httpclient/CancelRequestTest.java failed: WARNING: tracker for HttpClientImpl(42) has outstanding operations

[Daniel Fuchs]

On Mon, 14 Aug 2023 10:00:19 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> Please find here a patch for a very rare intermittent failure observed in CancelRequestTest.
>> 
>> The fact that the number of pending requests hasn't been decremented leads me to think that completable future returned by sendAsync (called from send) hasn't been fully completed. That is, the dependent actions registered by sendAsync have not been run within the timeout waiting for the number of pending requests to reach 0.
>> 
>> The fix is to call cf.get() again after calling cf.cancel(), and increase the timeout waiting for cleanup in testPostInterrupt.
>> client.close() is also called at the end of each test method to reclaim resources earlier
>
> src/java.net.http/share/classes/jdk/internal/net/http/HttpClientImpl.java line 941:
> 
>> 939:                     // cleanup dependant actions to execute
>> 940:                     // this should throw a CancellationException
>> 941:                     cf.get();
> 
> Hello Daniel,
> I'm a bit surprised that calling `get()` on a `cancel()`ed `CompletableFuture` does indeed "wait" and doesn't immediately throw a `CancellationException`. The javadoc of `CompletableFuture#get()` says:
> 
>>  @throws CancellationException if this future was cancelled
> 
> but looking at the implementation of that method, it doesn't seem to immediately throw a `CancellationException`.
> 
> If adding this get() does indeed wait for the dependent actions, then I think that's a good thing to do. However, if this is indeed the expected semantics of these APIs, then it might also mean that we might have to start looking for other similar places and the code might start becoming complicated.

Yes, I think you're right. I was thinking about that over the weekend and I don't think calling `get()` here will achieve anything, since the future should already be completed with a cancelletion exception by the time we call get(). If we wanted to wait for the dependent actions to complete what we would need to do is actually cancel the `mexCf` (and not the head of the chain) and wait for the cancellation exception to trickle up. That's not how we implemented MinimalFuture::cancel however, and getting it to do that is more complex than it looks. The way we specified cancel as a best effort allows the cancellation to continue in the backround after returning, so it's actually only an issue for the test where we need to take that into account. I believe what happened with this test failure is that we were on a busy CPU and simply did not wait long enough. So I might just drop the changes to `HttpClientImpl` out of this PR.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/15249#discussion_r1293324396