Don't suppress AlreadyClosedException (#19975)

Catching and suppressing AlreadyClosedException from Lucene is dangerous because it can mean there is a bug in ES since ES should normally guard against invoking Lucene classes after they were closed.

I reviewed the cases where we catch AlreadyClosedException from Lucene and removed the ones that I believe are not needed, or improved comments explaining why ACE is OK in that case.

I think (@s1monw can you confirm?) that holding the engine's readLock means IW will not be closed, except if disaster strikes (failEngine) at which point I think it's fine to see the original ACE in the logs?

Closes #19861

core/src/main/java/org/elasticsearch/index/engine/Engine.java

@@ -591,7 +591,7 @@ public abstract class Engine implements Closeable {
               the store is closed so we need to make sure we increment it here
              */
             try {
-                return !getSearcherManager().isSearcherCurrent();
+                return getSearcherManager().isSearcherCurrent() == false;
             } catch (IOException e) {
                 logger.error("failed to access searcher manager", e);
                 failEngine("failed to access searcher manager", e);

core/src/main/java/org/elasticsearch/index/engine/EngineSearcher.java

@@ -59,9 +59,8 @@ public class EngineSearcher extends Engine.Searcher {
         } catch (IOException e) {
             throw new IllegalStateException("Cannot close", e);
         } catch (AlreadyClosedException e) {
-                /* this one can happen if we already closed the
+            // This means there's a bug somewhere: don't suppress it
-                 * underlying store / directory and we call into the
+            throw new AssertionError(e);
-                 * IndexWriter to free up pending files. */
         } finally {
             store.decRef();
         }

core/src/main/java/org/elasticsearch/index/engine/InternalEngine.java

@@ -562,8 +562,8 @@ public class InternalEngine extends Engine {
             ensureOpen();
             searcherManager.maybeRefreshBlocking();
         } catch (AlreadyClosedException e) {
-            ensureOpen();
+            failOnTragicEvent(e);
-            maybeFailEngine("refresh", e);
+            throw e;
         } catch (EngineClosedException e) {
             throw e;
         } catch (Exception e) {
@@ -610,8 +610,8 @@ public class InternalEngine extends Engine {
                 indexWriter.flush();
             }
         } catch (AlreadyClosedException e) {
-            ensureOpen();
+            failOnTragicEvent(e);
-            maybeFailEngine("writeIndexingBuffer", e);
+            throw e;
         } catch (EngineClosedException e) {
             throw e;
         } catch (Exception e) {
@@ -835,6 +835,14 @@ public class InternalEngine extends Engine {
             } finally {
                 store.decRef();
             }
+        } catch (AlreadyClosedException ex) {
+            /* in this case we first check if the engine is still open. If so this exception is just fine
+             * and expected. We don't hold any locks while we block on forceMerge otherwise it would block
+             * closing the engine as well. If we are not closed we pass it on to failOnTragicEvent which ensures
+             * we are handling a tragic even exception here */
+            ensureOpen();
+            failOnTragicEvent(ex);
+            throw ex;
         } catch (Exception e) {
             try {
                 maybeFailEngine("force merge", e);
@@ -869,15 +877,7 @@ public class InternalEngine extends Engine {
         }
     }
 
-    @Override
+    private void failOnTragicEvent(AlreadyClosedException ex) {
-    protected boolean maybeFailEngine(String source, Exception e) {
-        boolean shouldFail = super.maybeFailEngine(source, e);
-        if (shouldFail) {
-            return true;
-        }
-
-        // Check for AlreadyClosedException
-        if (e instanceof AlreadyClosedException) {
         // if we are already closed due to some tragic exception
         // we need to fail the engine. it might have already been failed before
         // but we are double-checking it's failed and closed
@@ -888,7 +888,24 @@ public class InternalEngine extends Engine {
             failEngine("already closed by tragic event on the index writer", tragedy);
         } else if (translog.isOpen() == false && translog.getTragicException() != null) {
             failEngine("already closed by tragic event on the translog", translog.getTragicException());
+        } else {
+            // this smells like a bug - we only expect ACE if we are in a fatal case ie. either translog or IW is closed by
+            // a tragic event or has closed itself. if that is not the case we are in a buggy state and raise an assertion error
+            throw new AssertionError("Unexpected AlreadyClosedException", ex);
         }
+    }
+
+    @Override
+    protected boolean maybeFailEngine(String source, Exception e) {
+        boolean shouldFail = super.maybeFailEngine(source, e);
+        if (shouldFail) {
+            return true;
+        }
+        // Check for AlreadyClosedException -- ACE is a very special
+        // exception that should only be thrown in a tragic event. we pass on the checks to failOnTragicEvent which will
+        // throw and AssertionError if the tragic event condition is not met.
+        if (e instanceof AlreadyClosedException) {
+            failOnTragicEvent((AlreadyClosedException)e);
             return true;
         } else if (e != null &&
             ((indexWriter.isOpen() == false && indexWriter.getTragicException() == e)
@@ -914,6 +931,7 @@ public class InternalEngine extends Engine {
 
     @Override
     public long getIndexBufferRAMBytesUsed() {
+        // We don't guard w/ readLock here, so we could throw AlreadyClosedException
         return indexWriter.ramBytesUsed() + versionMap.ramBytesUsedForRefresh();
     }
 
@@ -963,8 +981,9 @@ public class InternalEngine extends Engine {
                 logger.trace("rollback indexWriter");
                 try {
                     indexWriter.rollback();
-                } catch (AlreadyClosedException e) {
+                } catch (AlreadyClosedException ex) {
-                    // ignore
+                    failOnTragicEvent(ex);
+                    throw ex;
                 }
                 logger.trace("rollback indexWriter done");
             } catch (Exception e) {

core/src/main/java/org/elasticsearch/index/engine/ShadowEngine.java

@@ -191,7 +191,8 @@ public class ShadowEngine extends Engine {
             ensureOpen();
             searcherManager.maybeRefreshBlocking();
         } catch (AlreadyClosedException e) {
-            ensureOpen();
+            // This means there's a bug somewhere: don't suppress it
+            throw new AssertionError(e);
         } catch (EngineClosedException e) {
             throw e;
         } catch (Exception e) {