Avoid self-suppression on grouped action listener (#53262)

It can be that a failure is repeated to a grouped action listener. For
example, if the same exception such as a connect transport exception, is
the cause of repeated failures. Previously we were unconditionally
self-suppressing the exception into the first exception, but
self-supressing is not allowed. Thus, we would throw an exception and
the grouped action listener would never complete. This commit addresses
this by guarding against self-suppression.
This commit is contained in:
Jason Tedor 2020-03-08 08:58:59 -04:00
parent c5738ae312
commit a0b235888f
No known key found for this signature in database
GPG Key ID: FA89F05560F16BC5
2 changed files with 26 additions and 3 deletions

View File

@ -71,9 +71,12 @@ public final class GroupedActionListener<T> implements ActionListener<T> {
@Override @Override
public void onFailure(Exception e) { public void onFailure(Exception e) {
if (failure.compareAndSet(null, e) == false) { if (failure.compareAndSet(null, e) == false) {
failure.accumulateAndGet(e, (previous, current) -> { failure.accumulateAndGet(e, (current, update) -> {
previous.addSuppressed(current); // we have to avoid self-suppression!
return previous; if (update != current) {
current.addSuppressed(update);
}
return current;
}); });
} }
if (countDown.countDown()) { if (countDown.countDown()) {

View File

@ -33,6 +33,9 @@ import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.atomic.AtomicReference; import java.util.concurrent.atomic.AtomicReference;
import static org.hamcrest.CoreMatchers.instanceOf; import static org.hamcrest.CoreMatchers.instanceOf;
import static org.hamcrest.CoreMatchers.nullValue;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.not;
public class GroupedActionListenerTests extends ESTestCase { public class GroupedActionListenerTests extends ESTestCase {
@ -133,4 +136,21 @@ public class GroupedActionListenerTests extends ESTestCase {
assertThat(exception, instanceOf(IOException.class)); assertThat(exception, instanceOf(IOException.class));
assertEquals(numGroups - 1, exception.getSuppressed().length); assertEquals(numGroups - 1, exception.getSuppressed().length);
} }
/*
* It can happen that the same exception causes a grouped listener to be notified of the failure multiple times. Since we suppress
* additional exceptions into the first exception, we have to guard against suppressing into the same exception, which could occur if we
* are notified of with the same failure multiple times. This test verifies that the guard against self-suppression remains.
*/
public void testRepeatNotificationForTheSameException() {
final AtomicReference<Exception> finalException = new AtomicReference<>();
final GroupedActionListener<Void> listener = new GroupedActionListener<>(ActionListener.wrap(r -> {}, finalException::set), 2);
final Exception e = new Exception();
// repeat notification for the same exception
listener.onFailure(e);
listener.onFailure(e);
assertThat(finalException.get(), not(nullValue()));
assertThat(finalException.get(), equalTo(e));
}
} }