From a79896674ad76cbc17ae2a8f6cca56bd152f5bd7 Mon Sep 17 00:00:00 2001 From: Simon Willnauer Date: Wed, 11 Jan 2017 14:47:45 +0100 Subject: [PATCH] Simplify ActionListener helpers and add dedicated unittests --- .../elasticsearch/action/ActionListener.java | 27 ++-- .../action/ActionListenerTests.java | 151 ++++++++++++++++++ 2 files changed, 160 insertions(+), 18 deletions(-) create mode 100644 core/src/test/java/org/elasticsearch/action/ActionListenerTests.java diff --git a/core/src/main/java/org/elasticsearch/action/ActionListener.java b/core/src/main/java/org/elasticsearch/action/ActionListener.java index 19d33707ed7..f9fafa9f95a 100644 --- a/core/src/main/java/org/elasticsearch/action/ActionListener.java +++ b/core/src/main/java/org/elasticsearch/action/ActionListener.java @@ -19,8 +19,11 @@ package org.elasticsearch.action; +import org.elasticsearch.ExceptionsHelper; import org.elasticsearch.common.CheckedConsumer; +import java.util.ArrayList; +import java.util.List; import java.util.function.Consumer; /** @@ -72,7 +75,7 @@ public interface ActionListener { * listeners will be processed and the caught exception will be re-thrown. */ static void onResponse(Iterable> listeners, Response response) { - RuntimeException exception = null; + List exceptionList = new ArrayList<>(); for (ActionListener listener : listeners) { try { listener.onResponse(response); @@ -80,17 +83,11 @@ public interface ActionListener { try { listener.onFailure(ex); } catch (Exception ex1) { - if (exception != null) { - exception = new RuntimeException(ex1); - } else { - exception.addSuppressed(ex1); - } + exceptionList.add(ex1); } } } - if (exception != null) { - throw exception; - } + ExceptionsHelper.maybeThrowRuntimeAndSuppress(exceptionList); } /** @@ -98,20 +95,14 @@ public interface ActionListener { * all remaining listeners will be processed and the caught exception will be re-thrown. */ static void onFailure(Iterable> listeners, Exception failure) { - RuntimeException exception = null; + List exceptionList = new ArrayList<>(); for (ActionListener listener : listeners) { try { listener.onFailure(failure); } catch (Exception ex) { - if (exception != null) { - exception = new RuntimeException(ex); - } else { - exception.addSuppressed(ex); - } + exceptionList.add(ex); } } - if (exception != null) { - throw exception; - } + ExceptionsHelper.maybeThrowRuntimeAndSuppress(exceptionList); } } diff --git a/core/src/test/java/org/elasticsearch/action/ActionListenerTests.java b/core/src/test/java/org/elasticsearch/action/ActionListenerTests.java new file mode 100644 index 00000000000..6414c81058b --- /dev/null +++ b/core/src/test/java/org/elasticsearch/action/ActionListenerTests.java @@ -0,0 +1,151 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.elasticsearch.action; + +import org.elasticsearch.common.CheckedConsumer; +import org.elasticsearch.test.ESTestCase; + +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; + +public class ActionListenerTests extends ESTestCase { + + public void testWrap() { + AtomicReference reference = new AtomicReference<>(); + AtomicReference exReference = new AtomicReference<>(); + + CheckedConsumer handler = (o) -> { + if (Boolean.FALSE.equals(o)) { + throw new IllegalArgumentException("must not be false"); + } + reference.set(o); + }; + ActionListener wrap = ActionListener.wrap(handler, exReference::set); + wrap.onResponse(Boolean.FALSE); + assertNull(reference.get()); + assertNotNull(exReference.get()); + assertEquals("must not be false", exReference.get().getMessage()); + exReference.set(null); + + wrap.onResponse(Boolean.TRUE); + assertTrue(reference.get()); + assertNull(exReference.get()); + } + + public void testOnResponse() { + final int numListeners = randomIntBetween(1, 20); + List> refList = new ArrayList<>(); + List> excList = new ArrayList<>(); + List> listeners = new ArrayList<>(); + List failOnTrue = new ArrayList<>(); + AtomicInteger exceptionCounter = new AtomicInteger(0); + for (int i = 0; i < numListeners; i++) { + boolean doFailOnTrue = rarely(); + failOnTrue.add(doFailOnTrue); + AtomicReference reference = new AtomicReference<>(); + AtomicReference exReference = new AtomicReference<>(); + refList.add(reference); + excList.add(exReference); + CheckedConsumer handler = (o) -> { + if (Boolean.FALSE.equals(o)) { + throw new IllegalArgumentException("must not be false " + exceptionCounter.getAndIncrement()); + } + if (doFailOnTrue) { + throw new IllegalStateException("must not be true"); + } + reference.set(o); + }; + listeners.add(ActionListener.wrap(handler, exReference::set)); + } + + ActionListener.onResponse(listeners, Boolean.TRUE); + for (int i = 0; i < numListeners; i++) { + if (failOnTrue.get(i) == false) { + assertTrue("listener index " + i, refList.get(i).get()); + refList.get(i).set(null); + } else { + assertNull("listener index " + i, refList.get(i).get()); + } + + } + + for (int i = 0; i < numListeners; i++) { + if (failOnTrue.get(i) == false) { + assertNull("listener index " + i, excList.get(i).get()); + } else { + assertEquals("listener index " + i, "must not be true", excList.get(i).get().getMessage()); + } + } + + ActionListener.onResponse(listeners, Boolean.FALSE); + for (int i = 0; i < numListeners; i++) { + assertNull("listener index " + i, refList.get(i).get()); + } + + assertEquals(numListeners, exceptionCounter.get()); + for (int i = 0; i < numListeners; i++) { + assertNotNull(excList.get(i).get()); + assertEquals("listener index " + i, "must not be false " + i, excList.get(i).get().getMessage()); + } + } + + public void testOnFailure() { + final int numListeners = randomIntBetween(1, 20); + List> refList = new ArrayList<>(); + List> excList = new ArrayList<>(); + List> listeners = new ArrayList<>(); + + final int listenerToFail = randomBoolean() ? -1 : randomIntBetween(0, numListeners-1); + for (int i = 0; i < numListeners; i++) { + AtomicReference reference = new AtomicReference<>(); + AtomicReference exReference = new AtomicReference<>(); + refList.add(reference); + excList.add(exReference); + boolean fail = i == listenerToFail; + CheckedConsumer handler = (o) -> { + reference.set(o); + }; + listeners.add(ActionListener.wrap(handler, (e) -> { + exReference.set(e); + if (fail) { + throw new RuntimeException("double boom"); + } + })); + } + + try { + ActionListener.onFailure(listeners, new Exception("booom")); + assertTrue("unexpected succces listener to fail: " + listenerToFail, listenerToFail == -1); + } catch (RuntimeException ex) { + assertTrue("listener to fail: " + listenerToFail, listenerToFail >= 0); + assertNotNull(ex.getCause()); + assertEquals("double boom", ex.getCause().getMessage()); + } + + for (int i = 0; i < numListeners; i++) { + assertNull("listener index " + i, refList.get(i).get()); + } + + for (int i = 0; i < numListeners; i++) { + assertEquals("listener index " + i, "booom", excList.get(i).get().getMessage()); + } + } +}