Port generic exception handling from MemorySegmentIndexInput to ByteBufferIndexInput (#11918)

Port generic exception handling from MemorySegmentIndexInput to ByteBufferIndexInput. This also adds the invalid position while seeking or reading to the exception message.

lucene/CHANGES.txt

@@ -192,6 +192,13 @@ Bug Fixes
  This addresses a bug that was introduced in 9.2.0 where having many vectors is not handled well
  in the vector connections reader.
 
+Improvements
+---------------------
+* GITHUB#11912, GITHUB#11918: Port generic exception handling from MemorySegmentIndexInput
+  to ByteBufferIndexInput. This also adds the invalid position while seeking or reading
+  to the exception message. Allows better debugging and analysis of bugs like GITHUB#11905.
+  (Uwe Schindler, Robert Muir)
+
 ======================== Lucene 9.4.1 =======================
 
 Bug Fixes

lucene/core/src/java/org/apache/lucene/store/ByteBufferIndexInput.java

@@ -89,6 +89,21 @@ public abstract class ByteBufferIndexInput extends IndexInput implements RandomA
     curIntBufferViews = null;
   }
 
+  // the unused parameter is just to silence javac about unused variables
+  RuntimeException handlePositionalIOOBE(RuntimeException unused, String action, long pos)
+      throws IOException {
+    if (pos < 0L) {
+      return new IllegalArgumentException(action + " negative position (pos=" + pos + "): " + this);
+    } else {
+      throw new EOFException(action + " past EOF (pos=" + pos + "): " + this);
+    }
+  }
+
+  // the unused parameter is just to silence javac about unused variables
+  AlreadyClosedException alreadyClosed(RuntimeException unused) {
+    return new AlreadyClosedException("Already closed: " + this);
+  }
+
   @Override
   public final byte readByte() throws IOException {
     try {
@@ -105,10 +120,8 @@ public abstract class ByteBufferIndexInput extends IndexInput implements RandomA
         curBuf.position(0);
       } while (!curBuf.hasRemaining());
       return guard.getByte(curBuf);
-    } catch (
+    } catch (NullPointerException e) {
-        @SuppressWarnings("unused")
+      throw alreadyClosed(e);
-        NullPointerException npe) {
-      throw new AlreadyClosedException("Already closed: " + this);
     }
   }
 
@@ -133,10 +146,8 @@ public abstract class ByteBufferIndexInput extends IndexInput implements RandomA
         curAvail = curBuf.remaining();
       }
       guard.getBytes(curBuf, b, offset, len);
-    } catch (
+    } catch (NullPointerException e) {
-        @SuppressWarnings("unused")
+      throw alreadyClosed(e);
-        NullPointerException npe) {
-      throw new AlreadyClosedException("Already closed: " + this);
     }
   }
 
@@ -173,10 +184,8 @@ public abstract class ByteBufferIndexInput extends IndexInput implements RandomA
         @SuppressWarnings("unused")
         BufferUnderflowException e) {
       super.readLongs(dst, offset, length);
-    } catch (
+    } catch (NullPointerException e) {
-        @SuppressWarnings("unused")
+      throw alreadyClosed(e);
-        NullPointerException npe) {
-      throw new AlreadyClosedException("Already closed: " + this);
     }
   }
 
@@ -204,10 +213,8 @@ public abstract class ByteBufferIndexInput extends IndexInput implements RandomA
         @SuppressWarnings("unused")
         BufferUnderflowException e) {
       super.readInts(dst, offset, length);
-    } catch (
+    } catch (NullPointerException e) {
-        @SuppressWarnings("unused")
+      throw alreadyClosed(e);
-        NullPointerException npe) {
-      throw new AlreadyClosedException("Already closed: " + this);
     }
   }
 
@@ -238,10 +245,8 @@ public abstract class ByteBufferIndexInput extends IndexInput implements RandomA
         @SuppressWarnings("unused")
         BufferUnderflowException e) {
       super.readFloats(floats, offset, len);
-    } catch (
+    } catch (NullPointerException e) {
-        @SuppressWarnings("unused")
+      throw alreadyClosed(e);
-        NullPointerException npe) {
-      throw new AlreadyClosedException("Already closed: " + this);
     }
   }
 
@@ -253,10 +258,8 @@ public abstract class ByteBufferIndexInput extends IndexInput implements RandomA
         @SuppressWarnings("unused")
         BufferUnderflowException e) {
       return super.readShort();
-    } catch (
+    } catch (NullPointerException e) {
-        @SuppressWarnings("unused")
+      throw alreadyClosed(e);
-        NullPointerException npe) {
-      throw new AlreadyClosedException("Already closed: " + this);
     }
   }
 
