From 7743afea607a680bbc4887dc6653c081010ed9fc Mon Sep 17 00:00:00 2001 From: Chris Vest Date: Mon, 28 Mar 2016 13:03:44 +0200 Subject: [PATCH] Exception-free, branch-free PageCursor bounds-checks. Performing an out-of-bounds memory access to a page via a page cursor now no longer throws an exception. The reason is that we might have an inconsistent read that fools decoding logic computing wrong offsets for data, or to try and read more data than can fit on a page. Previously, such out-of-bounds accesses would cause the PageCursor to throw an exception, but this is inconsiderate - it is not the fault of the decoding logic that it was fed wrong data and tricked into an out-of-bounds access. So instead we now raise a boolean flag the cursor has experienced and out-of-bounds access, and this flag can then be checked after the decoder has checked that its read was consistent. Then it is up to the decoder to make a decision about how to handle the fact that it did an out-of-bounds access. At present, the record formats (where the decoding logic is) don't do anything, but this is just because this change is not yet complete. I will make them do something in the next commit. --- .../org/neo4j/io/pagecache/PageCursor.java | 8 + .../io/pagecache/impl/muninn/CursorPool.java | 14 +- .../impl/muninn/MuninnPageCache.java | 92 +++-- .../impl/muninn/MuninnPageCursor.java | 203 +++++------ .../impl/muninn/MuninnPagedFile.java | 94 +++-- .../impl/muninn/MuninnReadPageCursor.java | 7 +- .../impl/muninn/MuninnWritePageCursor.java | 4 +- .../impl/muninn/VictimPageReference.java | 46 +++ .../pagecache/AdversarialReadPageCursor.java | 6 + .../pagecache/AdversarialWritePageCursor.java | 6 + .../org/neo4j/io/pagecache/PageCacheTest.java | 337 ++++++++++++------ .../neo4j/io/pagecache/StubPageCursor.java | 6 + .../RecordBoundaryCheckingPagedFile.java | 6 + 13 files changed, 499 insertions(+), 330 deletions(-) create mode 100644 community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/VictimPageReference.java diff --git a/community/io/src/main/java/org/neo4j/io/pagecache/PageCursor.java b/community/io/src/main/java/org/neo4j/io/pagecache/PageCursor.java index c04e8b4e7ffd..f79ec51b0c36 100644 --- a/community/io/src/main/java/org/neo4j/io/pagecache/PageCursor.java +++ b/community/io/src/main/java/org/neo4j/io/pagecache/PageCursor.java @@ -348,4 +348,12 @@ public interface PageCursor extends AutoCloseable * @return The number of bytes actually copied. */ int copyTo( int sourceOffset, PageCursor targetCursor, int targetOffset, int lengthInBytes ); + + /** + * Discern whether an out-of-bounds access has occurred since the last call to {@link #next()} or + * {@link #next(long)}, or since the last call to {@link #shouldRetry()} that returned {@code true}, or since the + * last call to this method. + * @return {@code true} if an access was out of bounds. + */ + boolean checkAndClearBoundsFlag(); } diff --git a/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/CursorPool.java b/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/CursorPool.java index aa9c160c5add..eb98bc451027 100644 --- a/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/CursorPool.java +++ b/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/CursorPool.java @@ -22,12 +22,12 @@ final class CursorPool extends ThreadLocal { private final MuninnPagedFile pagedFile; - private final int cachePageSize; + private final long victimPage; - public CursorPool( MuninnPagedFile pagedFile ) + CursorPool( MuninnPagedFile pagedFile ) { this.pagedFile = pagedFile; - this.cachePageSize = pagedFile.pageCache.pageSize(); + this.victimPage = pagedFile.pageCache.victimPage; } @Override @@ -36,7 +36,7 @@ protected CursorSets initialValue() return new CursorSets(); } - public MuninnReadPageCursor takeReadCursor( long pageId, int pf_flags ) + MuninnReadPageCursor takeReadCursor( long pageId, int pf_flags ) { CursorSets cursorSets = get(); MuninnReadPageCursor cursor = cursorSets.readCursors; @@ -54,12 +54,12 @@ public MuninnReadPageCursor takeReadCursor( long pageId, int pf_flags ) private MuninnReadPageCursor createReadCursor( CursorSets cursorSets ) { - MuninnReadPageCursor cursor = new MuninnReadPageCursor( cursorSets, cachePageSize ); + MuninnReadPageCursor cursor = new MuninnReadPageCursor( cursorSets, victimPage ); cursor.initialiseFile( pagedFile ); return cursor; } - public MuninnWritePageCursor takeWriteCursor( long pageId, int pf_flags ) + MuninnWritePageCursor takeWriteCursor( long pageId, int pf_flags ) { CursorSets cursorSets = get(); MuninnWritePageCursor cursor = cursorSets.writeCursors; @@ -77,7 +77,7 @@ public MuninnWritePageCursor takeWriteCursor( long pageId, int pf_flags ) private MuninnWritePageCursor createWriteCursor( CursorSets cursorSets ) { - MuninnWritePageCursor cursor = new MuninnWritePageCursor( cursorSets, cachePageSize ); + MuninnWritePageCursor cursor = new MuninnWritePageCursor( cursorSets, victimPage ); cursor.initialiseFile( pagedFile ); return cursor; } diff --git a/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnPageCache.java b/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnPageCache.java index 42171f1b38c6..03f5d613a933 100644 --- a/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnPageCache.java +++ b/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnPageCache.java @@ -148,7 +148,9 @@ public class MuninnPageCache implements PageCache private final int keepFree; private final PageCacheTracer tracer; private final MuninnPage[] pages; - private final AtomicInteger backgroundFlushPauseRequests; + // All PageCursors are initialised with their pointers pointing to the victim page. This way, we can do branch-free + // bounds checking of page accesses without fear of segfaulting newly allocated cursors. + final long victimPage; // The freelist is a thread-safe linked-list of 2 types of objects. A link // can either be a MuninnPage or a FreePage. @@ -178,8 +180,6 @@ public class MuninnPageCache implements PageCache // threads scheduling meta-data in the OS kernel. private volatile boolean evictorParked; private volatile IOException evictorException; - // The thread that does background flushing. - private volatile Thread flushThread; // Flag for when page cache is closed - writes guarded by synchronized(this), reads can be unsynchronized private volatile boolean closed; @@ -187,9 +187,6 @@ public class MuninnPageCache implements PageCache // Only used by ensureThreadsInitialised while holding the monitor lock on this MuninnPageCache instance. private boolean threadsInitialised; - // The accumulator for the flush task sleep debt. This is only accessed from the flush task. - private long sleepDebtNanos; - // 'true' (the default) if we should print any exceptions we get when unmapping a file. private boolean printExceptionsOnClose; @@ -209,12 +206,12 @@ public MuninnPageCache( this.keepFree = Math.min( pagesToKeepFree, maxPages / 2 ); this.tracer = tracer; this.pages = new MuninnPage[maxPages]; - this.backgroundFlushPauseRequests = new AtomicInteger(); this.printExceptionsOnClose = true; long alignment = swapperFactory.getRequiredBufferAlignment(); long expectedMaxMemory = ((long) maxPages) * cachePageSize; // cast to long prevents overflow MemoryManager memoryManager = new MemoryManager( expectedMaxMemory, alignment ); + this.victimPage = VictimPageReference.getVictimPage( cachePageSize ); Object pageList = null; int pageIndex = maxPages; while ( pageIndex --> 0 ) @@ -279,9 +276,15 @@ public synchronized PagedFile map( File file, int filePageSize, OpenOption... op ensureThreadsInitialised(); if ( filePageSize > cachePageSize ) { - throw new IllegalArgumentException( "Cannot map files with a filePageSize (" + - filePageSize + ") that is greater than the cachePageSize (" + - cachePageSize + ")" ); + throw new IllegalArgumentException( + "Cannot map files with a filePageSize (" + filePageSize + ") that is greater than the " + + "cachePageSize (" + cachePageSize + ")" ); + } + if ( filePageSize < Long.BYTES ) + { + throw new IllegalArgumentException( + "Cannot map files with a filePageSize (" + filePageSize + ") that is less than " + + Long.BYTES + " bytes" ); } boolean createIfNotExists = false; boolean truncateExisting = false; @@ -544,8 +547,6 @@ public synchronized void close() throws IOException interrupt( evictionThread ); evictionThread = null; - interrupt( flushThread ); - flushThread = null; } private void interrupt( Thread thread ) @@ -788,7 +789,7 @@ private int parkUntilEvictionRequired( int keepFree ) for (;;) { parkEvictor( parkNanos ); - if ( Thread.currentThread().isInterrupted() || closed ) + if ( Thread.interrupted() || closed ) { return 0; } @@ -841,34 +842,36 @@ int evictPages( int pageCountToEvict, int clockArm, EvictionRunEvent evictionRun // This reduces the CPU load and power usage in such a // scenario. pageCountToEvict--; - boolean pageEvicted; + boolean pageEvicted = false; try ( EvictionEvent evictionEvent = evictionRunEvent.beginEviction() ) { pageEvicted = page.isLoaded() && evictPage( page, evictionEvent ); - } - - if ( pageEvicted ) - { - Object current; - Object nextListHead; - FreePage freePage = null; - do + if ( pageEvicted ) { - current = getFreelistHead(); - freePage = freePage == null? - new FreePage( page ) : freePage; - freePage.setNext( (FreePage) current ); - nextListHead = freePage; + Object current; + Object nextListHead; + FreePage freePage = null; + do + { + current = getFreelistHead(); + freePage = freePage == null? + new FreePage( page ) : freePage; + freePage.setNext( (FreePage) current ); + nextListHead = freePage; + } + while ( !compareAndSetFreelistHead( current, nextListHead ) ); } - while ( !compareAndSetFreelistHead( current, nextListHead ) ); } - else + finally { - // Pages we put into the free-list remain exclusively locked until a page fault unlocks them - // If we somehow failed to evict the page, then we need to make sure that we release the - // exclusive lock. - page.unlockExclusive(); + if ( !pageEvicted ) + { + // Pages we put into the free-list remain exclusively locked until a page fault unlocks + // them. If we somehow failed to evict the page, then we need to make sure that we release + // the exclusive lock. + page.unlockExclusive(); + } } } } @@ -879,6 +882,10 @@ int evictPages( int pageCountToEvict, int clockArm, EvictionRunEvent evictionRun return clockArm; } + /** + * Evict the given page, or return {@code false} if the eviction failed for any reason. + * This method will never throw an exception! + */ private boolean evictPage( MuninnPage page, EvictionEvent evictionEvent ) { //noinspection TryWithIdenticalCatches - this warning is a false positive; bug in Intellij inspection @@ -915,25 +922,6 @@ private void clearEvictorException() } } - void pauseBackgroundFlushTask() - { - backgroundFlushPauseRequests.getAndIncrement(); - } - - void unpauseBackgroundFlushTask() - { - backgroundFlushPauseRequests.getAndDecrement(); - LockSupport.unpark( flushThread ); - } - - private void checkBackgroundFlushPause() - { - while ( backgroundFlushPauseRequests.get() > 0 ) - { - LockSupport.parkNanos( TimeUnit.MILLISECONDS.toNanos( 10 ) ); - } - } - @Override public String toString() { diff --git a/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnPageCursor.java b/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnPageCursor.java index 72671bcb2d16..b78badbc1364 100644 --- a/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnPageCursor.java +++ b/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnPageCursor.java @@ -43,7 +43,7 @@ abstract class MuninnPageCursor implements PageCursor private static final int SIZE_OF_INT = Integer.BYTES; private static final int SIZE_OF_LONG = Long.BYTES; - private final int cachePageSize; + private final long victimPage; protected MuninnPagedFile pagedFile; protected PageSwapper swapper; protected PageCacheTracer tracer; @@ -54,25 +54,29 @@ abstract class MuninnPageCursor implements PageCursor protected long currentPageId; protected long nextPageId; private long pointer; - private int size; + private int pageSize; + private int filePageSize; private int offset; + private boolean outOfBounds; - public MuninnPageCursor( int cachePageSize ) + MuninnPageCursor( long victimPage ) { - this.cachePageSize = cachePageSize; + this.victimPage = victimPage; + pointer = victimPage; } - public final void initialiseFile( MuninnPagedFile pagedFile ) + final void initialiseFile( MuninnPagedFile pagedFile ) { this.swapper = pagedFile.swapper; this.tracer = pagedFile.tracer; } - public final void initialiseFlags( MuninnPagedFile pagedFile, long pageId, int pf_flags ) + final void initialiseFlags( MuninnPagedFile pagedFile, long pageId, int pf_flags ) { this.pagedFile = pagedFile; this.pageId = pageId; this.pf_flags = pf_flags; + this.filePageSize = pagedFile.filePageSize; } @Override @@ -87,7 +91,8 @@ public final void reset( MuninnPage page ) this.page = page; this.offset = 0; this.pointer = page.address(); - this.size = cachePageSize; + this.pageSize = filePageSize; + checkAndClearBoundsFlag(); if ( tracePinnedCachePageId ) { pinEvent.setCachePageId( page.getCachePageId() ); @@ -114,9 +119,10 @@ public final void close() /** * Must be called by {@link #unpinCurrentPage()}. */ - protected void clearPageState() + void clearPageState() { - size = 0; // make all future bound checks fail + pointer = victimPage; // make all future page access go to the victim page + pageSize = 0; // make all future bound checks fail page = null; // make all future page navigation fail } @@ -306,7 +312,7 @@ private void abortPageFault( Throwable throwable, Object[] chunk, long chunkOffs pinEvent.done(); } - protected long assertPagedFileStillMappedAndGetIdOfLastPage() + long assertPagedFileStillMappedAndGetIdOfLastPage() { return pagedFile.getLastPageId(); } @@ -325,33 +331,28 @@ protected long assertPagedFileStillMappedAndGetIdOfLastPage() // --- IO methods: - private void checkBounds( int position ) + /** + * Compute a pointer that guarantees (assuming {@code size} is less than or equal to {@link #pageSize}) that the + * page access will be within the bounds of the page. + * This might mean that the pointer won't point to where one might naively expect, but will instead be + * truncated to point within the page. In this case, an overflow has happened anf the {@link #outOfBounds} + * flag will be raised. + */ + private long getBoundedPointer( int offset, int size ) { - if ( position > size ) - { - throw new IndexOutOfBoundsException( getOutOfBoundsMessage( position ) ); - } - } - - private String getOutOfBoundsMessage( int position ) - { - if ( size > 0 ) - { - return "Position " + position + " is greater than the upper " + - "page size bound of " + size; - } - else - { - return "The PageCursor is not bound to a page. " + - "Maybe next() returned false or was not called, or the cursor has been closed."; - } + long can = pointer + offset; + long lim = pointer + pageSize - size; + long ref = Math.min( can, lim ); + ref = Math.max( ref, pointer ); + outOfBounds |= ref != can | lim < pointer; + return ref; } @Override public final byte getByte() { - checkBounds( offset + SIZE_OF_BYTE ); - byte b = UnsafeUtil.getByte( pointer + offset ); + long p = getBoundedPointer( offset, SIZE_OF_BYTE ); + byte b = UnsafeUtil.getByte( p ); offset++; return b; } @@ -359,23 +360,23 @@ public final byte getByte() @Override public byte getByte( int offset ) { - checkBounds( offset + SIZE_OF_BYTE ); - return UnsafeUtil.getByte( pointer + offset ); + long p = getBoundedPointer( offset, SIZE_OF_BYTE ); + return UnsafeUtil.getByte( p ); } @Override public void putByte( byte value ) { - checkBounds( offset + SIZE_OF_BYTE ); - UnsafeUtil.putByte( pointer + offset, value ); + long p = getBoundedPointer( offset, SIZE_OF_BYTE ); + UnsafeUtil.putByte( p, value ); offset++; } @Override public void putByte( int offset, byte value ) { - checkBounds( offset + SIZE_OF_BYTE ); - UnsafeUtil.putByte( pointer + offset, value ); + long p = getBoundedPointer( offset, SIZE_OF_BYTE ); + UnsafeUtil.putByte( p, value ); } @Override @@ -389,11 +390,11 @@ public long getLong() @Override public long getLong( int offset ) { - checkBounds( offset + SIZE_OF_LONG ); + long p = getBoundedPointer( offset, SIZE_OF_LONG ); long value; if ( UnsafeUtil.allowUnalignedMemoryAccess ) { - value = UnsafeUtil.getLong( pointer + offset ); + value = UnsafeUtil.getLong( p ); if ( !UnsafeUtil.storeByteOrderIsNative ) { value = Long.reverseBytes( value ); @@ -401,14 +402,13 @@ public long getLong( int offset ) } else { - value = getLongBigEndian( offset ); + value = getLongBigEndian( p ); } return value; } - private long getLongBigEndian( int offset ) + private long getLongBigEndian( long p ) { - long p = pointer + offset; long a = UnsafeUtil.getByte( p ) & 0xFF; long b = UnsafeUtil.getByte( p + 1 ) & 0xFF; long c = UnsafeUtil.getByte( p + 2 ) & 0xFF; @@ -430,21 +430,19 @@ public void putLong( long value ) @Override public void putLong( int offset, long value ) { - checkBounds( offset + SIZE_OF_LONG ); + long p = getBoundedPointer( offset, SIZE_OF_LONG ); if ( UnsafeUtil.allowUnalignedMemoryAccess ) { - long p = pointer + offset; UnsafeUtil.putLong( p, UnsafeUtil.storeByteOrderIsNative ? value : Long.reverseBytes( value ) ); } else { - putLongBigEndian( value, offset ); + putLongBigEndian( value, p ); } } - private void putLongBigEndian( long value, int offset ) + private void putLongBigEndian( long value, long p ) { - long p = pointer + offset; UnsafeUtil.putByte( p , (byte)( value >> 56 ) ); UnsafeUtil.putByte( p + 1, (byte)( value >> 48 ) ); UnsafeUtil.putByte( p + 2, (byte)( value >> 40 ) ); @@ -466,18 +464,17 @@ public int getInt() @Override public int getInt( int offset ) { - checkBounds( offset + SIZE_OF_INT ); + long p = getBoundedPointer( offset, SIZE_OF_INT ); if ( UnsafeUtil.allowUnalignedMemoryAccess ) { - int x = UnsafeUtil.getInt( pointer + offset ); + int x = UnsafeUtil.getInt( p ); return UnsafeUtil.storeByteOrderIsNative ? x : Integer.reverseBytes( x ); } - return getIntBigEndian( offset ); + return getIntBigEndian( p ); } - private int getIntBigEndian( int offset ) + private int getIntBigEndian( long p ) { - long p = pointer + offset; int a = UnsafeUtil.getByte( p ) & 0xFF; int b = UnsafeUtil.getByte( p + 1 ) & 0xFF; int c = UnsafeUtil.getByte( p + 2 ) & 0xFF; @@ -495,21 +492,19 @@ public void putInt( int value ) @Override public void putInt( int offset, int value ) { - checkBounds( offset + SIZE_OF_INT ); + long p = getBoundedPointer( offset, SIZE_OF_INT ); if ( UnsafeUtil.allowUnalignedMemoryAccess ) { - long p = pointer + offset; UnsafeUtil.putInt( p, UnsafeUtil.storeByteOrderIsNative ? value : Integer.reverseBytes( value ) ); } else { - putIntBigEndian( value, offset ); + putIntBigEndian( value, p ); } } - private void putIntBigEndian( int value, int offset ) + private void putIntBigEndian( int value, long p ) { - long p = pointer + offset; UnsafeUtil.putByte( p , (byte)( value >> 24 ) ); UnsafeUtil.putByte( p + 1, (byte)( value >> 16 ) ); UnsafeUtil.putByte( p + 2, (byte)( value >> 8 ) ); @@ -537,11 +532,13 @@ public void getBytes( byte[] data ) @Override public void getBytes( byte[] data, int arrayOffset, int length ) { - checkBounds( offset + length ); - long address = pointer + offset; - for ( int i = 0; i < length; i++ ) + long p = getBoundedPointer( offset, length ); + if ( !outOfBounds ) { - data[arrayOffset + i] = UnsafeUtil.getByte( address + i ); + for ( int i = 0; i < length; i++ ) + { + data[arrayOffset + i] = UnsafeUtil.getByte( p + i ); + } } offset += length; } @@ -555,12 +552,14 @@ public final void putBytes( byte[] data ) @Override public void putBytes( byte[] data, int arrayOffset, int length ) { - checkBounds( offset + length ); - long address = pointer + offset; - for ( int i = 0; i < length; i++ ) + long p = getBoundedPointer( offset, length ); + if ( !outOfBounds ) { - byte b = data[arrayOffset + i]; - UnsafeUtil.putByte( address + i, b ); + for ( int i = 0; i < length; i++ ) + { + byte b = data[arrayOffset + i]; + UnsafeUtil.putByte( p + i, b ); + } } offset += length; } @@ -576,18 +575,17 @@ public final short getShort() @Override public short getShort( int offset ) { - checkBounds( offset + SIZE_OF_SHORT ); + long p = getBoundedPointer( offset, SIZE_OF_SHORT ); if ( UnsafeUtil.allowUnalignedMemoryAccess ) { - short x = UnsafeUtil.getShort( pointer + offset ); + short x = UnsafeUtil.getShort( p ); return UnsafeUtil.storeByteOrderIsNative ? x : Short.reverseBytes( x ); } - return getShortBigEndian( offset ); + return getShortBigEndian( p ); } - private short getShortBigEndian( int offset ) + private short getShortBigEndian( long p ) { - long p = pointer + offset; short a = (short) (UnsafeUtil.getByte( p ) & 0xFF); short b = (short) (UnsafeUtil.getByte( p + 1 ) & 0xFF); return (short) ((a << 8) | b); @@ -603,21 +601,19 @@ public void putShort( short value ) @Override public void putShort( int offset, short value ) { - checkBounds( offset + SIZE_OF_SHORT ); + long p = getBoundedPointer( offset, SIZE_OF_SHORT ); if ( UnsafeUtil.allowUnalignedMemoryAccess ) { - long p = pointer + offset; UnsafeUtil.putShort( p, UnsafeUtil.storeByteOrderIsNative ? value : Short.reverseBytes( value ) ); } else { - putShortBigEndian( value, offset ); + putShortBigEndian( value, p ); } } - private void putShortBigEndian( short value, int offset ) + private void putShortBigEndian( short value, long p ) { - long p = pointer + offset; UnsafeUtil.putByte( p , (byte)( value >> 8 ) ); UnsafeUtil.putByte( p + 1, (byte)( value ) ); } @@ -627,10 +623,15 @@ public int copyTo( int sourceOffset, PageCursor targetCursor, int targetOffset, { int sourcePageSize = getCurrentPageSize(); int targetPageSize = targetCursor.getCurrentPageSize(); - if ( targetCursor.getClass() == MuninnWritePageCursor.class - & sourceOffset >= 0 & targetOffset >= 0 - & sourceOffset < sourcePageSize & targetOffset < targetPageSize - & lengthInBytes > 0 ) + if ( targetCursor.getClass() != MuninnWritePageCursor.class ) + { + throw new IllegalArgumentException( "Target cursor must be writable" ); + } + if ( sourceOffset >= 0 + & targetOffset >= 0 + & sourceOffset < sourcePageSize + & targetOffset < targetPageSize + & lengthInBytes > 0 ) { MuninnPageCursor cursor = (MuninnPageCursor) targetCursor; int remainingSource = sourcePageSize - sourceOffset; @@ -639,42 +640,14 @@ public int copyTo( int sourceOffset, PageCursor targetCursor, int targetOffset, UnsafeUtil.copyMemory( pointer + sourceOffset, cursor.pointer + targetOffset, bytes ); return bytes; } - return illegalCopyToArgs( sourceOffset, sourcePageSize, targetOffset, targetPageSize, lengthInBytes ); - } - - private int illegalCopyToArgs( - int sourceOffset, int sourcePageSize, int targetOffset, int targetPageSize, int lengthInBytes ) - { - if ( sourceOffset < 0 ) - { - throw new IndexOutOfBoundsException( "Negative source offset: " + sourceOffset ); - } - if ( targetOffset < 0 ) - { - throw new IndexOutOfBoundsException( "Negative target offset: " + targetOffset ); - } - if ( sourceOffset >= sourcePageSize ) - { - throw new IndexOutOfBoundsException( "Source offset beyond page bounds: " + sourceOffset ); - } - if ( targetOffset >= targetPageSize ) - { - throw new IndexOutOfBoundsException( "Target offset beyond page bounds: " + sourceOffset ); - } - if ( lengthInBytes < 0 ) - { - throw new IllegalArgumentException( "Cannot copy a negative number of bytes: " + lengthInBytes ); - } - throw new IllegalArgumentException( "Target cursor must be writable" ); + outOfBounds = true; + return 0; } @Override public void setOffset( int offset ) { - if ( offset < 0 ) - { - throw new IndexOutOfBoundsException(); - } + getBoundedPointer( offset, 0 ); this.offset = offset; } @@ -683,4 +656,12 @@ public final int getOffset() { return offset; } + + @Override + public boolean checkAndClearBoundsFlag() + { + boolean b = outOfBounds; + outOfBounds = false; + return b; + } } diff --git a/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnPagedFile.java b/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnPagedFile.java index abc3c553a7c4..b5706f3889fa 100644 --- a/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnPagedFile.java +++ b/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnPagedFile.java @@ -225,77 +225,69 @@ void flushAndForceForClose() throws IOException void flushAndForceInternal( FlushEventOpportunity flushOpportunity, boolean forClosing, IOLimiter limiter ) throws IOException { - pageCache.pauseBackgroundFlushTask(); // TODO it'd be awesome if, on Linux, we'd call sync_file_range(2) instead of fsync Flushable flushable = swapper::force; MuninnPage[] pages = new MuninnPage[translationTableChunkSize]; long filePageId = -1; // Start at -1 because we increment at the *start* of the chunk-loop iteration. long limiterStamp = IOLimiter.INITIAL_STAMP; - try + for ( Object[] chunk : translationTable ) { - for ( Object[] chunk : translationTable ) + // TODO Look into if we can tolerate flushing a few clean pages if it means we can use larger vectors. + // TODO The clean pages in question must still be loaded, though. Otherwise we'll end up writing + // TODO garbage to the file. + int pagesGrabbed = 0; + chunkLoop:for ( int i = 0; i < chunk.length; i++ ) { - // TODO Look into if we can tolerate flushing a few clean pages if it means we can use larger vectors. - // TODO The clean pages in question must still be loaded, though. Otherwise we'll end up writing - // TODO garbage to the file. - int pagesGrabbed = 0; - chunkLoop:for ( int i = 0; i < chunk.length; i++ ) - { - filePageId++; + filePageId++; - long offset = computeChunkOffset( filePageId ); - // We might race with eviction, but we also mustn't miss a dirty page, so we loop until we succeed - // in getting a lock on all available pages. - for (;;) + long offset = computeChunkOffset( filePageId ); + // We might race with eviction, but we also mustn't miss a dirty page, so we loop until we succeed + // in getting a lock on all available pages. + for (;;) + { + Object element = UnsafeUtil.getObjectVolatile( chunk, offset ); + if ( element instanceof MuninnPage ) { - Object element = UnsafeUtil.getObjectVolatile( chunk, offset ); - if ( element instanceof MuninnPage ) + MuninnPage page = (MuninnPage) element; + if ( !(forClosing? page.tryExclusiveLock() : page.tryFlushLock()) ) { - MuninnPage page = (MuninnPage) element; - if ( !(forClosing? page.tryExclusiveLock() : page.tryFlushLock()) ) - { - continue; - } - if ( page.isBoundTo( swapper, filePageId ) && page.isDirty() ) - { - // The page is still bound to the expected file and file page id after we locked it, - // so we didn't race with eviction and faulting, and the page is dirty. - // So we add it to our IO vector. - pages[pagesGrabbed] = page; - pagesGrabbed++; - continue chunkLoop; - } - else if ( forClosing ) - { - page.unlockExclusive(); - } - else - { - page.unlockFlush(); - } + continue; + } + if ( page.isBoundTo( swapper, filePageId ) && page.isDirty() ) + { + // The page is still bound to the expected file and file page id after we locked it, + // so we didn't race with eviction and faulting, and the page is dirty. + // So we add it to our IO vector. + pages[pagesGrabbed] = page; + pagesGrabbed++; + continue chunkLoop; + } + else if ( forClosing ) + { + page.unlockExclusive(); + } + else + { + page.unlockFlush(); } - break; - } - if ( pagesGrabbed > 0 ) - { - vectoredFlush( pages, pagesGrabbed, flushOpportunity, forClosing ); - limiterStamp = limiter.maybeLimitIO( limiterStamp, pagesGrabbed, flushable ); - pagesGrabbed = 0; } + break; } if ( pagesGrabbed > 0 ) { vectoredFlush( pages, pagesGrabbed, flushOpportunity, forClosing ); limiterStamp = limiter.maybeLimitIO( limiterStamp, pagesGrabbed, flushable ); + pagesGrabbed = 0; } } - - swapper.force(); - } - finally - { - pageCache.unpauseBackgroundFlushTask(); + if ( pagesGrabbed > 0 ) + { + vectoredFlush( pages, pagesGrabbed, flushOpportunity, forClosing ); + limiterStamp = limiter.maybeLimitIO( limiterStamp, pagesGrabbed, flushable ); + } } + + swapper.force(); } private void vectoredFlush( diff --git a/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnReadPageCursor.java b/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnReadPageCursor.java index b6978f0948e5..a88f01bac6d5 100644 --- a/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnReadPageCursor.java +++ b/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnReadPageCursor.java @@ -26,12 +26,12 @@ final class MuninnReadPageCursor extends MuninnPageCursor { private final CursorPool.CursorSets cursorSets; - protected long lockStamp; + private long lockStamp; MuninnReadPageCursor nextCursor; - public MuninnReadPageCursor( CursorPool.CursorSets cursorSets, int cachePageSize ) + MuninnReadPageCursor( CursorPool.CursorSets cursorSets, long victimPage ) { - super( cachePageSize ); + super( victimPage ); this.cursorSets = cursorSets; } @@ -108,6 +108,7 @@ public boolean shouldRetry() throws IOException private void startRetry() throws IOException { setOffset( 0 ); + checkAndClearBoundsFlag(); lockStamp = page.tryOptimisticReadLock(); // The page might have been evicted while we held the optimistic // read lock, so we need to check with page.pin that this is still diff --git a/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnWritePageCursor.java b/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnWritePageCursor.java index f4bceb5ac319..af6db1d03c26 100644 --- a/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnWritePageCursor.java +++ b/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnWritePageCursor.java @@ -29,9 +29,9 @@ final class MuninnWritePageCursor extends MuninnPageCursor private final CursorPool.CursorSets cursorSets; MuninnWritePageCursor nextCursor; - public MuninnWritePageCursor( CursorPool.CursorSets cursorSets, int cachePageSize ) + MuninnWritePageCursor( CursorPool.CursorSets cursorSets, long victimPage ) { - super( cachePageSize ); + super( victimPage ); this.cursorSets = cursorSets; } diff --git a/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/VictimPageReference.java b/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/VictimPageReference.java new file mode 100644 index 000000000000..068a827d760b --- /dev/null +++ b/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/VictimPageReference.java @@ -0,0 +1,46 @@ +/* + * Copyright (c) 2002-2016 "Neo Technology," + * Network Engine for Objects in Lund AB [http://neotechnology.com] + * + * This file is part of Neo4j. + * + * Neo4j is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ +package org.neo4j.io.pagecache.impl.muninn; + +import org.neo4j.unsafe.impl.internal.dragons.UnsafeUtil; + +class VictimPageReference +{ + private static int victimPageSize = -1; + private static long victimPagePointer; + + private VictimPageReference() + { + // All state is static + } + + static synchronized long getVictimPage( int pageSize ) + { + if ( victimPageSize < pageSize ) + { + // Note that we NEVER free any old victim pages. This is important because we cannot tell + // when we are done using them. Therefor, victim pages are allocated and stay allocated + // until our process terminates. + victimPagePointer = UnsafeUtil.allocateMemory( pageSize ); + victimPageSize = pageSize; + } + return victimPagePointer; + } +} diff --git a/community/io/src/test/java/org/neo4j/adversaries/pagecache/AdversarialReadPageCursor.java b/community/io/src/test/java/org/neo4j/adversaries/pagecache/AdversarialReadPageCursor.java index 2bd514201920..8aa309f1827b 100644 --- a/community/io/src/test/java/org/neo4j/adversaries/pagecache/AdversarialReadPageCursor.java +++ b/community/io/src/test/java/org/neo4j/adversaries/pagecache/AdversarialReadPageCursor.java @@ -280,4 +280,10 @@ public int copyTo( int sourceOffset, PageCursor targetCursor, int targetOffset, } return lengthInBytes; } + + @Override + public boolean checkAndClearBoundsFlag() + { + return delegate.checkAndClearBoundsFlag(); + } } diff --git a/community/io/src/test/java/org/neo4j/adversaries/pagecache/AdversarialWritePageCursor.java b/community/io/src/test/java/org/neo4j/adversaries/pagecache/AdversarialWritePageCursor.java index 4bd1d2c2433f..2cef68932b67 100644 --- a/community/io/src/test/java/org/neo4j/adversaries/pagecache/AdversarialWritePageCursor.java +++ b/community/io/src/test/java/org/neo4j/adversaries/pagecache/AdversarialWritePageCursor.java @@ -276,4 +276,10 @@ public int copyTo( int sourceOffset, PageCursor targetCursor, int targetOffset, adversary.injectFailure( IndexOutOfBoundsException.class ); return delegate.copyTo( sourceOffset, targetCursor, targetOffset, lengthInBytes ); } + + @Override + public boolean checkAndClearBoundsFlag() + { + return delegate.checkAndClearBoundsFlag(); + } } diff --git a/community/io/src/test/java/org/neo4j/io/pagecache/PageCacheTest.java b/community/io/src/test/java/org/neo4j/io/pagecache/PageCacheTest.java index 1d6a9ad61a4c..6ed14c79891a 100644 --- a/community/io/src/test/java/org/neo4j/io/pagecache/PageCacheTest.java +++ b/community/io/src/test/java/org/neo4j/io/pagecache/PageCacheTest.java @@ -48,7 +48,6 @@ import java.util.concurrent.atomic.AtomicReference; import org.neo4j.concurrent.BinaryLatch; -import org.neo4j.function.ThrowingAction; import org.neo4j.function.ThrowingConsumer; import org.neo4j.graphdb.mockfs.DelegatingFileSystemAbstraction; import org.neo4j.graphdb.mockfs.DelegatingStoreChannel; @@ -72,7 +71,6 @@ import static org.hamcrest.Matchers.both; import static org.hamcrest.Matchers.greaterThan; import static org.hamcrest.Matchers.greaterThanOrEqualTo; -import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.lessThan; import static org.hamcrest.Matchers.lessThanOrEqualTo; @@ -681,6 +679,14 @@ public void mappingFileWithPageSizeGreaterThanCachePageSizeMustThrow() throws IO cache.map( file( "a" ), pageCachePageSize + 1 ); // this must throw } + @Test( timeout = SHORT_TIMEOUT_MILLIS, expected = IllegalArgumentException.class ) + public void mappingFileWithPageSizeSmallerThanLongSizeBytesMustThrow() throws IOException + { + // Because otherwise we cannot ensure that our branch-free bounds checking always lands within a page boundary. + PageCache cache = getPageCache( fs, maxPages, pageCachePageSize, PageCacheTracer.NULL ); + cache.map( file( "a" ), Long.BYTES - 1 ); // this must throw + } + @Test( timeout = SHORT_TIMEOUT_MILLIS ) public void mappingFileWithPageSizeEqualToCachePageSizeMustNotThrow() throws IOException { @@ -892,113 +898,98 @@ public void pageCursorMustKnowCurrentFile() throws Exception @Test( timeout = SHORT_TIMEOUT_MILLIS ) public void readingFromUnboundReadCursorMustThrow() throws IOException { - verifyExceptionOnCursorRead( this::applyToUnboundReadCursor, RuntimeException.class ); + verifyOnReadCursor( this::checkUnboundReadCursorAccess ); } @Test( timeout = SHORT_TIMEOUT_MILLIS ) public void readingFromUnboundWriteCursorMustThrow() throws IOException { - verifyExceptionOnCursorRead( this::applyToUnboundWriteCursor, RuntimeException.class ); + verifyOnReadCursor( this::checkUnboundWriteCursorAccess ); } @Test( timeout = SHORT_TIMEOUT_MILLIS ) public void readingFromPreviouslyBoundCursorMustThrow() throws IOException { - verifyExceptionOnCursorRead( this::applyToPreviouslyBoundWriteCursor, RuntimeException.class ); + verifyOnReadCursor( this::checkPreviouslyBoundWriteCursorAccess ); } @Test( timeout = SHORT_TIMEOUT_MILLIS ) public void writingToUnboundCursorMustThrow() throws IOException { - verifyExceptionOnCursorWrite( this::applyToUnboundWriteCursor, RuntimeException.class ); + verifyOnWriteCursor( this::checkUnboundWriteCursorAccess ); } @Test( timeout = SHORT_TIMEOUT_MILLIS ) public void writingToPreviouslyBoundCursorMustThrow() throws IOException { - verifyExceptionOnCursorWrite( this::applyToPreviouslyBoundWriteCursor, RuntimeException.class ); + verifyOnWriteCursor( this::checkPreviouslyBoundWriteCursorAccess ); } @Test( timeout = SHORT_TIMEOUT_MILLIS ) public void readFromReadCursorAfterNextReturnsFalseMustThrow() throws Exception { - verifyExceptionOnCursorRead( this::applyToReadCursorAfterFailedNext, RuntimeException.class ); + verifyOnReadCursor( this::checkReadCursorAfterFailedNext ); } @Test( timeout = SHORT_TIMEOUT_MILLIS ) public void readFromPreviouslyBoundReadCursorAfterNextReturnsFalseMustThrow() throws Exception { - verifyExceptionOnCursorRead( this::applyToPreviouslyBoundReadCursorAfterFailedNext, RuntimeException.class ); + verifyOnReadCursor( this::checkPreviouslyBoundReadCursorAfterFailedNext ); } @Test( timeout = SHORT_TIMEOUT_MILLIS ) public void readFromWriteCursorAfterNextReturnsFalseMustThrow() throws Exception { - verifyExceptionOnCursorRead( this::applyToWriteCursorAfterFailedNext, RuntimeException.class ); + verifyOnReadCursor( this::checkWriteCursorAfterFailedNext ); } @Test( timeout = SHORT_TIMEOUT_MILLIS ) public void readFromPreviouslyBoundWriteCursorAfterNextReturnsFalseMustThrow() throws Exception { - verifyExceptionOnCursorRead( this::applyToPreviouslyBoundWriteCursorAfterFailedNext, RuntimeException.class ); + verifyOnReadCursor( this::checkPreviouslyBoundWriteCursorAfterFailedNext ); } @Test( timeout = SHORT_TIMEOUT_MILLIS ) public void writeAfterNextReturnsFalseMustThrow() throws Exception { - verifyExceptionOnCursorWrite( this::applyToWriteCursorAfterFailedNext, RuntimeException.class ); + verifyOnWriteCursor( this::checkWriteCursorAfterFailedNext ); } @Test( timeout = SHORT_TIMEOUT_MILLIS ) public void writeToPreviouslyBoundCursorAfterNextReturnsFalseMustThrow() throws Exception { - verifyExceptionOnCursorWrite( this::applyToPreviouslyBoundWriteCursorAfterFailedNext, RuntimeException.class ); + verifyOnWriteCursor( this::checkPreviouslyBoundWriteCursorAfterFailedNext ); } - private void verifyExceptionOnCursorRead( - ThrowingConsumer testTemplate, - Class exceptionType ) throws IOException + private void verifyOnReadCursor( + ThrowingConsumer testTemplate ) throws IOException { - assertThrows( exceptionType, () -> testTemplate.accept( PageCursor::getByte ) ); - assertThrows( exceptionType, () -> testTemplate.accept( PageCursor::getInt ) ); - assertThrows( exceptionType, () -> testTemplate.accept( PageCursor::getLong ) ); - assertThrows( exceptionType, () -> testTemplate.accept( PageCursor::getShort ) ); - assertThrows( exceptionType, () -> testTemplate.accept( PageCursor::getUnsignedInt ) ); - assertThrows( exceptionType, () -> testTemplate.accept( ( cursor ) -> cursor.getByte( 0 ) ) ); - assertThrows( exceptionType, () -> testTemplate.accept( ( cursor ) -> cursor.getInt( 0 ) ) ); - assertThrows( exceptionType, () -> testTemplate.accept( ( cursor ) -> cursor.getLong( 0 ) ) ); - assertThrows( exceptionType, () -> testTemplate.accept( ( cursor ) -> cursor.getShort( 0 ) ) ); - assertThrows( exceptionType, () -> testTemplate.accept( ( cursor ) -> cursor.getUnsignedInt( 0 ) ) ); + testTemplate.accept( PageCursor::getByte ); + testTemplate.accept( PageCursor::getInt ); + testTemplate.accept( PageCursor::getLong ); + testTemplate.accept( PageCursor::getShort ); + testTemplate.accept( PageCursor::getUnsignedInt ); + testTemplate.accept( ( cursor ) -> cursor.getByte( 0 ) ); + testTemplate.accept( ( cursor ) -> cursor.getInt( 0 ) ); + testTemplate.accept( ( cursor ) -> cursor.getLong( 0 ) ); + testTemplate.accept( ( cursor ) -> cursor.getShort( 0 ) ); + testTemplate.accept( ( cursor ) -> cursor.getUnsignedInt( 0 ) ); } - private void verifyExceptionOnCursorWrite( - ThrowingConsumer testTemplate, - Class exceptionType ) throws IOException + private void verifyOnWriteCursor( + ThrowingConsumer testTemplate ) throws IOException { - assertThrows( exceptionType, () -> testTemplate.accept( ( cursor ) -> cursor.putByte( (byte) 1 ) ) ); - assertThrows( exceptionType, () -> testTemplate.accept( ( cursor ) -> cursor.putInt( 1 ) ) ); - assertThrows( exceptionType, () -> testTemplate.accept( ( cursor ) -> cursor.putLong( 1 ) ) ); - assertThrows( exceptionType, () -> testTemplate.accept( ( cursor ) -> cursor.putShort( (short) 1 ) ) ); - assertThrows( exceptionType, () -> testTemplate.accept( ( cursor ) -> cursor.putByte( 0, (byte) 1 ) ) ); - assertThrows( exceptionType, () -> testTemplate.accept( ( cursor ) -> cursor.putInt( 0, 1 ) ) ); - assertThrows( exceptionType, () -> testTemplate.accept( ( cursor ) -> cursor.putLong( 0, 1 ) ) ); - assertThrows( exceptionType, () -> testTemplate.accept( ( cursor ) -> cursor.putShort( 0, (short) 1 ) ) ); + testTemplate.accept( ( cursor ) -> cursor.putByte( (byte) 1 ) ); + testTemplate.accept( ( cursor ) -> cursor.putInt( 1 ) ); + testTemplate.accept( ( cursor ) -> cursor.putLong( 1 ) ); + testTemplate.accept( ( cursor ) -> cursor.putShort( (short) 1 ) ); + testTemplate.accept( ( cursor ) -> cursor.putByte( 0, (byte) 1 ) ); + testTemplate.accept( ( cursor ) -> cursor.putInt( 0, 1 ) ); + testTemplate.accept( ( cursor ) -> cursor.putLong( 0, 1 ) ); + testTemplate.accept( ( cursor ) -> cursor.putShort( 0, (short) 1 ) ); } - private void assertThrows( Class exceptionType, ThrowingAction action ) - { - try - { - action.apply(); - fail( action + " should have thrown a " + exceptionType.getName() ); - } - catch ( Exception e ) - { - assertThat( e, instanceOf( exceptionType ) ); - } - } - - private void applyToUnboundReadCursor( PageCursorAction action ) throws IOException + private void checkUnboundReadCursorAccess( PageCursorAction action ) throws IOException { getPageCache( fs, maxPages, pageCachePageSize, PageCacheTracer.NULL ); @@ -1006,10 +997,12 @@ private void applyToUnboundReadCursor( PageCursorAction action ) throws IOExcept PageCursor cursor = pagedFile.io( 0, PF_SHARED_READ_LOCK ) ) { action.apply( cursor ); + assertTrue( cursor.checkAndClearBoundsFlag() ); + } } - private void applyToUnboundWriteCursor( PageCursorAction action ) throws IOException + private void checkUnboundWriteCursorAccess( PageCursorAction action ) throws IOException { getPageCache( fs, maxPages, pageCachePageSize, PageCacheTracer.NULL ); @@ -1017,10 +1010,11 @@ private void applyToUnboundWriteCursor( PageCursorAction action ) throws IOExcep PageCursor cursor = pagedFile.io( 0, PF_SHARED_WRITE_LOCK ) ) { action.apply( cursor ); + assertTrue( cursor.checkAndClearBoundsFlag() ); } } - private void applyToPreviouslyBoundWriteCursor( PageCursorAction action ) throws IOException + private void checkPreviouslyBoundWriteCursorAccess( PageCursorAction action ) throws IOException { getPageCache( fs, maxPages, pageCachePageSize, PageCacheTracer.NULL ); @@ -1029,12 +1023,14 @@ private void applyToPreviouslyBoundWriteCursor( PageCursorAction action ) throws PageCursor cursor = pagedFile.io( 0, PF_SHARED_WRITE_LOCK ); assertTrue( cursor.next() ); action.apply( cursor ); + assertFalse( cursor.checkAndClearBoundsFlag() ); cursor.close(); action.apply( cursor ); + assertTrue( cursor.checkAndClearBoundsFlag() ); } } - private void applyToReadCursorAfterFailedNext( PageCursorAction action ) throws IOException + private void checkReadCursorAfterFailedNext( PageCursorAction action ) throws IOException { getPageCache( fs, maxPages, pageCachePageSize, PageCacheTracer.NULL ); @@ -1043,10 +1039,11 @@ private void applyToReadCursorAfterFailedNext( PageCursorAction action ) throws { assertFalse( cursor.next() ); action.apply( cursor ); + assertTrue( cursor.checkAndClearBoundsFlag() ); } } - private void applyToPreviouslyBoundReadCursorAfterFailedNext( PageCursorAction action ) + private void checkPreviouslyBoundReadCursorAfterFailedNext( PageCursorAction action ) throws IOException { getPageCache( fs, maxPages, pageCachePageSize, PageCacheTracer.NULL ); @@ -1063,10 +1060,11 @@ private void applyToPreviouslyBoundReadCursorAfterFailedNext( PageCursorAction a assertTrue( cursor.next() ); assertFalse( cursor.next() ); action.apply( cursor ); + assertTrue( cursor.checkAndClearBoundsFlag() ); } } - private void applyToWriteCursorAfterFailedNext( PageCursorAction action ) throws IOException + private void checkWriteCursorAfterFailedNext( PageCursorAction action ) throws IOException { getPageCache( fs, maxPages, pageCachePageSize, PageCacheTracer.NULL ); @@ -1075,10 +1073,11 @@ private void applyToWriteCursorAfterFailedNext( PageCursorAction action ) throws { assertFalse( cursor.next() ); action.apply( cursor ); + assertTrue( cursor.checkAndClearBoundsFlag() ); } } - private void applyToPreviouslyBoundWriteCursorAfterFailedNext( PageCursorAction action ) + private void checkPreviouslyBoundWriteCursorAfterFailedNext( PageCursorAction action ) throws IOException { getPageCache( fs, maxPages, pageCachePageSize, PageCacheTracer.NULL ); @@ -1095,6 +1094,7 @@ private void applyToPreviouslyBoundWriteCursorAfterFailedNext( PageCursorAction assertTrue( cursor.next() ); assertFalse( cursor.next() ); action.apply( cursor ); + assertTrue( cursor.checkAndClearBoundsFlag() ); } } @@ -2614,12 +2614,166 @@ private void verifyPageBounds( PageCursorAction action ) throws IOException for ( int i = 0; i < 100000; i++ ) { action.apply( cursor ); + if ( cursor.checkAndClearBoundsFlag() ) + { + throw new IndexOutOfBoundsException(); + } } } } - @Test( timeout = SHORT_TIMEOUT_MILLIS, expected = IndexOutOfBoundsException.class ) - public void settingNegativeCursorOffsetMustThrow() throws IOException + @Test(timeout = SHORT_TIMEOUT_MILLIS) + public void shouldRetryMustClearBoundsFlagWhenReturningTrue() throws Exception + { + getPageCache( fs, maxPages, pageCachePageSize, PageCacheTracer.NULL ); + + try ( PagedFile pf = pageCache.map( file( "a" ), filePageSize ); + PageCursor reader = pf.io( 0, PF_SHARED_READ_LOCK ) ) + { + PageCursor writer = pf.io( 0, PF_SHARED_WRITE_LOCK ); + assertTrue( writer.next() ); + + assertTrue( reader.next() ); + reader.getByte( -1 ); // out-of-bounds flag now raised + writer.close(); // reader overlapped with writer, so must retry + assertTrue( reader.shouldRetry() ); + + // shouldRetry returned 'true', so it must clear the out-of-bounds flag + assertFalse( reader.checkAndClearBoundsFlag() ); + } + } + + @Test(timeout = SHORT_TIMEOUT_MILLIS) + public void nextMustClearBoundsFlagOnReadCursor() throws Exception + { + getPageCache( fs, maxPages, pageCachePageSize, PageCacheTracer.NULL ); + + try ( PagedFile pf = pageCache.map( file( "a" ), filePageSize ); + PageCursor reader = pf.io( 0, PF_SHARED_READ_LOCK ) ) + { + PageCursor writer = pf.io( 0, PF_SHARED_WRITE_LOCK ); + assertTrue( writer.next() ); + + assertTrue( reader.next() ); + reader.getByte( -1 ); // out-of-bounds flag now raised + writer.next(); // make sure there's a next page for the reader to move to + writer.close(); // reader overlapped with writer, so must retry + assertTrue( reader.next() ); + + // shouldRetry returned 'true', so it must clear the out-of-bounds flag + assertFalse( reader.checkAndClearBoundsFlag() ); + } + } + + @Test(timeout = SHORT_TIMEOUT_MILLIS) + public void nextMustClearBoundsFlagOnWriteCursor() throws Exception + { + getPageCache( fs, maxPages, pageCachePageSize, PageCacheTracer.NULL ); + + try ( PagedFile pf = pageCache.map( file( "a" ), filePageSize ); + PageCursor reader = pf.io( 0, PF_SHARED_WRITE_LOCK ) ) + { + PageCursor writer = pf.io( 0, PF_SHARED_WRITE_LOCK ); + assertTrue( writer.next() ); + + assertTrue( reader.next() ); + reader.getByte( -1 ); // out-of-bounds flag now raised + writer.close(); // reader overlapped with writer, so must retry + assertTrue( reader.next() ); + + // shouldRetry returned 'true', so it must clear the out-of-bounds flag + assertFalse( reader.checkAndClearBoundsFlag() ); + } + } + + @Test(timeout = SHORT_TIMEOUT_MILLIS) + public void nextThatReturnsFalseMustNotClearBoundsFlagOnReadCursor() throws Exception + { + getPageCache( fs, maxPages, pageCachePageSize, PageCacheTracer.NULL ); + + try ( PagedFile pf = pageCache.map( file( "a" ), filePageSize ); + PageCursor reader = pf.io( 0, PF_SHARED_READ_LOCK ) ) + { + PageCursor writer = pf.io( 0, PF_SHARED_WRITE_LOCK ); + assertTrue( writer.next() ); + + assertTrue( reader.next() ); + reader.getByte( -1 ); // out-of-bounds flag now raised + // don't call next of the writer, so there won't be a page for the reader to move onto + writer.close(); // reader overlapped with writer, so must retry + assertFalse( reader.next() ); + + // shouldRetry returned 'true', so it must clear the out-of-bounds flag + assertTrue( reader.checkAndClearBoundsFlag() ); + } + } + + @Test(timeout = SHORT_TIMEOUT_MILLIS) + public void nextThatReturnsFalseMustNotClearBoundsFlagOnWriteCursor() throws Exception + { + getPageCache( fs, maxPages, pageCachePageSize, PageCacheTracer.NULL ); + + try ( PagedFile pf = pageCache.map( file( "a" ), filePageSize ); + PageCursor reader = pf.io( 0, PF_SHARED_WRITE_LOCK | PF_NO_GROW ) ) + { + PageCursor writer = pf.io( 0, PF_SHARED_WRITE_LOCK ); + assertTrue( writer.next() ); + + assertTrue( reader.next() ); + reader.getByte( -1 ); // out-of-bounds flag now raised + writer.close(); // reader overlapped with writer, so must retry + assertFalse( reader.next() ); + + // shouldRetry returned 'true', so it must clear the out-of-bounds flag + assertTrue( reader.checkAndClearBoundsFlag() ); + } + } + + @Test(timeout = SHORT_TIMEOUT_MILLIS) + public void nextWithPageIdMustClearBoundsFlagOnReadCursor() throws Exception + { + getPageCache( fs, maxPages, pageCachePageSize, PageCacheTracer.NULL ); + + try ( PagedFile pf = pageCache.map( file( "a" ), filePageSize ); + PageCursor reader = pf.io( 0, PF_SHARED_READ_LOCK ) ) + { + PageCursor writer = pf.io( 0, PF_SHARED_WRITE_LOCK ); + assertTrue( writer.next() ); + + assertTrue( reader.next() ); + reader.getByte( -1 ); // out-of-bounds flag now raised + writer.next( 3 ); // make sure there's a next page for the reader to move to + writer.close(); // reader overlapped with writer, so must retry + assertTrue( reader.next( 3 ) ); + + // shouldRetry returned 'true', so it must clear the out-of-bounds flag + assertFalse( reader.checkAndClearBoundsFlag() ); + } + } + + @Test(timeout = SHORT_TIMEOUT_MILLIS) + public void nextWithPageIdMustClearBoundsFlagOnWriteCursor() throws Exception + { + getPageCache( fs, maxPages, pageCachePageSize, PageCacheTracer.NULL ); + + try ( PagedFile pf = pageCache.map( file( "a" ), filePageSize ); + PageCursor reader = pf.io( 0, PF_SHARED_WRITE_LOCK ) ) + { + PageCursor writer = pf.io( 0, PF_SHARED_WRITE_LOCK ); + assertTrue( writer.next() ); + + assertTrue( reader.next() ); + reader.getByte( -1 ); // out-of-bounds flag now raised + writer.close(); // reader overlapped with writer, so must retry + assertTrue( reader.next( 3 ) ); + + // shouldRetry returned 'true', so it must clear the out-of-bounds flag + assertFalse( reader.checkAndClearBoundsFlag() ); + } + } + + @Test( timeout = SHORT_TIMEOUT_MILLIS ) + public void settingNegativeCursorOffsetMustRaiseBoundsFlag() throws IOException { generateFileWithRecords( file( "a" ), 1, recordSize ); @@ -2628,6 +2782,7 @@ public void settingNegativeCursorOffsetMustThrow() throws IOException PageCursor cursor = pagedFile.io( 0, PF_SHARED_READ_LOCK ) ) { cursor.setOffset( -1 ); + assertTrue( cursor.checkAndClearBoundsFlag() ); } } @@ -3780,65 +3935,39 @@ public void copyToMustCheckBounds() throws Exception assertTrue( cursorA.next() ); // source buffer underflow - try - { - cursorA.copyTo( -1, cursorB, 0, 1 ); - fail( "should have thrown on source buffer underflow" ); - } - catch ( IndexOutOfBoundsException ignore ) - { - // expected - } + cursorA.copyTo( -1, cursorB, 0, 1 ); + assertTrue( cursorA.checkAndClearBoundsFlag() ); + assertFalse( cursorB.checkAndClearBoundsFlag() ); // target buffer underflow - try - { - cursorA.copyTo( 0, cursorB, -1, 1 ); - fail( "should have thrown on target buffer underflow" ); - } - catch ( IndexOutOfBoundsException ignore ) - { - // expected - } + cursorA.copyTo( 0, cursorB, -1, 1 ); + assertTrue( cursorA.checkAndClearBoundsFlag() ); + assertFalse( cursorB.checkAndClearBoundsFlag() ); // source buffer offset overflow - try - { - cursorA.copyTo( pageSize, cursorB, 0, 1 ); - fail( "should have thrown on source buffer overflow" ); - } - catch ( IndexOutOfBoundsException ignore ) - { - // expected - } + cursorA.copyTo( pageSize, cursorB, 0, 1 ); + assertTrue( cursorA.checkAndClearBoundsFlag() ); + assertFalse( cursorB.checkAndClearBoundsFlag() ); // target buffer offset overflow - try - { - cursorA.copyTo( 0, cursorB, pageSize, 1 ); - fail( "should have thrown on target buffer overflow" ); - } - catch ( IndexOutOfBoundsException ignore ) - { - // expected - } + cursorA.copyTo( 0, cursorB, pageSize, 1 ); + assertTrue( cursorA.checkAndClearBoundsFlag() ); + assertFalse( cursorB.checkAndClearBoundsFlag() ); // source buffer length overflow assertThat( cursorA.copyTo( 1, cursorB, 0, pageSize ), is( pageSize - 1 ) ); + assertFalse( cursorA.checkAndClearBoundsFlag() ); + assertFalse( cursorB.checkAndClearBoundsFlag() ); // target buffer length overflow assertThat( cursorA.copyTo( 0, cursorB, 1, pageSize ), is( pageSize - 1 ) ); + assertFalse( cursorA.checkAndClearBoundsFlag() ); + assertFalse( cursorB.checkAndClearBoundsFlag() ); // negative length - try - { - cursorA.copyTo( 1, cursorB, 1, -1 ); - fail( "should have thrown on negative length" ); - } - catch ( IllegalArgumentException ignore ) - { - // expected - } + cursorA.copyTo( 1, cursorB, 1, -1 ); + assertTrue( cursorA.checkAndClearBoundsFlag() ); + assertFalse( cursorB.checkAndClearBoundsFlag() ); } } diff --git a/community/io/src/test/java/org/neo4j/io/pagecache/StubPageCursor.java b/community/io/src/test/java/org/neo4j/io/pagecache/StubPageCursor.java index c9665ab2479b..07181aff91d1 100644 --- a/community/io/src/test/java/org/neo4j/io/pagecache/StubPageCursor.java +++ b/community/io/src/test/java/org/neo4j/io/pagecache/StubPageCursor.java @@ -105,6 +105,12 @@ public int copyTo( int sourceOffset, PageCursor targetCursor, int targetOffset, return 0; } + @Override + public boolean checkAndClearBoundsFlag() + { + return false; + } + @Override public byte getByte() { diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/store/format/RecordBoundaryCheckingPagedFile.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/store/format/RecordBoundaryCheckingPagedFile.java index 207019027bb2..26a63a734093 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/store/format/RecordBoundaryCheckingPagedFile.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/store/format/RecordBoundaryCheckingPagedFile.java @@ -291,6 +291,12 @@ public int copyTo( int sourceOffset, PageCursor targetCursor, int targetOffset, return actual.copyTo( sourceOffset, targetCursor, targetOffset, lengthInBytes ); } + @Override + public boolean checkAndClearBoundsFlag() + { + return false; + } + @Override public void setOffset( int offset ) {