On Mon, 14 Aug 2023 10:00:19 GMT, Jaikiran Pai <[email protected]> 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