@@ -268,10 +271,8 @@ public abstract class ByteBufferIndexInput extends IndexInput implements RandomA
         @SuppressWarnings("unused")
         BufferUnderflowException e) {
       return super.readInt();
-    } catch (
+    } catch (NullPointerException e) {
-        @SuppressWarnings("unused")
+      throw alreadyClosed(e);
-        NullPointerException npe) {
-      throw new AlreadyClosedException("Already closed: " + this);
     }
   }
 
@@ -283,10 +284,8 @@ public abstract class ByteBufferIndexInput extends IndexInput implements RandomA
         @SuppressWarnings("unused")
         BufferUnderflowException e) {
       return super.readLong();
-    } catch (
+    } catch (NullPointerException e) {
-        @SuppressWarnings("unused")
+      throw alreadyClosed(e);
-        NullPointerException npe) {
-      throw new AlreadyClosedException("Already closed: " + this);
     }
   }
 
@@ -294,10 +293,8 @@ public abstract class ByteBufferIndexInput extends IndexInput implements RandomA
   public long getFilePointer() {
     try {
       return (((long) curBufIndex) << chunkSizePower) + curBuf.position();
-    } catch (
+    } catch (NullPointerException e) {
-        @SuppressWarnings("unused")
+      throw alreadyClosed(e);
-        NullPointerException npe) {
-      throw new AlreadyClosedException("Already closed: " + this);
     }
   }
 
@@ -316,14 +313,10 @@ public abstract class ByteBufferIndexInput extends IndexInput implements RandomA
         this.curBufIndex = bi;
         setCurBuf(b);
       }
-    } catch (@SuppressWarnings("unused")
+    } catch (ArrayIndexOutOfBoundsException | IllegalArgumentException e) {
-        ArrayIndexOutOfBoundsException
+      throw handlePositionalIOOBE(e, "seek", pos);
-        | IllegalArgumentException e) {
+    } catch (NullPointerException e) {
-      throw new EOFException("seek past EOF: " + this);
+      throw alreadyClosed(e);
-    } catch (
-        @SuppressWarnings("unused")
-        NullPointerException npe) {
-      throw new AlreadyClosedException("Already closed: " + this);
     }
   }
 
@@ -332,14 +325,10 @@ public abstract class ByteBufferIndexInput extends IndexInput implements RandomA
     try {
       final int bi = (int) (pos >> chunkSizePower);
       return guard.getByte(buffers[bi], (int) (pos & chunkSizeMask));
-    } catch (
+    } catch (IndexOutOfBoundsException ioobe) {
-        @SuppressWarnings("unused")
+      throw handlePositionalIOOBE(ioobe, "read", pos);
-        IndexOutOfBoundsException ioobe) {
+    } catch (NullPointerException e) {
-      throw new EOFException("seek past EOF: " + this);
+      throw alreadyClosed(e);
-    } catch (
-        @SuppressWarnings("unused")
-        NullPointerException npe) {
-      throw new AlreadyClosedException("Already closed: " + this);
     }
   }
 
@@ -350,14 +339,10 @@ public abstract class ByteBufferIndexInput extends IndexInput implements RandomA
       b.position((int) (pos & chunkSizeMask));
       this.curBufIndex = bi;
       setCurBuf(b);
-    } catch (@SuppressWarnings("unused")
+    } catch (ArrayIndexOutOfBoundsException | IllegalArgumentException aioobe) {
-        ArrayIndexOutOfBoundsException
+      throw handlePositionalIOOBE(aioobe, "read", pos);
-        | IllegalArgumentException aioobe) {
+    } catch (NullPointerException e) {
-      throw new EOFException("seek past EOF: " + this);
+      throw alreadyClosed(e);
-    } catch (
-        @SuppressWarnings("unused")
-        NullPointerException npe) {
-      throw new AlreadyClosedException("Already closed: " + this);
     }
   }
 
@@ -372,10 +357,8 @@ public abstract class ByteBufferIndexInput extends IndexInput implements RandomA
       // either it's a boundary, or read past EOF, fall back:
       setPos(pos, bi);
       return readShort();
-    } catch (
+    } catch (NullPointerException e) {
-        @SuppressWarnings("unused")
+      throw alreadyClosed(e);
-        NullPointerException npe) {
-      throw new AlreadyClosedException("Already closed: " + this);
     }
   }
 
