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.
