You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@lucene.apache.org by us...@apache.org on 2022/11/11 15:50:31 UTC
[lucene] branch branch_9_4 updated: Port generic exception handling from MemorySegmentIndexInput to ByteBufferIndexInput (#11918)
This is an automated email from the ASF dual-hosted git repository.
uschindler pushed a commit to branch branch_9_4
in repository https://gitbox.apache.org/repos/asf/lucene.git
The following commit(s) were added to refs/heads/branch_9_4 by this push:
new 3459b4f4083 Port generic exception handling from MemorySegmentIndexInput to ByteBufferIndexInput (#11918)
3459b4f4083 is described below
commit 3459b4f408371fcf0e830c2fe8a7cb76495fde40
Author: Uwe Schindler <us...@apache.org>
AuthorDate: Fri Nov 11 16:47:52 2022 +0100
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 | 7 +
.../apache/lucene/store/ByteBufferIndexInput.java | 199 ++++++++-------------
.../lucene/store/MemorySegmentIndexInput.java | 64 +++----
.../org/apache/lucene/store/TestMultiMMap.java | 21 +++
4 files changed, 132 insertions(+), 159 deletions(-)
diff --git a/lucene/CHANGES.txt b/lucene/CHANGES.txt
index 42352aca742..df3286fe2b0 100644
--- a/lucene/CHANGES.txt
+++ b/lucene/CHANGES.txt
@@ -15,6 +15,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
diff --git a/lucene/core/src/java/org/apache/lucene/store/ByteBufferIndexInput.java b/lucene/core/src/java/org/apache/lucene/store/ByteBufferIndexInput.java
index 3229cde0845..cd856018841 100644
--- a/lucene/core/src/java/org/apache/lucene/store/ByteBufferIndexInput.java
+++ b/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 (
- @SuppressWarnings("unused")
- NullPointerException npe) {
- throw new AlreadyClosedException("Already closed: " + this);
+ } catch (NullPointerException e) {
+ throw alreadyClosed(e);
}
}
@@ -133,10 +146,8 @@ public abstract class ByteBufferIndexInput extends IndexInput implements RandomA
curAvail = curBuf.remaining();
}
guard.getBytes(curBuf, b, offset, len);
- } catch (
- @SuppressWarnings("unused")
- NullPointerException npe) {
- throw new AlreadyClosedException("Already closed: " + this);
+ } catch (NullPointerException e) {
+ throw alreadyClosed(e);
}
}
@@ -173,10 +184,8 @@ public abstract class ByteBufferIndexInput extends IndexInput implements RandomA
@SuppressWarnings("unused")
BufferUnderflowException e) {
super.readLongs(dst, offset, length);
- } catch (
- @SuppressWarnings("unused")
- NullPointerException npe) {
- throw new AlreadyClosedException("Already closed: " + this);
+ } catch (NullPointerException e) {
+ throw alreadyClosed(e);
}
}
@@ -204,10 +213,8 @@ public abstract class ByteBufferIndexInput extends IndexInput implements RandomA
@SuppressWarnings("unused")
BufferUnderflowException e) {
super.readInts(dst, offset, length);
- } catch (
- @SuppressWarnings("unused")
- NullPointerException npe) {
- throw new AlreadyClosedException("Already closed: " + this);
+ } catch (NullPointerException e) {
+ throw alreadyClosed(e);
}
}
@@ -238,10 +245,8 @@ public abstract class ByteBufferIndexInput extends IndexInput implements RandomA
@SuppressWarnings("unused")
BufferUnderflowException e) {
super.readFloats(floats, offset, len);
- } catch (
- @SuppressWarnings("unused")
- NullPointerException npe) {
- throw new AlreadyClosedException("Already closed: " + this);
+ } catch (NullPointerException e) {
+ throw alreadyClosed(e);
}
}
@@ -253,10 +258,8 @@ public abstract class ByteBufferIndexInput extends IndexInput implements RandomA
@SuppressWarnings("unused")
BufferUnderflowException e) {
return super.readShort();
- } catch (
- @SuppressWarnings("unused")
- NullPointerException npe) {
- throw new AlreadyClosedException("Already closed: " + this);
+ } catch (NullPointerException e) {
+ throw alreadyClosed(e);
}
}
@@ -268,10 +271,8 @@ public abstract class ByteBufferIndexInput extends IndexInput implements RandomA
@SuppressWarnings("unused")
BufferUnderflowException e) {
return super.readInt();
- } catch (
- @SuppressWarnings("unused")
- NullPointerException npe) {
- throw new AlreadyClosedException("Already closed: " + this);
+ } catch (NullPointerException e) {
+ throw alreadyClosed(e);
}
}
@@ -283,10 +284,8 @@ public abstract class ByteBufferIndexInput extends IndexInput implements RandomA
@SuppressWarnings("unused")
BufferUnderflowException e) {
return super.readLong();
- } catch (
- @SuppressWarnings("unused")
- NullPointerException npe) {
- throw new AlreadyClosedException("Already closed: " + this);
+ } catch (NullPointerException e) {
+ throw alreadyClosed(e);
}
}
@@ -294,10 +293,8 @@ public abstract class ByteBufferIndexInput extends IndexInput implements RandomA
public long getFilePointer() {
try {
return (((long) curBufIndex) << chunkSizePower) + curBuf.position();
- } catch (
- @SuppressWarnings("unused")
- NullPointerException npe) {
- throw new AlreadyClosedException("Already closed: " + this);
+ } catch (NullPointerException e) {
+ throw alreadyClosed(e);
}
}
@@ -316,14 +313,10 @@ public abstract class ByteBufferIndexInput extends IndexInput implements RandomA
this.curBufIndex = bi;
setCurBuf(b);
}
- } catch (@SuppressWarnings("unused")
- ArrayIndexOutOfBoundsException
- | IllegalArgumentException e) {
- throw new EOFException("seek past EOF: " + this);
- } catch (
- @SuppressWarnings("unused")
- NullPointerException npe) {
- throw new AlreadyClosedException("Already closed: " + this);
+ } catch (ArrayIndexOutOfBoundsException | IllegalArgumentException e) {
+ throw handlePositionalIOOBE(e, "seek", pos);
+ } catch (NullPointerException e) {
+ throw alreadyClosed(e);
}
}
@@ -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 (
- @SuppressWarnings("unused")
- IndexOutOfBoundsException ioobe) {
- throw new EOFException("seek past EOF: " + this);
- } catch (
- @SuppressWarnings("unused")
- NullPointerException npe) {
- throw new AlreadyClosedException("Already closed: " + this);
+ } catch (IndexOutOfBoundsException ioobe) {
+ throw handlePositionalIOOBE(ioobe, "read", pos);
+ } catch (NullPointerException e) {
+ throw alreadyClosed(e);
}
}
@@ -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")
- ArrayIndexOutOfBoundsException
- | IllegalArgumentException aioobe) {
- throw new EOFException("seek past EOF: " + this);
- } catch (
- @SuppressWarnings("unused")
- NullPointerException npe) {
- throw new AlreadyClosedException("Already closed: " + this);
+ } catch (ArrayIndexOutOfBoundsException | IllegalArgumentException aioobe) {
+ throw handlePositionalIOOBE(aioobe, "read", pos);
+ } catch (NullPointerException e) {
+ throw alreadyClosed(e);
}
}
@@ -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 (
- @SuppressWarnings("unused")
- NullPointerException npe) {
- throw new AlreadyClosedException("Already closed: " + this);
+ } catch (NullPointerException e) {
+ throw alreadyClosed(e);
}
}
@@ -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 (
- @SuppressWarnings("unused")
- NullPointerException npe) {
- throw new AlreadyClosedException("Already closed: " + this);
+ } catch (NullPointerException e) {
+ throw alreadyClosed(e);
}
}
@@ -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 (
- @SuppressWarnings("unused")
- NullPointerException npe) {
- throw new AlreadyClosedException("Already closed: " + this);
+ } catch (NullPointerException e) {
+ throw alreadyClosed(e);
}
}
@@ -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 new IllegalArgumentException("Seeking to negative position: " + this, e);
- } else {
- throw new EOFException("seek past EOF: " + this);
- }
- } catch (
- @SuppressWarnings("unused")
- NullPointerException npe) {
- throw new AlreadyClosedException("Already closed: " + this);
+ throw handlePositionalIOOBE(e, "seek", pos);
+ } catch (NullPointerException e) {
+ throw alreadyClosed(e);
}
}
@@ -580,10 +553,8 @@ public abstract class ByteBufferIndexInput extends IndexInput implements RandomA
public long getFilePointer() {
try {
return curBuf.position();
- } catch (
- @SuppressWarnings("unused")
- NullPointerException npe) {
- throw new AlreadyClosedException("Already closed: " + this);
+ } catch (NullPointerException e) {
+ throw alreadyClosed(e);
}
}
@@ -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 new IllegalArgumentException("Seeking to negative position: " + this, e);
- } else {
- throw new EOFException("seek past EOF: " + this);
- }
- } catch (
- @SuppressWarnings("unused")
- NullPointerException npe) {
- throw new AlreadyClosedException("Already closed: " + this);
+ throw handlePositionalIOOBE(e, "read", pos);
+ } catch (NullPointerException e) {
+ throw alreadyClosed(e);
}
}
@@ -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 new IllegalArgumentException("Seeking to negative position: " + this, e);
- } else {
- throw new EOFException("seek past EOF: " + this);
- }
- } catch (
- @SuppressWarnings("unused")
- NullPointerException npe) {
- throw new AlreadyClosedException("Already closed: " + this);
+ throw handlePositionalIOOBE(e, "read", pos);
+ } catch (NullPointerException e) {
+ throw alreadyClosed(e);
}
}
@@ -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 new IllegalArgumentException("Seeking to negative position: " + this, e);
- } else {
- throw new EOFException("seek past EOF: " + this);
- }
- } catch (
- @SuppressWarnings("unused")
- NullPointerException npe) {
- throw new AlreadyClosedException("Already closed: " + this);
+ throw handlePositionalIOOBE(e, "read", pos);
+ } catch (NullPointerException e) {
+ throw alreadyClosed(e);
}
}
@@ -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 new IllegalArgumentException("Seeking to negative position: " + this, e);
- } else {
- throw new EOFException("seek past EOF: " + this);
- }
- } catch (
- @SuppressWarnings("unused")
- NullPointerException npe) {
- throw new AlreadyClosedException("Already closed: " + this);
+ throw handlePositionalIOOBE(e, "read", pos);
+ } catch (NullPointerException e) {
+ throw alreadyClosed(e);
}
}
}
@@ -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);
}
diff --git a/lucene/core/src/java19/org/apache/lucene/store/MemorySegmentIndexInput.java b/lucene/core/src/java19/org/apache/lucene/store/MemorySegmentIndexInput.java
index 7754645c708..cef86269ae8 100644
--- a/lucene/core/src/java19/org/apache/lucene/store/MemorySegmentIndexInput.java
+++ b/lucene/core/src/java19/org/apache/lucene/store/MemorySegmentIndexInput.java
@@ -91,11 +91,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);
}
}
@@ -272,10 +274,8 @@ abstract class MemorySegmentIndexInput extends IndexInput implements RandomAcces
this.curSegment = seg;
}
this.curPosition = Objects.checkIndex(pos & chunkSizeMask, curSegment.byteSize() + 1);
- } catch (
- @SuppressWarnings("unused")
- IndexOutOfBoundsException e) {
- throw handlePositionalIOOBE("seek", pos);
+ } catch (IndexOutOfBoundsException e) {
+ throw handlePositionalIOOBE(e, "seek", pos);
}
}
@@ -284,10 +284,8 @@ abstract class MemorySegmentIndexInput extends IndexInput implements RandomAcces
try {
final int si = (int) (pos >> chunkSizePower);
return segments[si].get(LAYOUT_BYTE, pos & chunkSizeMask);
- } catch (
- @SuppressWarnings("unused")
- IndexOutOfBoundsException ioobe) {
- throw handlePositionalIOOBE("read", pos);
+ } catch (IndexOutOfBoundsException ioobe) {
+ throw handlePositionalIOOBE(ioobe, "read", pos);
} catch (NullPointerException | IllegalStateException e) {
throw alreadyClosed(e);
}
@@ -301,10 +299,8 @@ abstract class MemorySegmentIndexInput extends IndexInput implements RandomAcces
this.curPosition = pos & chunkSizeMask;
this.curSegmentIndex = si;
this.curSegment = seg;
- } catch (
- @SuppressWarnings("unused")
- IndexOutOfBoundsException ioobe) {
- throw handlePositionalIOOBE("read", pos);
+ } catch (IndexOutOfBoundsException ioobe) {
+ throw handlePositionalIOOBE(ioobe, "read", pos);
} catch (NullPointerException | IllegalStateException e) {
throw alreadyClosed(e);
}
@@ -471,10 +467,8 @@ abstract class MemorySegmentIndexInput extends IndexInput implements RandomAcces
ensureOpen();
try {
curPosition = Objects.checkIndex(pos, length + 1);
- } catch (
- @SuppressWarnings("unused")
- IndexOutOfBoundsException e) {
- throw handlePositionalIOOBE("seek", pos);
+ } catch (IndexOutOfBoundsException e) {
+ throw handlePositionalIOOBE(e, "seek", pos);
}
}
@@ -488,10 +482,8 @@ abstract class MemorySegmentIndexInput extends IndexInput implements RandomAcces
public byte readByte(long pos) throws IOException {
try {
return curSegment.get(LAYOUT_BYTE, pos);
- } catch (
- @SuppressWarnings("unused")
- IndexOutOfBoundsException e) {
- throw handlePositionalIOOBE("read", pos);
+ } catch (IndexOutOfBoundsException e) {
+ throw handlePositionalIOOBE(e, "read", pos);
} catch (NullPointerException | IllegalStateException e) {
throw alreadyClosed(e);
}
@@ -501,10 +493,8 @@ abstract class MemorySegmentIndexInput extends IndexInput implements RandomAcces
public short readShort(long pos) throws IOException {
try {
return curSegment.get(LAYOUT_LE_SHORT, pos);
- } catch (
- @SuppressWarnings("unused")
- IndexOutOfBoundsException e) {
- throw handlePositionalIOOBE("read", pos);
+ } catch (IndexOutOfBoundsException e) {
+ throw handlePositionalIOOBE(e, "read", pos);
} catch (NullPointerException | IllegalStateException e) {
throw alreadyClosed(e);
}
@@ -514,10 +504,8 @@ abstract class MemorySegmentIndexInput extends IndexInput implements RandomAcces
public int readInt(long pos) throws IOException {
try {
return curSegment.get(LAYOUT_LE_INT, pos);
- } catch (
- @SuppressWarnings("unused")
- IndexOutOfBoundsException e) {
- throw handlePositionalIOOBE("read", pos);
+ } catch (IndexOutOfBoundsException e) {
+ throw handlePositionalIOOBE(e, "read", pos);
} catch (NullPointerException | IllegalStateException e) {
throw alreadyClosed(e);
}
@@ -527,10 +515,8 @@ abstract class MemorySegmentIndexInput extends IndexInput implements RandomAcces
public long readLong(long pos) throws IOException {
try {
return curSegment.get(LAYOUT_LE_LONG, pos);
- } catch (
- @SuppressWarnings("unused")
- IndexOutOfBoundsException e) {
- throw handlePositionalIOOBE("read", pos);
+ } catch (IndexOutOfBoundsException e) {
+ throw handlePositionalIOOBE(e, "read", pos);
} catch (NullPointerException | IllegalStateException e) {
throw alreadyClosed(e);
}
@@ -558,9 +544,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);
}
diff --git a/lucene/core/src/test/org/apache/lucene/store/TestMultiMMap.java b/lucene/core/src/test/org/apache/lucene/store/TestMultiMMap.java
index 7ef6c1d82e4..9cdd6e2b54e 100644
--- a/lucene/core/src/test/org/apache/lucene/store/TestMultiMMap.java
+++ b/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?