@@ -390,10 +373,8 @@ public abstract class ByteBufferIndexInput extends IndexInput implements RandomA
       // either it's a boundary, or read past EOF, fall back:
       setPos(pos, bi);
       return readInt();
-    } catch (
+    } catch (NullPointerException e) {
-        @SuppressWarnings("unused")
+      throw alreadyClosed(e);
-        NullPointerException npe) {
-      throw new AlreadyClosedException("Already closed: " + this);
     }
   }
 
@@ -408,10 +389,8 @@ public abstract class ByteBufferIndexInput extends IndexInput implements RandomA
       // either it's a boundary, or read past EOF, fall back:
       setPos(pos, bi);
       return readLong();
-    } catch (
+    } catch (NullPointerException e) {
-        @SuppressWarnings("unused")
+      throw alreadyClosed(e);
-        NullPointerException npe) {
-      throw new AlreadyClosedException("Already closed: " + this);
     }
   }
 
@@ -458,7 +437,7 @@ public abstract class ByteBufferIndexInput extends IndexInput implements RandomA
   /** Builds the actual sliced IndexInput (may apply extra offset in subclasses). * */
   protected ByteBufferIndexInput buildSlice(String sliceDescription, long offset, long length) {
     if (buffers == null) {
-      throw new AlreadyClosedException("Already closed: " + this);
+      throw alreadyClosed(null);
     }
 
     final ByteBuffer[] newBuffers = buildSlice(buffers, offset, length);
@@ -564,15 +543,9 @@ public abstract class ByteBufferIndexInput extends IndexInput implements RandomA
       try {
         curBuf.position((int) pos);
       } catch (IllegalArgumentException e) {
-        if (pos < 0) {
+        throw handlePositionalIOOBE(e, "seek", pos);
-          throw new IllegalArgumentException("Seeking to negative position: " + this, e);
+      } catch (NullPointerException e) {
-        } else {
+        throw alreadyClosed(e);
-          throw new EOFException("seek past EOF: " + this);
-        }
-      } catch (
-          @SuppressWarnings("unused")
-          NullPointerException npe) {
-        throw new AlreadyClosedException("Already closed: " + this);
       }
     }
 
@@ -580,10 +553,8 @@ public abstract class ByteBufferIndexInput extends IndexInput implements RandomA
     public long getFilePointer() {
       try {
         return curBuf.position();
-      } catch (
+      } catch (NullPointerException e) {
-          @SuppressWarnings("unused")
+        throw alreadyClosed(e);
-          NullPointerException npe) {
-        throw new AlreadyClosedException("Already closed: " + this);
       }
     }
 
@@ -592,15 +563,9 @@ public abstract class ByteBufferIndexInput extends IndexInput implements RandomA
       try {
         return guard.getByte(curBuf, (int) pos);
       } catch (IllegalArgumentException e) {
-        if (pos < 0) {
+        throw handlePositionalIOOBE(e, "read", pos);
-          throw new IllegalArgumentException("Seeking to negative position: " + this, e);
+      } catch (NullPointerException e) {
-        } else {
+        throw alreadyClosed(e);
-          throw new EOFException("seek past EOF: " + this);
-        }
-      } catch (
-          @SuppressWarnings("unused")
-          NullPointerException npe) {
-        throw new AlreadyClosedException("Already closed: " + this);
       }
     }
 
@@ -609,15 +574,9 @@ public abstract class ByteBufferIndexInput extends IndexInput implements RandomA
       try {
         return guard.getShort(curBuf, (int) pos);
       } catch (IllegalArgumentException e) {
-        if (pos < 0) {
+        throw handlePositionalIOOBE(e, "read", pos);
-          throw new IllegalArgumentException("Seeking to negative position: " + this, e);
+      } catch (NullPointerException e) {
-        } else {
+        throw alreadyClosed(e);
-          throw new EOFException("seek past EOF: " + this);
-        }
-      } catch (
-          @SuppressWarnings("unused")
-          NullPointerException npe) {
-        throw new AlreadyClosedException("Already closed: " + this);
       }
     }
 
@@ -626,15 +585,9 @@ public abstract class ByteBufferIndexInput extends IndexInput implements RandomA
       try {
         return guard.getInt(curBuf, (int) pos);
       } catch (IllegalArgumentException e) {
-        if (pos < 0) {
+        throw handlePositionalIOOBE(e, "read", pos);
-          throw new IllegalArgumentException("Seeking to negative position: " + this, e);
+      } catch (NullPointerException e) {
-        } else {
+        throw alreadyClosed(e);
-          throw new EOFException("seek past EOF: " + this);
-        }
-      } catch (
-          @SuppressWarnings("unused")
-          NullPointerException npe) {
-        throw new AlreadyClosedException("Already closed: " + this);
       }
     }
 
