Redo #12707: Do not rely on isAlive() status of MemorySegment#Scope and make sure IndexInput#close() does not throw IllegalStateException and waits instead (#12785)

lucene/CHANGES.txt

@@ -208,8 +208,11 @@ Improvements
 
 * GITHUB#12586: Remove over-counting of deleted terms. (Guo Feng)
 
-* GITHUB#12705, GITHUB#12705: Improve handling of NullPointerException and IllegalStateException
+* GITHUB#12705, GITHUB#12705, GITHUB#12785: Improve handling of NullPointerException and
-  in MMapDirectory's IndexInputs.  (Uwe Schindler, Michael Sokolov)
+  IllegalStateException in MMapDirectory's IndexInputs. Also makes sure to close master
+  MemorySegmentIndexInput while not throwing IllegalStateException (retry in spin loop).
+  Also improve TestMmapDirectory.testAceWithThreads to run faster and use less resources.
+  (Uwe Schindler, Maurizio Cimadamore, Michael Sokolov)
 
 * GITHUB#12689: TaskExecutor to cancel all tasks on exception to avoid needless computation. (Luca Cavanna)
 

lucene/core/src/java19/org/apache/lucene/store/MemorySegmentIndexInput.java

@@ -105,12 +105,15 @@ abstract class MemorySegmentIndexInput extends IndexInput implements RandomAcces
   AlreadyClosedException alreadyClosed(RuntimeException e) {
     // we use NPE to signal if this input is closed (to not have checks everywhere). If NPE happens,
     // we check the "is closed" condition explicitly by checking that our "curSegment" is null.
+    // Care must be taken to not leak the NPE to code outside MemorySegmentIndexInput!
     if (this.curSegment == null) {
       return new AlreadyClosedException("Already closed: " + this);
     }
-    // we also check if the session of all segments is still alive:
+    // ISE can be thrown by MemorySegment and contains "closed" in message:
-    if (Arrays.stream(segments).allMatch(s -> s.session().isAlive()) == false) {
+    if (e instanceof IllegalStateException
-      return new AlreadyClosedException("Already closed: " + this);
+        && e.getMessage() != null
+        && e.getMessage().contains("closed")) {
+      return new AlreadyClosedException("Already closed: " + this, e);
     }
     // otherwise rethrow unmodified NPE/ISE (as it possibly a bug with passing a null parameter to
     // the IndexInput method):
@@ -472,17 +475,26 @@ abstract class MemorySegmentIndexInput extends IndexInput implements RandomAcces
       return;
     }
 
-    // make sure all accesses to this IndexInput instance throw NPE:
-    curSegment = null;
-    Arrays.fill(segments, null);
-
     // the master IndexInput has a MemorySession and is able
     // to release all resources (unmap segments) - a
     // side effect is that other threads still using clones
     // will throw IllegalStateException
     if (session != null) {
-      session.close();
+      while (session.isAlive()) {
+        try {
+          session.close();
+          break;
+        } catch (
+            @SuppressWarnings("unused")
+            IllegalStateException e) {
+          Thread.onSpinWait();
+        }
+      }
     }
+
+    // make sure all accesses to this IndexInput instance throw NPE:
+    curSegment = null;
+    Arrays.fill(segments, null);
   }
 
   /** Optimization of MemorySegmentIndexInput for when there is only one segment. */

lucene/core/src/java20/org/apache/lucene/store/MemorySegmentIndexInput.java

@@ -103,12 +103,15 @@ abstract class MemorySegmentIndexInput extends IndexInput implements RandomAcces
   AlreadyClosedException alreadyClosed(RuntimeException e) {
     // we use NPE to signal if this input is closed (to not have checks everywhere). If NPE happens,
     // we check the "is closed" condition explicitly by checking that our "curSegment" is null.
+    // Care must be taken to not leak the NPE to code outside MemorySegmentIndexInput!
     if (this.curSegment == null) {
       return new AlreadyClosedException("Already closed: " + this);
     }
-    // we also check if the scope of all segments is still alive:
+    // ISE can be thrown by MemorySegment and contains "closed" in message:
-    if (Arrays.stream(segments).allMatch(s -> s.scope().isAlive()) == false) {
+    if (e instanceof IllegalStateException
-      return new AlreadyClosedException("Already closed: " + this);
+        && e.getMessage() != null
+        && e.getMessage().contains("closed")) {
+      return new AlreadyClosedException("Already closed: " + this, e);
     }
     // otherwise rethrow unmodified NPE/ISE (as it possibly a bug with passing a null parameter to
     // the IndexInput method):
@@ -470,17 +473,26 @@ abstract class MemorySegmentIndexInput extends IndexInput implements RandomAcces
       return;
     }
 
-    // make sure all accesses to this IndexInput instance throw NPE:
-    curSegment = null;
-    Arrays.fill(segments, null);
-
     // the master IndexInput has an Arena and is able
     // to release all resources (unmap segments) - a
     // side effect is that other threads still using clones
     // will throw IllegalStateException
     if (arena != null) {
-      arena.close();
+      while (arena.scope().isAlive()) {
+        try {
+          arena.close();
+          break;
+        } catch (
+            @SuppressWarnings("unused")
+            IllegalStateException e) {
+          Thread.onSpinWait();
+        }
+      }
     }
+
+    // make sure all accesses to this IndexInput instance throw NPE:
+    curSegment = null;
+    Arrays.fill(segments, null);
   }
 
   /** Optimization of MemorySegmentIndexInput for when there is only one segment. */

