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

[Jaikiran Pai]

On Fri, 11 Aug 2023 13:24:08 GMT, Daniel Fuchs <dfuchs 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.

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

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