@@ -643,15 +596,9 @@ public abstract class ByteBufferIndexInput extends IndexInput implements RandomA
       try {
         return guard.getLong(curBuf, (int) pos);
       } catch (IllegalArgumentException e) {
-        if (pos < 0) {
+        throw handlePositionalIOOBE(e, "read", pos);
-          throw new IllegalArgumentException("Seeking to negative position: " + this, e);
+      } catch (NullPointerException e) {
-        } else {
+        throw alreadyClosed(e);
-          throw new EOFException("seek past EOF: " + this);
-        }
-      } catch (
-          @SuppressWarnings("unused")
-          NullPointerException npe) {
-        throw new AlreadyClosedException("Already closed: " + this);
       }
     }
   }
@@ -676,9 +623,15 @@ public abstract class ByteBufferIndexInput extends IndexInput implements RandomA
       }
     }
 
+    @Override
+    RuntimeException handlePositionalIOOBE(RuntimeException unused, String action, long pos)
+        throws IOException {
+      return super.handlePositionalIOOBE(unused, action, pos - offset);
+    }
+
     @Override
     public void seek(long pos) throws IOException {
-      assert pos >= 0L;
+      assert pos >= 0L : "negative position";
       super.seek(pos + offset);
     }
 

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

@@ -92,11 +92,13 @@ abstract class MemorySegmentIndexInput extends IndexInput implements RandomAcces
     }
   }
 
-  RuntimeException handlePositionalIOOBE(String action, long pos) throws IOException {
+  // the unused parameter is just to silence javac about unused variables
+  RuntimeException handlePositionalIOOBE(RuntimeException unused, String action, long pos)
+      throws IOException {
     if (pos < 0L) {
-      return new IllegalArgumentException(action + " negative position: " + this);
+      return new IllegalArgumentException(action + " negative position (pos=" + pos + "): " + this);
     } else {
-      throw new EOFException(action + " past EOF: " + this);
+      throw new EOFException(action + " past EOF (pos=" + pos + "): " + this);
     }
   }
 
@@ -273,10 +275,8 @@ abstract class MemorySegmentIndexInput extends IndexInput implements RandomAcces
         this.curSegment = seg;
       }
       this.curPosition = Objects.checkIndex(pos & chunkSizeMask, curSegment.byteSize() + 1);
-    } catch (
+    } catch (IndexOutOfBoundsException e) {
-        @SuppressWarnings("unused")
+      throw handlePositionalIOOBE(e, "seek", pos);
-        IndexOutOfBoundsException e) {
-      throw handlePositionalIOOBE("seek", pos);
     }
   }
 
@@ -285,10 +285,8 @@ abstract class MemorySegmentIndexInput extends IndexInput implements RandomAcces
     try {
       final int si = (int) (pos >> chunkSizePower);
       return segments[si].get(LAYOUT_BYTE, pos & chunkSizeMask);
-    } catch (
+    } catch (IndexOutOfBoundsException ioobe) {
-        @SuppressWarnings("unused")
+      throw handlePositionalIOOBE(ioobe, "read", pos);
-        IndexOutOfBoundsException ioobe) {
-      throw handlePositionalIOOBE("read", pos);
     } catch (NullPointerException | IllegalStateException e) {
       throw alreadyClosed(e);
     }
@@ -302,10 +300,8 @@ abstract class MemorySegmentIndexInput extends IndexInput implements RandomAcces
       this.curPosition = pos & chunkSizeMask;
       this.curSegmentIndex = si;
       this.curSegment = seg;
-    } catch (
+    } catch (IndexOutOfBoundsException ioobe) {
-        @SuppressWarnings("unused")
+      throw handlePositionalIOOBE(ioobe, "read", pos);
-        IndexOutOfBoundsException ioobe) {
-      throw handlePositionalIOOBE("read", pos);
     } catch (NullPointerException | IllegalStateException e) {
       throw alreadyClosed(e);
     }
@@ -472,10 +468,8 @@ abstract class MemorySegmentIndexInput extends IndexInput implements RandomAcces
       ensureOpen();
       try {
         curPosition = Objects.checkIndex(pos, length + 1);
-      } catch (
+      } catch (IndexOutOfBoundsException e) {
-          @SuppressWarnings("unused")
+        throw handlePositionalIOOBE(e, "seek", pos);
-          IndexOutOfBoundsException e) {
-        throw handlePositionalIOOBE("seek", pos);
       }
     }
 