lucene/core/src/java21/org/apache/lucene/store/MemorySegmentIndexInput.java

@@ -103,12 +103,15 @@ abstract class MemorySegmentIndexInput extends IndexInput implements RandomAcces
   AlreadyClosedException alreadyClosed(RuntimeException e) {
     // we use NPE to signal if this input is closed (to not have checks everywhere). If NPE happens,
     // we check the "is closed" condition explicitly by checking that our "curSegment" is null.
+    // Care must be taken to not leak the NPE to code outside MemorySegmentIndexInput!
     if (this.curSegment == null) {
       return new AlreadyClosedException("Already closed: " + this);
     }
-    // we also check if the scope of all segments is still alive:
+    // ISE can be thrown by MemorySegment and contains "closed" in message:
-    if (Arrays.stream(segments).allMatch(s -> s.scope().isAlive()) == false) {
+    if (e instanceof IllegalStateException
-      return new AlreadyClosedException("Already closed: " + this);
+        && e.getMessage() != null
+        && e.getMessage().contains("closed")) {
+      return new AlreadyClosedException("Already closed: " + this, e);
     }
     // otherwise rethrow unmodified NPE/ISE (as it possibly a bug with passing a null parameter to
     // the IndexInput method):
@@ -470,17 +473,26 @@ abstract class MemorySegmentIndexInput extends IndexInput implements RandomAcces
       return;
     }
 
-    // make sure all accesses to this IndexInput instance throw NPE:
-    curSegment = null;
-    Arrays.fill(segments, null);
-
     // the master IndexInput has an Arena and is able
     // to release all resources (unmap segments) - a
     // side effect is that other threads still using clones
     // will throw IllegalStateException
     if (arena != null) {
-      arena.close();
+      while (arena.scope().isAlive()) {
+        try {
+          arena.close();
+          break;
+        } catch (
+            @SuppressWarnings("unused")
+            IllegalStateException e) {
+          Thread.onSpinWait();
+        }
+      }
     }
+
+    // make sure all accesses to this IndexInput instance throw NPE:
+    curSegment = null;
+    Arrays.fill(segments, null);
   }
 
   /** Optimization of MemorySegmentIndexInput for when there is only one segment. */

lucene/core/src/test/org/apache/lucene/store/TestMmapDirectory.java

@@ -60,48 +60,45 @@ public class TestMmapDirectory extends BaseDirectoryTestCase {
   public void testAceWithThreads() throws Exception {
     assumeTrue("Test requires MemorySegmentIndexInput", isMemorySegmentImpl());
 
-    final int iters = RANDOM_MULTIPLIER * (TEST_NIGHTLY ? 50 : 10);
+    final int nInts = 8 * 1024 * 1024;
-    for (int iter = 0; iter < iters; iter++) {
+
-      Directory dir = getDirectory(createTempDir("testAceWithThreads"));
+    try (Directory dir = getDirectory(createTempDir("testAceWithThreads"))) {
-      IndexOutput out = dir.createOutput("test", IOContext.DEFAULT);
+      try (IndexOutput out = dir.createOutput("test", IOContext.DEFAULT)) {
-      Random random = random();
+        final Random random = random();
-      for (int i = 0; i < 8 * 1024 * 1024; i++) {
+        for (int i = 0; i < nInts; i++) {
-        out.writeInt(random.nextInt());
+          out.writeInt(random.nextInt());
+        }
       }
-      out.close();
+
-      IndexInput in = dir.openInput("test", IOContext.DEFAULT);
+      final int iters = RANDOM_MULTIPLIER * (TEST_NIGHTLY ? 50 : 10);
-      IndexInput clone = in.clone();
+      for (int iter = 0; iter < iters; iter++) {
-      final byte[] accum = new byte[32 * 1024 * 1024];
+        final IndexInput in = dir.openInput("test", IOContext.DEFAULT);
-      final CountDownLatch shotgun = new CountDownLatch(1);
+        final IndexInput clone = in.clone();
-      Thread t1 =
+        final byte[] accum = new byte[nInts * Integer.BYTES];
-          new Thread(
+        final CountDownLatch shotgun = new CountDownLatch(1);
-              () -> {
+        final Thread t1 =
-                try {
+            new Thread(
-                  shotgun.await();
+                () -> {
-                  for (int i = 0; i < 10; i++) {
+                  try {
-                    clone.seek(0);
+                    shotgun.await();
-                    clone.readBytes(accum, 0, accum.length);
+                    for (int i = 0; i < 10; i++) {
+                      clone.seek(0);
+                      clone.readBytes(accum, 0, accum.length);
+                    }
+                  } catch (
+                      @SuppressWarnings("unused")
+                      AlreadyClosedException ok) {
+                    // OK
+                  } catch (InterruptedException | IOException e) {
+                    throw new RuntimeException(e);
                   }
-                } catch (@SuppressWarnings("unused") IOException | AlreadyClosedException ok) {
+                });
-                  // OK
+        t1.start();
-                } catch (InterruptedException e) {
+        shotgun.countDown();
-                  throw new RuntimeException(e);
+        // this triggers "bad behaviour": closing input while other threads are running
-                }
-              });
-      t1.start();
-      shotgun.countDown();
-      try {
         in.close();
-      } catch (
+        t1.join();
-          @SuppressWarnings("unused")
-          IllegalStateException ise) {
-        // this may also happen and is a valid exception, informing our user that, e.g., a query is
-        // running!
-        // "java.lang.IllegalStateException: Cannot close while another thread is accessing the
-        // segment"
       }
-      t1.join();
-      dir.close();
     }
   }