From 94afb8c6b64810bd3bf2bcf5d1eddf627099b089 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Fri, 9 Mar 2018 16:57:33 -0500 Subject: [PATCH 1/4] Maybe die before failing engine Today we check for a few cases where we should maybe die before failing the engine (e.g., when a merge fails). However, there are still other cases where a fatal error can be hidden from us (for example, a failed index writer commit). This commit modifies the mechanism for failing the engine to always check for a fatal error before failing the engine. --- .../elasticsearch/index/engine/Engine.java | 24 +++++++++++++++++++ .../index/engine/InternalEngine.java | 23 ------------------ 2 files changed, 24 insertions(+), 23 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/engine/Engine.java b/server/src/main/java/org/elasticsearch/index/engine/Engine.java index fb937ed4e9302..f9de998609661 100644 --- a/server/src/main/java/org/elasticsearch/index/engine/Engine.java +++ b/server/src/main/java/org/elasticsearch/index/engine/Engine.java @@ -81,6 +81,7 @@ import java.util.Locale; import java.util.Map; import java.util.Objects; +import java.util.Optional; import java.util.Set; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; @@ -847,11 +848,34 @@ public void forceMerge(boolean flush) throws IOException { */ public abstract IndexCommitRef acquireSafeIndexCommit() throws EngineException; + /** + * If the specified throwable contains a fatal error in the throwable graph, such a fatal error will be thrown. Callers should ensure + * that there are no catch statements that would catch an error in the stack as the fatal error here should go uncaught and be handled + * by the uncaught exception handler that we install during bootstrap. If the specified throwable dies indeed contain a fatal error, the + * specified message will attempt to be logged before throwing the fatal error. If the specified throwable does not contain a fatal + * error, this method is a no-op. + * + * @param maybeMessage the message to maybe log + * @param maybeFatal the throwable that maybe contains a fatal error + */ + @SuppressWarnings("finally") + private void maybeDie(final String maybeMessage, final Throwable maybeFatal) { + final Optional maybeError = ExceptionsHelper.maybeError(maybeFatal, logger); + if (maybeError.isPresent()) { + try { + logger.error(maybeMessage, maybeError.get()); + } finally { + throw maybeError.get(); + } + } + } + /** * fail engine due to some error. the engine will also be closed. * The underlying store is marked corrupted iff failure is caused by index corruption */ public void failEngine(String reason, @Nullable Exception failure) { + maybeDie(reason, failure); if (failEngineLock.tryLock()) { store.incRef(); try { diff --git a/server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java b/server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java index 100a133042d74..0b67ab21329ef 100644 --- a/server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java +++ b/server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java @@ -1730,7 +1730,6 @@ private boolean failOnTragicEvent(AlreadyClosedException ex) { // we need to fail the engine. it might have already been failed before // but we are double-checking it's failed and closed if (indexWriter.isOpen() == false && indexWriter.getTragicException() != null) { - maybeDie("tragic event in index writer", indexWriter.getTragicException()); failEngine("already closed by tragic event on the index writer", (Exception) indexWriter.getTragicException()); engineFailed = true; } else if (translog.isOpen() == false && translog.getTragicException() != null) { @@ -2080,34 +2079,12 @@ protected void doRun() throws Exception { * confidence that the call stack does not contain catch statements that would cause the error that might be thrown * here from being caught and never reaching the uncaught exception handler. */ - maybeDie("fatal error while merging", exc); - logger.error("failed to merge", exc); failEngine("merge failed", new MergePolicy.MergeException(exc, dir)); } }); } } - /** - * If the specified throwable is a fatal error, this throwable will be thrown. Callers should ensure that there are no catch statements - * that would catch an error in the stack as the fatal error here should go uncaught and be handled by the uncaught exception handler - * that we install during bootstrap. If the specified throwable is indeed a fatal error, the specified message will attempt to be logged - * before throwing the fatal error. If the specified throwable is not a fatal error, this method is a no-op. - * - * @param maybeMessage the message to maybe log - * @param maybeFatal the throwable that is maybe fatal - */ - @SuppressWarnings("finally") - private void maybeDie(final String maybeMessage, final Throwable maybeFatal) { - if (maybeFatal instanceof Error) { - try { - logger.error(maybeMessage, maybeFatal); - } finally { - throw (Error) maybeFatal; - } - } - } - /** * Commits the specified index writer. * From 06ffa668b79ca646981f13cd2e58930111b65816 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Fri, 9 Mar 2018 17:41:06 -0500 Subject: [PATCH 2/4] Simplify --- .../java/org/elasticsearch/index/engine/Engine.java | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/engine/Engine.java b/server/src/main/java/org/elasticsearch/index/engine/Engine.java index f9de998609661..55ca52dd3ab21 100644 --- a/server/src/main/java/org/elasticsearch/index/engine/Engine.java +++ b/server/src/main/java/org/elasticsearch/index/engine/Engine.java @@ -81,7 +81,6 @@ import java.util.Locale; import java.util.Map; import java.util.Objects; -import java.util.Optional; import java.util.Set; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; @@ -860,14 +859,13 @@ public void forceMerge(boolean flush) throws IOException { */ @SuppressWarnings("finally") private void maybeDie(final String maybeMessage, final Throwable maybeFatal) { - final Optional maybeError = ExceptionsHelper.maybeError(maybeFatal, logger); - if (maybeError.isPresent()) { + ExceptionsHelper.maybeError(maybeFatal, logger).ifPresent(error -> { try { - logger.error(maybeMessage, maybeError.get()); + logger.error(maybeMessage, error); } finally { - throw maybeError.get(); + throw error; } - } + }); } /** From 7db85fb62ef10b0889f25699bfb285681d3a725f Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Fri, 9 Mar 2018 18:10:14 -0500 Subject: [PATCH 3/4] Null check --- .../src/main/java/org/elasticsearch/index/engine/Engine.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/index/engine/Engine.java b/server/src/main/java/org/elasticsearch/index/engine/Engine.java index 55ca52dd3ab21..ef5b95f38ba3c 100644 --- a/server/src/main/java/org/elasticsearch/index/engine/Engine.java +++ b/server/src/main/java/org/elasticsearch/index/engine/Engine.java @@ -873,7 +873,9 @@ private void maybeDie(final String maybeMessage, final Throwable maybeFatal) { * The underlying store is marked corrupted iff failure is caused by index corruption */ public void failEngine(String reason, @Nullable Exception failure) { - maybeDie(reason, failure); + if (failure != null) { + maybeDie(reason, failure); + } if (failEngineLock.tryLock()) { store.incRef(); try { From 083572b1bb291698fe76839d7b75dc7b43805c32 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Fri, 9 Mar 2018 22:49:09 -0500 Subject: [PATCH 4/4] Fix typo --- server/src/main/java/org/elasticsearch/index/engine/Engine.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/index/engine/Engine.java b/server/src/main/java/org/elasticsearch/index/engine/Engine.java index ef5b95f38ba3c..1452c5de49278 100644 --- a/server/src/main/java/org/elasticsearch/index/engine/Engine.java +++ b/server/src/main/java/org/elasticsearch/index/engine/Engine.java @@ -850,7 +850,7 @@ public void forceMerge(boolean flush) throws IOException { /** * If the specified throwable contains a fatal error in the throwable graph, such a fatal error will be thrown. Callers should ensure * that there are no catch statements that would catch an error in the stack as the fatal error here should go uncaught and be handled - * by the uncaught exception handler that we install during bootstrap. If the specified throwable dies indeed contain a fatal error, the + * by the uncaught exception handler that we install during bootstrap. If the specified throwable does indeed contain a fatal error, the * specified message will attempt to be logged before throwing the fatal error. If the specified throwable does not contain a fatal * error, this method is a no-op. *