9 RFR: 8178147: Race conditions in timeout handling code in http/2 incubator client

[Daniel Fuchs]

Hi Pavel,

Thanks for taking a look!

On 06/04/2017 13:28, Pavel Rappo wrote:
> Heya Daniel,
>
>     750     /**
>     751      * same as above but for errors
>     752      */
>     753     void completeResponseExceptionally(Throwable t) {
>     754         synchronized (response_cfs) {
>     755             // use index to avoid ConcurrentModificationException
>     756             // caused by removing the CF from within the loop.
>     757             for (int i = 0; i < response_cfs.size(); i++) {
>     758                 CompletableFuture<Response> cf = response_cfs.get(i);
>     759                 if (!cf.isDone()) {
>     760                     cf.completeExceptionally(t);
>     761                     response_cfs.remove(i);
>     762                     return;
>     763                 }
>     764             }
>     765             response_cfs.add(MinimalFuture.failedFuture(t));
>     766         }
>     767     }
>
> I was wondering if @762 should be there in the first place. The logic seems
> a bit odd: find the first CF that hasn't been yet completed, complete it
> and return. Are we guaranteed there are no other not-yet-completed CFs in
> this list? Or we simply do not care about it?

AFAIU this takes care of requests that can require multiple responses
such has 100-continue. In such a scenario, the responses cannot cross
each others, so completing the first uncompleted CF is actually
correct, because it should be the last CF in the list.
Return is needed here because 1. we are removing something from the
list, so after that the next values of 'i' will be invalid, and 2. we
have found our uncompleted response, which is the one created by
getResponseAsync() - so we don't need (and must not) add a new one
(we must not execute line 765).

I do believe there is room for simplification in the way these race
conditions between getResponseAsync() and completeResponse() are
handled, but I'd rather leave such improvement for 10 (or for the
next update).

The only reason I touched this method is because I noticed the
possibility of ConcurrentModificationException if we reached there.

cheers,

-- daniel

>
> Thanks.
>