From bd3a59a239909c47bb220e81c8cec3aed51aef57 Mon Sep 17 00:00:00 2001 From: Henning Andersen <33268011+henningandersen@users.noreply.github.com> Date: Fri, 20 Dec 2019 10:25:39 +0100 Subject: [PATCH 1/4] Improve FutureUtils.get exception handling (#50339) FutureUtils.get() would unwrap ElasticsearchWrapperExceptions. This is trappy, since nearly all usages of FutureUtils.get() expected only to not have to deal with checked exceptions. In particular, StepListener builds upon ListenableFuture which uses FutureUtils.get to be informed about the exception passed to onFailure. This had the bad consequence of masking away any exception that was an ElasticsearchWrapperException like RemoteTransportException. Specifically for recovery, this made CircuitBreakerExceptions happening on the target node look like they originated from the source node. The only usage that expected that behaviour was AdapterActionFuture. The unwrap behaviour has been moved to that class. --- .../action/support/AdapterActionFuture.java | 21 ++++++++++++-- .../action/index/MappingUpdatedAction.java | 10 ++----- .../common/util/concurrent/FutureUtils.java | 14 +--------- .../action/StepListenerTests.java | 18 ++++++++++++ .../support/AdapterActionFutureTests.java | 28 +++++++++++++++++++ 5 files changed, 68 insertions(+), 23 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/support/AdapterActionFuture.java b/server/src/main/java/org/elasticsearch/action/support/AdapterActionFuture.java index 528750ba89b90..c37e89b8396b8 100644 --- a/server/src/main/java/org/elasticsearch/action/support/AdapterActionFuture.java +++ b/server/src/main/java/org/elasticsearch/action/support/AdapterActionFuture.java @@ -19,11 +19,13 @@ package org.elasticsearch.action.support; +import org.elasticsearch.ElasticsearchException; import org.elasticsearch.action.ActionFuture; import org.elasticsearch.action.ActionListener; import org.elasticsearch.common.unit.TimeValue; import org.elasticsearch.common.util.concurrent.BaseFuture; import org.elasticsearch.common.util.concurrent.FutureUtils; +import org.elasticsearch.common.util.concurrent.UncategorizedExecutionException; import java.util.concurrent.TimeUnit; @@ -31,7 +33,11 @@ public abstract class AdapterActionFuture extends BaseFuture implements @Override public T actionGet() { - return FutureUtils.get(this); + try { + return FutureUtils.get(this); + } catch (ElasticsearchException e) { + throw unwrapEsException(e); + } } @Override @@ -51,7 +57,11 @@ public T actionGet(TimeValue timeout) { @Override public T actionGet(long timeout, TimeUnit unit) { - return FutureUtils.get(this, timeout, unit); + try { + return FutureUtils.get(this, timeout, unit); + } catch (ElasticsearchException e) { + throw unwrapEsException(e); + } } @Override @@ -66,4 +76,11 @@ public void onFailure(Exception e) { protected abstract T convert(L listenerResponse); + private static RuntimeException unwrapEsException(ElasticsearchException esEx) { + Throwable root = esEx.unwrapCause(); + if (root instanceof RuntimeException) { + return (RuntimeException) root; + } + return new UncategorizedExecutionException("Failed execution", root); + } } diff --git a/server/src/main/java/org/elasticsearch/cluster/action/index/MappingUpdatedAction.java b/server/src/main/java/org/elasticsearch/cluster/action/index/MappingUpdatedAction.java index aeba27c4120fb..a4a8cecc52941 100644 --- a/server/src/main/java/org/elasticsearch/cluster/action/index/MappingUpdatedAction.java +++ b/server/src/main/java/org/elasticsearch/cluster/action/index/MappingUpdatedAction.java @@ -19,7 +19,6 @@ package org.elasticsearch.cluster.action.index; -import org.elasticsearch.ElasticsearchException; import org.elasticsearch.action.ActionListener; import org.elasticsearch.action.support.master.AcknowledgedResponse; import org.elasticsearch.action.support.master.MasterNodeRequest; @@ -31,7 +30,6 @@ import org.elasticsearch.common.settings.Setting.Property; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.unit.TimeValue; -import org.elasticsearch.common.util.concurrent.FutureUtils; import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.index.Index; import org.elasticsearch.index.mapper.MapperService; @@ -76,7 +74,7 @@ public void updateMappingOnMaster(Index index, String type, Mapping mappingUpdat } client.preparePutMapping().setConcreteIndex(index).setType(type).setSource(mappingUpdate.toString(), XContentType.JSON) .setMasterNodeTimeout(dynamicMappingUpdateTimeout).setTimeout(TimeValue.ZERO) - .execute(new ActionListener() { + .execute(new ActionListener<>() { @Override public void onResponse(AcknowledgedResponse acknowledgedResponse) { listener.onResponse(null); @@ -84,12 +82,8 @@ public void onResponse(AcknowledgedResponse acknowledgedResponse) { @Override public void onFailure(Exception e) { - listener.onFailure(unwrapException(e)); + listener.onFailure(e); } }); } - - private static Exception unwrapException(Exception cause) { - return cause instanceof ElasticsearchException ? FutureUtils.unwrapEsException((ElasticsearchException) cause) : cause; - } } diff --git a/server/src/main/java/org/elasticsearch/common/util/concurrent/FutureUtils.java b/server/src/main/java/org/elasticsearch/common/util/concurrent/FutureUtils.java index 15e26779071ec..236dc9c716266 100644 --- a/server/src/main/java/org/elasticsearch/common/util/concurrent/FutureUtils.java +++ b/server/src/main/java/org/elasticsearch/common/util/concurrent/FutureUtils.java @@ -19,7 +19,6 @@ package org.elasticsearch.common.util.concurrent; -import org.elasticsearch.ElasticsearchException; import org.elasticsearch.ElasticsearchTimeoutException; import org.elasticsearch.common.Nullable; import org.elasticsearch.common.SuppressForbidden; @@ -86,21 +85,10 @@ public static T get(Future future, long timeout, TimeUnit unit) { } public static RuntimeException rethrowExecutionException(ExecutionException e) { - if (e.getCause() instanceof ElasticsearchException) { - ElasticsearchException esEx = (ElasticsearchException) e.getCause(); - return unwrapEsException(esEx); - } else if (e.getCause() instanceof RuntimeException) { + if (e.getCause() instanceof RuntimeException) { return (RuntimeException) e.getCause(); } else { return new UncategorizedExecutionException("Failed execution", e); } } - - public static RuntimeException unwrapEsException(ElasticsearchException esEx) { - Throwable root = esEx.unwrapCause(); - if (root instanceof ElasticsearchException || root instanceof RuntimeException) { - return (RuntimeException) root; - } - return new UncategorizedExecutionException("Failed execution", root); - } } diff --git a/server/src/test/java/org/elasticsearch/action/StepListenerTests.java b/server/src/test/java/org/elasticsearch/action/StepListenerTests.java index 15e88830e47e9..df57b30c52276 100644 --- a/server/src/test/java/org/elasticsearch/action/StepListenerTests.java +++ b/server/src/test/java/org/elasticsearch/action/StepListenerTests.java @@ -22,11 +22,13 @@ import org.elasticsearch.test.ESTestCase; import org.elasticsearch.threadpool.TestThreadPool; import org.elasticsearch.threadpool.ThreadPool; +import org.elasticsearch.transport.RemoteTransportException; import org.junit.After; import org.junit.Before; import java.util.concurrent.CountDownLatch; import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; import java.util.function.Consumer; import static org.hamcrest.Matchers.equalTo; @@ -110,4 +112,20 @@ private void executeAction(Runnable runnable) { runnable.run(); } } + + /** + * This test checks that we no longer unwrap exceptions when using StepListener. + */ + public void testNoUnwrap() { + StepListener step = new StepListener<>(); + step.onFailure(new RemoteTransportException("test", new RuntimeException("expected"))); + AtomicReference exception = new AtomicReference<>(); + step.whenComplete(null, e -> { + exception.set((RuntimeException) e); + }); + + assertEquals(RemoteTransportException.class, exception.get().getClass()); + RuntimeException e = expectThrows(RuntimeException.class, () -> step.result()); + assertEquals(RemoteTransportException.class, e.getClass()); + } } diff --git a/server/src/test/java/org/elasticsearch/action/support/AdapterActionFutureTests.java b/server/src/test/java/org/elasticsearch/action/support/AdapterActionFutureTests.java index a7405ddae8cce..b2c6a8c5ba2aa 100644 --- a/server/src/test/java/org/elasticsearch/action/support/AdapterActionFutureTests.java +++ b/server/src/test/java/org/elasticsearch/action/support/AdapterActionFutureTests.java @@ -19,12 +19,16 @@ package org.elasticsearch.action.support; +import org.elasticsearch.ElasticsearchException; import org.elasticsearch.common.unit.TimeValue; +import org.elasticsearch.common.util.concurrent.UncategorizedExecutionException; import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.transport.RemoteTransportException; import java.util.Objects; import java.util.concurrent.BrokenBarrierException; import java.util.concurrent.CyclicBarrier; +import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; @@ -90,4 +94,28 @@ protected String convert(final Integer listenerResponse) { thread.join(); } + public void testUnwrapException() { + checkUnwrap(new RemoteTransportException("test", new RuntimeException()), RuntimeException.class, RemoteTransportException.class); + checkUnwrap(new RemoteTransportException("test", new Exception()), + UncategorizedExecutionException.class, RemoteTransportException.class); + checkUnwrap(new Exception(), UncategorizedExecutionException.class, Exception.class); + checkUnwrap(new ElasticsearchException("test", new Exception()), ElasticsearchException.class, ElasticsearchException.class); + } + + private void checkUnwrap(Exception exception, Class actionGetException, + Class getException) { + final AdapterActionFuture adapter = new AdapterActionFuture() { + @Override + protected Void convert(Void listenerResponse) { + fail(); + return null; + } + }; + + adapter.onFailure(exception); + assertEquals(actionGetException, expectThrows(RuntimeException.class, adapter::actionGet).getClass()); + assertEquals(actionGetException, expectThrows(RuntimeException.class, () -> adapter.actionGet(10, TimeUnit.SECONDS)).getClass()); + assertEquals(getException, expectThrows(ExecutionException.class, () -> adapter.get()).getCause().getClass()); + assertEquals(getException, expectThrows(ExecutionException.class, () -> adapter.get(10, TimeUnit.SECONDS)).getCause().getClass()); + } } From 2c85223934549cbe045f04a0bb39fd6253242b57 Mon Sep 17 00:00:00 2001 From: Henning Andersen Date: Fri, 20 Dec 2019 12:11:22 +0100 Subject: [PATCH 2/4] Compilation fix --- .../cluster/action/index/MappingUpdatedAction.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/cluster/action/index/MappingUpdatedAction.java b/server/src/main/java/org/elasticsearch/cluster/action/index/MappingUpdatedAction.java index a4a8cecc52941..ad7c566351b89 100644 --- a/server/src/main/java/org/elasticsearch/cluster/action/index/MappingUpdatedAction.java +++ b/server/src/main/java/org/elasticsearch/cluster/action/index/MappingUpdatedAction.java @@ -74,7 +74,7 @@ public void updateMappingOnMaster(Index index, String type, Mapping mappingUpdat } client.preparePutMapping().setConcreteIndex(index).setType(type).setSource(mappingUpdate.toString(), XContentType.JSON) .setMasterNodeTimeout(dynamicMappingUpdateTimeout).setTimeout(TimeValue.ZERO) - .execute(new ActionListener<>() { + .execute(new ActionListener() { @Override public void onResponse(AcknowledgedResponse acknowledgedResponse) { listener.onResponse(null); From 2d16e552f6aa5ab36da57db404a8706cd4fad680 Mon Sep 17 00:00:00 2001 From: Henning Andersen Date: Mon, 30 Dec 2019 16:05:34 +0100 Subject: [PATCH 3/4] Readd MappingUpdatedAction unwrapping of exceptions Since ElasticsearchException.guessRootCause does not support wrapped non-ElasticsearchException, this causes a test failure in 7.x. --- .../action/index/MappingUpdatedAction.java | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/cluster/action/index/MappingUpdatedAction.java b/server/src/main/java/org/elasticsearch/cluster/action/index/MappingUpdatedAction.java index ad7c566351b89..9b14c16a5c71f 100644 --- a/server/src/main/java/org/elasticsearch/cluster/action/index/MappingUpdatedAction.java +++ b/server/src/main/java/org/elasticsearch/cluster/action/index/MappingUpdatedAction.java @@ -19,6 +19,7 @@ package org.elasticsearch.cluster.action.index; +import org.elasticsearch.ElasticsearchException; import org.elasticsearch.action.ActionListener; import org.elasticsearch.action.support.master.AcknowledgedResponse; import org.elasticsearch.action.support.master.MasterNodeRequest; @@ -30,6 +31,8 @@ import org.elasticsearch.common.settings.Setting.Property; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.unit.TimeValue; +import org.elasticsearch.common.util.concurrent.FutureUtils; +import org.elasticsearch.common.util.concurrent.UncategorizedExecutionException; import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.index.Index; import org.elasticsearch.index.mapper.MapperService; @@ -82,8 +85,21 @@ public void onResponse(AcknowledgedResponse acknowledgedResponse) { @Override public void onFailure(Exception e) { - listener.onFailure(e); + listener.onFailure(unwrapException(e)); } }); } + + // todo: this explicit unwrap should not be necessary, but is until guessRootCause is fixed to allow wrapped non-es exception. + private static Exception unwrapException(Exception cause) { + return cause instanceof ElasticsearchException ? unwrapEsException((ElasticsearchException) cause) : cause; + } + + private static RuntimeException unwrapEsException(ElasticsearchException esEx) { + Throwable root = esEx.unwrapCause(); + if (root instanceof RuntimeException) { + return (RuntimeException) root; + } + return new UncategorizedExecutionException("Failed execution", root); + } } From d9d6f9d64f4980f2ce3dd9aff8d82b16e0b82801 Mon Sep 17 00:00:00 2001 From: Henning Andersen Date: Mon, 30 Dec 2019 21:43:37 +0100 Subject: [PATCH 4/4] Checkstyle fix --- .../elasticsearch/cluster/action/index/MappingUpdatedAction.java | 1 - 1 file changed, 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/cluster/action/index/MappingUpdatedAction.java b/server/src/main/java/org/elasticsearch/cluster/action/index/MappingUpdatedAction.java index 9b14c16a5c71f..a46f003b375b4 100644 --- a/server/src/main/java/org/elasticsearch/cluster/action/index/MappingUpdatedAction.java +++ b/server/src/main/java/org/elasticsearch/cluster/action/index/MappingUpdatedAction.java @@ -31,7 +31,6 @@ import org.elasticsearch.common.settings.Setting.Property; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.unit.TimeValue; -import org.elasticsearch.common.util.concurrent.FutureUtils; import org.elasticsearch.common.util.concurrent.UncategorizedExecutionException; import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.index.Index;