@@ -489,10 +483,8 @@ abstract class MemorySegmentIndexInput extends IndexInput implements RandomAcces
     public byte readByte(long pos) throws IOException {
       try {
         return curSegment.get(LAYOUT_BYTE, pos);
-      } catch (
+      } catch (IndexOutOfBoundsException e) {
-          @SuppressWarnings("unused")
+        throw handlePositionalIOOBE(e, "read", pos);
-          IndexOutOfBoundsException e) {
-        throw handlePositionalIOOBE("read", pos);
       } catch (NullPointerException | IllegalStateException e) {
         throw alreadyClosed(e);
       }
@@ -502,10 +494,8 @@ abstract class MemorySegmentIndexInput extends IndexInput implements RandomAcces
     public short readShort(long pos) throws IOException {
       try {
         return curSegment.get(LAYOUT_LE_SHORT, pos);
-      } catch (
+      } catch (IndexOutOfBoundsException e) {
-          @SuppressWarnings("unused")
+        throw handlePositionalIOOBE(e, "read", pos);
-          IndexOutOfBoundsException e) {
-        throw handlePositionalIOOBE("read", pos);
       } catch (NullPointerException | IllegalStateException e) {
         throw alreadyClosed(e);
       }
@@ -515,10 +505,8 @@ abstract class MemorySegmentIndexInput extends IndexInput implements RandomAcces
     public int readInt(long pos) throws IOException {
       try {
         return curSegment.get(LAYOUT_LE_INT, pos);
-      } catch (
+      } catch (IndexOutOfBoundsException e) {
-          @SuppressWarnings("unused")
+        throw handlePositionalIOOBE(e, "read", pos);
-          IndexOutOfBoundsException e) {
-        throw handlePositionalIOOBE("read", pos);
       } catch (NullPointerException | IllegalStateException e) {
         throw alreadyClosed(e);
       }
@@ -528,10 +516,8 @@ abstract class MemorySegmentIndexInput extends IndexInput implements RandomAcces
     public long readLong(long pos) throws IOException {
       try {
         return curSegment.get(LAYOUT_LE_LONG, pos);
-      } catch (
+      } catch (IndexOutOfBoundsException e) {
-          @SuppressWarnings("unused")
+        throw handlePositionalIOOBE(e, "read", pos);
-          IndexOutOfBoundsException e) {
-        throw handlePositionalIOOBE("read", pos);
       } catch (NullPointerException | IllegalStateException e) {
         throw alreadyClosed(e);
       }
@@ -559,9 +545,15 @@ abstract class MemorySegmentIndexInput extends IndexInput implements RandomAcces
       assert curSegment != null && curSegmentIndex >= 0;
     }
 
+    @Override
+    RuntimeException handlePositionalIOOBE(RuntimeException unused, String action, long pos)
+        throws IOException {
+      return super.handlePositionalIOOBE(unused, action, pos - offset);
+    }
+
     @Override
     public void seek(long pos) throws IOException {
-      assert pos >= 0L;
+      assert pos >= 0L : "negative position";
       super.seek(pos + offset);
     }
 

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

@@ -18,6 +18,7 @@ package org.apache.lucene.store;
 
 import java.io.IOException;
 import java.nio.file.Path;
+import java.util.List;
 import org.apache.lucene.tests.store.BaseChunkedDirectoryTestCase;
 import org.apache.lucene.util.BytesRef;
 import org.junit.BeforeClass;
@@ -40,6 +41,26 @@ public class TestMultiMMap extends BaseChunkedDirectoryTestCase {
     assertTrue(MMapDirectory.UNMAP_NOT_SUPPORTED_REASON, MMapDirectory.UNMAP_SUPPORTED);
   }
 
+  public void testSeekNegative() throws IOException {
+    try (Directory dir = getDirectory(createTempDir())) {
+      try (IndexOutput out = dir.createOutput("a", IOContext.DEFAULT)) {
+        for (int i = 0; i < 2048; ++i) {
+          out.writeByte((byte) 0);
+        }
+      }
+      try (IndexInput in = dir.openInput("a", IOContext.DEFAULT)) {
+        in.seek(1234);
+        assertEquals(1234, in.getFilePointer());
+        var e =
+            expectThrowsAnyOf(
+                List.of(IllegalArgumentException.class, AssertionError.class),
+                () -> in.seek(-1234));
+        assertTrue(
+            "does not mention negative position", e.getMessage().contains("negative position"));
+      }
+    }
+  }
+
   // TODO: can we improve ByteBuffersDirectory (without overhead) and move these clone safety tests
   // to the base test case?