From b13478ecea065613f544dd5e5e8cfa1768beec42 Mon Sep 17 00:00:00 2001 From: Mattias Persson Date: Mon, 9 Jan 2017 16:32:29 +0100 Subject: [PATCH] GB+Tree tests run with adversarial page cache also included are fixes for issues exposed by this added testing: - Removed Layout#read/writeMetaData because there's no need for it and it was implemented in the wrong way w/ regards to shouldRetry - KeySearch#search doesn't throw exception on unexpected end result, instead uses one of the high bits to signal success/failure. - GSPP#pointerState no longer throws exception on unexpected generation because it was done inside shouldRetry loop. Now instead reports BROKEN state. - SeekCursor asserts shouldRetry is true after unexpected read, otherwise throws exception about tree inconsistency. - Fixes a bug in AdversarialWritePageCursor#copyTo where this wasn't allowed previously, due to an instanceof check. --- .../java/org/neo4j/index/gbptree/GBPTree.java | 9 +++ .../neo4j/index/gbptree/GenSafePointer.java | 11 ++-- .../index/gbptree/GenSafePointerPair.java | 3 +- .../index/gbptree/InternalTreeLogic.java | 20 ++++-- .../org/neo4j/index/gbptree/KeySearch.java | 29 +++++++-- .../java/org/neo4j/index/gbptree/Layout.java | 3 +- .../org/neo4j/index/gbptree/SeekCursor.java | 64 ++++++++++++++++--- .../org/neo4j/index/gbptree/GBPTreeIT.java | 49 +++++++------- .../index/gbptree/GBPTreeRecoveryTest.java | 16 ++--- .../org/neo4j/index/gbptree/GBPTreeTest.java | 60 +++++++++-------- .../neo4j/index/gbptree/SeekCursorTest.java | 59 +++++++++++++++++ .../neo4j/index/gbptree/SimpleLongLayout.java | 16 ++++- .../pagecache/AdversarialWritePageCursor.java | 6 +- 13 files changed, 249 insertions(+), 96 deletions(-) diff --git a/community/index/src/main/java/org/neo4j/index/gbptree/GBPTree.java b/community/index/src/main/java/org/neo4j/index/gbptree/GBPTree.java index fa2fafce4d302..039ba82835cbf 100644 --- a/community/index/src/main/java/org/neo4j/index/gbptree/GBPTree.java +++ b/community/index/src/main/java/org/neo4j/index/gbptree/GBPTree.java @@ -39,6 +39,7 @@ import org.neo4j.index.IndexWriter; import org.neo4j.index.ValueMerger; import org.neo4j.index.ValueMergers; +import org.neo4j.io.pagecache.CursorException; import org.neo4j.io.pagecache.IOLimiter; import org.neo4j.io.pagecache.PageCache; import org.neo4j.io.pagecache.PageCursor; @@ -439,7 +440,15 @@ private void readMeta( File indexFile, Layout layout, PagedFile paged } while ( metaCursor.shouldRetry() ); checkOutOfBounds( metaCursor ); + metaCursor.checkAndClearCursorException(); } + catch ( CursorException e ) + { + throw new MetadataMismatchException( format( + "Tried to open %s, but caught an error while reading meta data. " + + "File is expected to be corrupt, try to rebuild.", indexFile ), e ); + } + if ( formatVersion != FORMAT_VERSION ) { throw new MetadataMismatchException( "Tried to open %s with a different format version than " + diff --git a/community/index/src/main/java/org/neo4j/index/gbptree/GenSafePointer.java b/community/index/src/main/java/org/neo4j/index/gbptree/GenSafePointer.java index a67c155449449..48fb1aa7968c3 100644 --- a/community/index/src/main/java/org/neo4j/index/gbptree/GenSafePointer.java +++ b/community/index/src/main/java/org/neo4j/index/gbptree/GenSafePointer.java @@ -97,23 +97,22 @@ private static void assertPointerOnWrite( long pointer ) } } - public static long readGeneration( PageCursor cursor ) + static long readGeneration( PageCursor cursor ) { return getUnsignedInt( cursor ); } - public static long readPointer( PageCursor cursor ) + static long readPointer( PageCursor cursor ) { - long result = get6BLong( cursor ); - return result; + return get6BLong( cursor ); } - public static short readChecksum( PageCursor cursor ) + static short readChecksum( PageCursor cursor ) { return cursor.getShort(); } - public static boolean verifyChecksum( PageCursor cursor, long generation, long pointer ) + static boolean verifyChecksum( PageCursor cursor, long generation, long pointer ) { short checksum = cursor.getShort(); return checksum == checksumOf( generation, pointer ); diff --git a/community/index/src/main/java/org/neo4j/index/gbptree/GenSafePointerPair.java b/community/index/src/main/java/org/neo4j/index/gbptree/GenSafePointerPair.java index 931b2363b4cc1..77828407066ce 100644 --- a/community/index/src/main/java/org/neo4j/index/gbptree/GenSafePointerPair.java +++ b/community/index/src/main/java/org/neo4j/index/gbptree/GenSafePointerPair.java @@ -354,8 +354,7 @@ static byte pointerState( long stableGeneration, long unstableGeneration, } if ( generation < MIN_GENERATION ) { - throw new TreeInconsistencyException( "Generation was less than MIN_GENERATION " + MIN_GENERATION + - " but checksum was correct. Pointer was " + generation + "," + pointer ); + return BROKEN; } if ( generation <= stableGeneration ) { diff --git a/community/index/src/main/java/org/neo4j/index/gbptree/InternalTreeLogic.java b/community/index/src/main/java/org/neo4j/index/gbptree/InternalTreeLogic.java index 64b9c806cf426..35d440efa5fee 100644 --- a/community/index/src/main/java/org/neo4j/index/gbptree/InternalTreeLogic.java +++ b/community/index/src/main/java/org/neo4j/index/gbptree/InternalTreeLogic.java @@ -28,7 +28,6 @@ import static org.neo4j.index.gbptree.KeySearch.isHit; import static org.neo4j.index.gbptree.KeySearch.positionOf; -import static org.neo4j.index.gbptree.KeySearch.search; import static org.neo4j.index.gbptree.PageCursorUtil.goTo; /** @@ -258,7 +257,7 @@ private void moveToCorrectLeaf( PageCursor cursor, KEY key, long stableGeneratio { // We still need to go down further, but we're on the right path int keyCount = bTreeNode.keyCount( cursor ); - int searchResult = search( cursor, bTreeNode, key, readKey, keyCount ); + int searchResult = search( cursor, key, readKey, keyCount ); int childPos = positionOf( searchResult ); if ( isHit( searchResult ) ) { @@ -373,6 +372,13 @@ void insert( PageCursor cursor, StructurePropagation structurePropagation, } } + private int search( PageCursor cursor, KEY key, KEY readKey, int keyCount ) + { + int searchResult = KeySearch.search( cursor, bTreeNode, key, readKey, keyCount ); + KeySearch.assertSuccess( searchResult ); + return searchResult; + } + /** * Asserts that cursor is where it's expected to be at, compared to current level. * @@ -409,7 +415,7 @@ private void insertInInternal( PageCursor cursor, StructurePropagation stru if ( keyCount < bTreeNode.internalMaxKeyCount() ) { // No overflow - int pos = positionOf( search( cursor, bTreeNode, primKey, readKey, keyCount ) ); + int pos = positionOf( search( cursor, primKey, readKey, keyCount ) ); bTreeNode.insertKeyAt( cursor, primKey, pos, keyCount ); // NOTE pos+1 since we never insert a new child before child(0) because its key is really @@ -450,7 +456,7 @@ private void splitInternal( PageCursor cursor, StructurePropagation structu long newRight = idProvider.acquireNewId( stableGeneration, unstableGeneration ); // Find position to insert new key - int pos = positionOf( search( cursor, bTreeNode, primKey, readKey, keyCount ) ); + int pos = positionOf( search( cursor, primKey, readKey, keyCount ) ); int keyCountAfterInsert = keyCount + 1; int middlePos = middle( keyCountAfterInsert ); @@ -598,7 +604,7 @@ private void insertInLeaf( PageCursor cursor, StructurePropagation structur long stableGeneration, long unstableGeneration ) throws IOException { int keyCount = bTreeNode.keyCount( cursor ); - int search = search( cursor, bTreeNode, key, readKey, keyCount ); + int search = search( cursor, key, readKey, keyCount ); int pos = positionOf( search ); if ( isHit( search ) ) { @@ -704,7 +710,7 @@ private void splitLeaf( PageCursor cursor, StructurePropagation structurePr // 5. Write new key/values into L // Position where newKey / newValue is to be inserted - int pos = positionOf( search( cursor, bTreeNode, newKey, readKey, keyCount ) ); + int pos = positionOf( search( cursor, newKey, readKey, keyCount ) ); int keyCountAfterInsert = keyCount + 1; int middlePos = middle( keyCountAfterInsert ); @@ -876,7 +882,7 @@ private boolean removeFromLeaf( PageCursor cursor, StructurePropagation str int keyCount = bTreeNode.keyCount( cursor ); // No overflow, insert key and value - int search = search( cursor, bTreeNode, key, readKey, keyCount ); + int search = search( cursor, key, readKey, keyCount ); int pos = positionOf( search ); boolean hit = isHit( search ); if ( !hit ) diff --git a/community/index/src/main/java/org/neo4j/index/gbptree/KeySearch.java b/community/index/src/main/java/org/neo4j/index/gbptree/KeySearch.java index 38be808f69f5a..e3c1d83def5c3 100644 --- a/community/index/src/main/java/org/neo4j/index/gbptree/KeySearch.java +++ b/community/index/src/main/java/org/neo4j/index/gbptree/KeySearch.java @@ -28,8 +28,13 @@ */ class KeySearch { - private static final int POSITION_MASK = 0x7FFFFFFF; - private static final int HIT_MASK = 0x80000000; + private static final int POSITION_MASK = 0x3FFFFFFF; + private static final int HIT_FLAG = 0x80000000; + private static final int NO_HIT_FLAG = 0x00000000; + private static final int HIT_MASK = HIT_FLAG | NO_HIT_FLAG; + private static final int SUCCESS_FLAG = 0x00000000; + private static final int NO_SUCCESS_FLAG = 0x40000000; + private static final int SUCCESS_MASK = SUCCESS_FLAG | NO_SUCCESS_FLAG; /** * Search for left most pos such that keyAtPos obeys key <= keyAtPos. @@ -106,8 +111,7 @@ else if ( (comparison = comparator.compare( key, bTreeNode.keyAt( cursor, readKe } if ( lower != higher ) { - throw new TreeInconsistencyException( "The binary search terminated in an unexpected way. " + - "Expected lower and higher to be equal but was " + "lower=" + lower + ", higher=" + higher ); + return NO_SUCCESS_FLAG; } pos = lower; @@ -118,7 +122,7 @@ else if ( (comparison = comparator.compare( key, bTreeNode.keyAt( cursor, readKe private static int searchResult( int pos, boolean hit ) { - return (pos & POSITION_MASK) | ((hit ? 1 : 0) << 31); + return (pos & POSITION_MASK) | (hit ? HIT_FLAG : NO_HIT_FLAG); } /** @@ -141,6 +145,19 @@ static int positionOf( int searchResult ) */ static boolean isHit( int searchResult ) { - return (searchResult & HIT_MASK) != 0; + return (searchResult & HIT_MASK) == HIT_FLAG; + } + + static boolean isSuccess( int searchResult ) + { + return (searchResult & SUCCESS_MASK) == SUCCESS_FLAG; + } + + static void assertSuccess( int searchResult ) + { + if ( !isSuccess( searchResult ) ) + { + throw new TreeInconsistencyException( "Search terminated in unexpected way" ); + } } } diff --git a/community/index/src/main/java/org/neo4j/index/gbptree/Layout.java b/community/index/src/main/java/org/neo4j/index/gbptree/Layout.java index 93da28dd0322e..7f21a54400abd 100644 --- a/community/index/src/main/java/org/neo4j/index/gbptree/Layout.java +++ b/community/index/src/main/java/org/neo4j/index/gbptree/Layout.java @@ -128,9 +128,10 @@ public interface Layout extends Comparator * Reads meta data specific to this layout instance from {@code cursor} at its current offset. * The read meta data must also be verified against meta data provided in constructor of this Layout. * Constructor-provided meta data can be {@code null} to skip this verification. + * if read meta data doesn't match with the meta data provided in constructor + * {@link PageCursor#setCursorException(String)} should be called with appropriate error message. * * @param cursor {@link PageCursor} to read from, at its current offset. - * @throws MetadataMismatchException if read meta data doesn't match with the meta data provided in constructor. */ void readMetaData( PageCursor cursor ); diff --git a/community/index/src/main/java/org/neo4j/index/gbptree/SeekCursor.java b/community/index/src/main/java/org/neo4j/index/gbptree/SeekCursor.java index 1c2b96e97c077..13afdecbf990f 100644 --- a/community/index/src/main/java/org/neo4j/index/gbptree/SeekCursor.java +++ b/community/index/src/main/java/org/neo4j/index/gbptree/SeekCursor.java @@ -228,9 +228,9 @@ class SeekCursor implements RawCursor,IOException>, Hi private int pos; /** - * Number of keys in the current leaf, this value is cached and only re-read every time there's - * a {@link PageCursor#shouldRetry() retry due to concurrent write}. - */ + * Number of keys in the current leaf, this value is cached and only re-read every time there's + * a {@link PageCursor#shouldRetry() retry due to concurrent write}. + */ private int keyCount; /** @@ -394,21 +394,32 @@ private void traverseDownToFirstLeaf() throws IOException do { // Read + boolean fullRead; + int searchResult = 0; do { + fullRead = false; + // Where we are if ( !readHeader() ) { continue; } - // Where we're going - pos = searchKey( fromInclusive ); + searchResult = searchKey( fromInclusive ); + if ( !KeySearch.isSuccess( searchResult ) ) + { + continue; + } + pos = positionOf( searchResult ); + if ( isInternal ) { pointerId = bTreeNode.childAt( cursor, pos, stableGeneration, unstableGeneration ); pointerGen = readPointerGenOnSuccess( pointerId ); } + + fullRead = true; } while ( cursor.shouldRetry() ); checkOutOfBounds( cursor ); @@ -421,6 +432,15 @@ private void traverseDownToFirstLeaf() throws IOException continue; } + if ( !fullRead ) + { + throw new TreeInconsistencyException( "Read inconsistent tree node %d%n" + + " nodeType:%d%n currentNodeGen:%d%n newGen:%d%n newGenGen:%d%n isInternal:%b%n" + + " keyCount:%d%n maxKeyCount:%d%n searchResult:%d%n pos:%d%n childId:%d%n childIdGen:%d", + cursor.getCurrentPageId(), nodeType, currentNodeGen, newGen, newGenGen, + isInternal, keyCount, maxKeyCount, searchResult, pos, pointerId, pointerGen ); + } + if ( goToNewGen() ) { continue; @@ -449,9 +469,13 @@ public boolean next() throws IOException while ( true ) { pos += stride; + boolean fullRead; + int searchResult = 0; // Read do { + fullRead = false; + // Where we are if ( !readHeader() ) { @@ -468,7 +492,13 @@ public boolean next() throws IOException { // Keys could have been moved so we need to make sure we are not missing any keys by // moving position back until we find previously returned key - pos = searchKey( first ? fromInclusive : prevKey ); + searchResult = searchKey( first ? fromInclusive : prevKey ); + if ( !KeySearch.isSuccess( searchResult ) ) + { + continue; + } + pos = positionOf( searchResult ); + if ( !seekForward && pos >= keyCount ) { // We may need to go to previous sibling to find correct place to start seeking from @@ -490,6 +520,8 @@ public boolean next() throws IOException bTreeNode.keyAt( cursor, mutableKey, pos ); bTreeNode.valueAt( cursor, mutableValue, pos ); } + + fullRead = true; } while ( concurrentWriteHappened = cursor.shouldRetry() ); checkOutOfBounds( cursor ); @@ -503,6 +535,16 @@ public boolean next() throws IOException continue; } + if ( !fullRead ) + { + throw new TreeInconsistencyException( "Read inconsistent tree node %d%n" + + " nodeType:%d%n currentNodeGen:%d%n newGen:%d%n newGenGen:%d%n" + + " keyCount:%d%n maxKeyCount:%d%n searchResult:%d%n pos:%d%n" + + " rightSibling:%d%n rightSiblingGen:%d", + cursor.getCurrentPageId(), nodeType, currentNodeGen, newGen, newGenGen, + keyCount, maxKeyCount, searchResult, pos, pointerId, pointerGen ); + } + if ( !verifyFirstKeyInNodeIsExpectedAfterGoTo() ) { continue; @@ -673,11 +715,15 @@ private long readNextSibling() */ private int searchKey( KEY key ) { - int search = KeySearch.search( cursor, bTreeNode, key, mutableKey, keyCount ); - int pos = KeySearch.positionOf( search ); + return KeySearch.search( cursor, bTreeNode, key, mutableKey, keyCount ); + } + + private int positionOf( int searchResult ) + { + int pos = KeySearch.positionOf( searchResult ); // Assuming unique keys - if ( isInternal && KeySearch.isHit( search ) ) + if ( isInternal && KeySearch.isHit( searchResult ) ) { pos++; } diff --git a/community/index/src/test/java/org/neo4j/index/gbptree/GBPTreeIT.java b/community/index/src/test/java/org/neo4j/index/gbptree/GBPTreeIT.java index d3e1cbafbcfb5..9e928bccb1bcb 100644 --- a/community/index/src/test/java/org/neo4j/index/gbptree/GBPTreeIT.java +++ b/community/index/src/test/java/org/neo4j/index/gbptree/GBPTreeIT.java @@ -24,9 +24,6 @@ import org.junit.Rule; import org.junit.Test; import org.junit.rules.RuleChain; -import org.junit.rules.TemporaryFolder; - -import java.io.File; import java.io.IOException; import java.util.Comparator; import java.util.Map; @@ -46,13 +43,12 @@ import org.neo4j.index.Hit; import org.neo4j.index.Index; import org.neo4j.index.IndexWriter; -import org.neo4j.io.fs.DefaultFileSystemAbstraction; import org.neo4j.io.pagecache.IOLimiter; import org.neo4j.io.pagecache.PageCache; -import org.neo4j.io.pagecache.PageSwapperFactory; -import org.neo4j.io.pagecache.impl.SingleFilePageSwapperFactory; -import org.neo4j.io.pagecache.impl.muninn.MuninnPageCache; +import org.neo4j.test.rule.PageCacheRule; import org.neo4j.test.rule.RandomRule; +import org.neo4j.test.rule.TestDirectory; +import org.neo4j.test.rule.fs.DefaultFileSystemRule; import static java.lang.Integer.max; import static java.util.concurrent.TimeUnit.MILLISECONDS; @@ -60,19 +56,25 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; +import static org.junit.rules.RuleChain.outerRule; + import static org.neo4j.index.gbptree.GBPTree.NO_MONITOR; -import static org.neo4j.io.pagecache.tracing.PageCacheTracer.NULL; +import static org.neo4j.test.rule.PageCacheRule.config; public class GBPTreeIT { - private final TemporaryFolder folder = new TemporaryFolder( new File( "target" ) ); + private final DefaultFileSystemRule fs = new DefaultFileSystemRule(); + private final TestDirectory directory = TestDirectory.testDirectory( getClass(), fs.get() ); + private final PageCacheRule pageCacheRule = new PageCacheRule(); private final RandomRule random = new RandomRule(); + @Rule - public RuleChain ruleChain = RuleChain.outerRule( random ).around( folder ); + public final RuleChain rules = outerRule( fs ).around( directory ).around( pageCacheRule ).around( random ); private final Layout layout = new SimpleLongLayout(); private GBPTree index; - private ExecutorService threadPool = Executors.newFixedThreadPool( Runtime.getRuntime().availableProcessors() ); + private final ExecutorService threadPool = Executors.newFixedThreadPool( Runtime.getRuntime().availableProcessors() ); + private PageCache pageCache; private GBPTree createIndex( int pageSize ) throws IOException @@ -83,23 +85,17 @@ private GBPTree createIndex( int pageSize ) private GBPTree createIndex( int pageSize, GBPTree.Monitor monitor ) throws IOException { - PageCache pageCache = new MuninnPageCache( swapperFactory(), 10_000, pageSize, NULL ); - File indexFile = new File( folder.getRoot(), "index" ); - return index = new GBPTree<>( pageCache, indexFile, layout, 0/*use whatever page cache says*/, monitor ); - } - - private static PageSwapperFactory swapperFactory() - { - PageSwapperFactory swapperFactory = new SingleFilePageSwapperFactory(); - swapperFactory.setFileSystemAbstraction( new DefaultFileSystemAbstraction() ); - return swapperFactory; + pageCache = pageCacheRule.getPageCache( fs.get(), config().withPageSize( pageSize ) ); + return index = new GBPTree<>( pageCache, directory.file( "index" ), + layout, 0/*use whatever page cache says*/, monitor ); } @After - public void consistencyCheck() throws IOException + public void consistencyCheckAndClose() throws IOException { threadPool.shutdownNow(); index.consistencyCheck(); + index.close(); } @Test @@ -278,7 +274,7 @@ public void shouldReadCorrectlyWhenConcurrentlyInsertingInOrder() throws Throwab startSignal.countDown(); Random random1 = ThreadLocalRandom.current(); int inserted = 0; - while ( (inserted < 100_000 || numberOfReads.get() < 100) && !failHalt.get() ) + while ( (inserted < 100_000 || numberOfReads.get() < 10_000) && !failHalt.get() ) { try ( IndexWriter writer = index.writer() ) { @@ -320,7 +316,7 @@ public void shouldReadCorrectlyWhenConcurrentlyInsertingOutOfOrder() throws Thro int nbrOfGroups = 10; int wantedRangeWidth = 1_000; int rangeWidth = wantedRangeWidth - wantedRangeWidth % nbrOfGroups; - int wantedNbrOfReads = 10_000; + int wantedNbrOfReads = 100_000; // Readers config int readers = max( 1, Runtime.getRuntime().availableProcessors() - 1 ); @@ -444,7 +440,10 @@ public void shouldReadCorrectlyWhenConcurrentlyInsertingOutOfOrderAndSeekingBack assertTrue( readerReadySignal.await( 10, SECONDS ) ); startSignal.countDown(); int iteration = currentWriteIteration.get(); - while ( !failHalt.get() && numberOfReads.get() < wantedNbrOfReads ) + int writesPerIteration = rangeWidth / nbrOfGroups; + int nbrOfLeastWantedIterations = 100_000 / writesPerIteration; + while ( !failHalt.get() && + (numberOfReads.get() < wantedNbrOfReads || iteration < nbrOfLeastWantedIterations) ) { try ( IndexWriter writer = index.writer() ) { diff --git a/community/index/src/test/java/org/neo4j/index/gbptree/GBPTreeRecoveryTest.java b/community/index/src/test/java/org/neo4j/index/gbptree/GBPTreeRecoveryTest.java index 4fcbc55e1f574..0ec8b895e1b9b 100644 --- a/community/index/src/test/java/org/neo4j/index/gbptree/GBPTreeRecoveryTest.java +++ b/community/index/src/test/java/org/neo4j/index/gbptree/GBPTreeRecoveryTest.java @@ -39,9 +39,7 @@ import org.neo4j.index.ValueMerger; import org.neo4j.io.pagecache.IOLimiter; import org.neo4j.io.pagecache.PageCache; -import org.neo4j.io.pagecache.PageSwapperFactory; -import org.neo4j.io.pagecache.impl.SingleFilePageSwapperFactory; -import org.neo4j.io.pagecache.impl.muninn.MuninnPageCache; +import org.neo4j.test.rule.PageCacheRule; import org.neo4j.test.rule.RandomRule; import org.neo4j.test.rule.TestDirectory; import org.neo4j.test.rule.fs.EphemeralFileSystemRule; @@ -49,23 +47,25 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; +import static org.junit.rules.RuleChain.outerRule; import static org.neo4j.index.gbptree.GBPTree.NO_MONITOR; import static org.neo4j.index.gbptree.ThrowingRunnable.throwing; import static org.neo4j.io.pagecache.IOLimiter.unlimited; -import static org.neo4j.io.pagecache.tracing.PageCacheTracer.NULL; +import static org.neo4j.test.rule.PageCacheRule.config; public class GBPTreeRecoveryTest { private static final int PAGE_SIZE = 256; private static final Action CHECKPOINT = new CheckpointAction(); - private final RandomRule random = new RandomRule(); private final EphemeralFileSystemRule fs = new EphemeralFileSystemRule(); private final TestDirectory directory = TestDirectory.testDirectory( getClass(), fs.get() ); + private final PageCacheRule pageCacheRule = new PageCacheRule( config().withPageSize( PAGE_SIZE ) ); + private final RandomRule random = new RandomRule(); @Rule - public RuleChain ruleChain = RuleChain.outerRule( random ).around( fs ).around( directory ); + public final RuleChain rules = outerRule( fs ).around( directory ).around( pageCacheRule ).around( random ); private final MutableLong key = new MutableLong(); private final MutableLong value = new MutableLong(); @@ -348,9 +348,7 @@ private static GBPTree createIndex( PageCache pageCache private PageCache createPageCache() { - PageSwapperFactory swapper = new SingleFilePageSwapperFactory(); - swapper.setFileSystemAbstraction( fs.get() ); - return new MuninnPageCache( swapper, 4_000, PAGE_SIZE, NULL ); + return pageCacheRule.getPageCache( fs.get() ); } interface Action diff --git a/community/index/src/test/java/org/neo4j/index/gbptree/GBPTreeTest.java b/community/index/src/test/java/org/neo4j/index/gbptree/GBPTreeTest.java index c6560c71eb847..5fcc93e567f90 100644 --- a/community/index/src/test/java/org/neo4j/index/gbptree/GBPTreeTest.java +++ b/community/index/src/test/java/org/neo4j/index/gbptree/GBPTreeTest.java @@ -21,9 +21,10 @@ import org.apache.commons.lang3.mutable.MutableLong; import org.junit.After; +import org.junit.Before; import org.junit.Rule; import org.junit.Test; -import org.junit.rules.TemporaryFolder; +import org.junit.rules.RuleChain; import java.io.File; import java.io.IOException; @@ -39,16 +40,15 @@ import org.neo4j.index.Index; import org.neo4j.index.IndexWriter; import org.neo4j.index.gbptree.GBPTree.Monitor; -import org.neo4j.io.fs.DefaultFileSystemAbstraction; import org.neo4j.io.pagecache.IOLimiter; import org.neo4j.io.pagecache.PageCache; import org.neo4j.io.pagecache.PageCursor; -import org.neo4j.io.pagecache.PageSwapperFactory; import org.neo4j.io.pagecache.PagedFile; -import org.neo4j.io.pagecache.impl.SingleFilePageSwapperFactory; -import org.neo4j.io.pagecache.impl.muninn.MuninnPageCache; import org.neo4j.test.Barrier; +import org.neo4j.test.rule.PageCacheRule; import org.neo4j.test.rule.RandomRule; +import org.neo4j.test.rule.TestDirectory; +import org.neo4j.test.rule.fs.DefaultFileSystemRule; import static org.hamcrest.CoreMatchers.containsString; import static org.junit.Assert.assertEquals; @@ -56,16 +56,22 @@ import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; +import static org.junit.rules.RuleChain.outerRule; + import static org.neo4j.index.gbptree.GBPTree.NO_MONITOR; import static org.neo4j.index.gbptree.ThrowingRunnable.throwing; -import static org.neo4j.io.pagecache.tracing.PageCacheTracer.NULL; +import static org.neo4j.test.rule.PageCacheRule.config; public class GBPTreeTest { + private final DefaultFileSystemRule fs = new DefaultFileSystemRule(); + private final TestDirectory directory = TestDirectory.testDirectory( getClass(), fs.get() ); + private final PageCacheRule pageCacheRule = new PageCacheRule(); + private final RandomRule random = new RandomRule(); + @Rule - public final TemporaryFolder folder = new TemporaryFolder( new File( "target" ) ); - @Rule - public final RandomRule random = new RandomRule(); + public final RuleChain rules = outerRule( fs ).around( directory ).around( pageCacheRule ).around( random ); + private PageCache pageCache; private File indexFile; private final Layout layout = new SimpleLongLayout(); @@ -80,27 +86,29 @@ private GBPTree createIndex( int pageSize ) private GBPTree createIndex( int pageSize, Monitor monitor ) throws IOException { - pageCache = new MuninnPageCache( swapperFactory(), 10_000, pageSize, NULL ); - indexFile = new File( folder.getRoot(), "index" ); + pageCache = createPageCache( pageSize ); return index = new GBPTree<>( pageCache, indexFile, layout, 0/*use whatever page cache says*/, monitor ); } - private static PageSwapperFactory swapperFactory() + private PageCache createPageCache( int pageSize ) { - PageSwapperFactory swapperFactory = new SingleFilePageSwapperFactory(); - swapperFactory.setFileSystemAbstraction( new DefaultFileSystemAbstraction() ); - return swapperFactory; + return pageCacheRule.getPageCache( fs.get(), config().withPageSize( pageSize ) ); + } + + @Before + public void setUpIndexFile() + { + indexFile = directory.file( "index" ); } @After - public void closePageCache() throws IOException + public void closeIndexAndPageCache() throws IOException { if ( index != null ) { assertTrue( index.consistencyCheck() ); index.close(); } - pageCache.close(); } /* Meta and state page tests */ @@ -241,7 +249,7 @@ public void shouldFailOnOpenWithDifferentPageSize() throws Exception // WHEN pageCache.close(); - pageCache = new MuninnPageCache( swapperFactory(), 10_000, pageSize / 2, NULL ); + pageCache = createPageCache( pageSize / 2 ); try ( Index index = new GBPTree<>( pageCache, indexFile, layout, 0, NO_MONITOR ) ) { fail( "Should not load" ); @@ -258,8 +266,7 @@ public void shouldFailOnStartingWithPageSizeLargerThanThatOfPageCache() throws E { // WHEN int pageSize = 512; - pageCache = new MuninnPageCache( swapperFactory(), 10_000, pageSize, NULL ); - indexFile = new File( folder.getRoot(), "index" ); + pageCache = createPageCache( pageSize ); try ( Index index = new GBPTree<>( pageCache, indexFile, layout, pageSize * 2, NO_MONITOR ) ) { @@ -277,8 +284,7 @@ public void shouldMapIndexFileWithProvidedPageSizeIfLessThanOrEqualToCachePageSi { // WHEN int pageSize = 1024; - pageCache = new MuninnPageCache( swapperFactory(), 10_000, pageSize, NULL ); - indexFile = new File( folder.getRoot(), "index" ); + pageCache = createPageCache( pageSize ); try ( Index index = new GBPTree<>( pageCache, indexFile, layout, pageSize / 2, NO_MONITOR ) ) { @@ -291,15 +297,14 @@ public void shouldFailWhenTryingToRemapWithPageSizeLargerThanCachePageSize() thr { // WHEN int pageSize = 1024; - pageCache = new MuninnPageCache( swapperFactory(), 10_000, pageSize, NULL ); - indexFile = new File( folder.getRoot(), "index" ); + pageCache = createPageCache( pageSize ); try ( Index index = new GBPTree<>( pageCache, indexFile, layout, pageSize, NO_MONITOR ) ) { // Good } - pageCache = new MuninnPageCache( swapperFactory(), 10_000, pageSize / 2, NULL ); + pageCache = createPageCache( pageSize / 2 ); try ( GBPTree index = new GBPTree<>( pageCache, indexFile, layout, pageSize, NO_MONITOR ) ) { @@ -317,8 +322,7 @@ public void shouldRemapFileIfMappedWithPageSizeLargerThanCreationSize() throws E { // WHEN int pageSize = 1024; - pageCache = new MuninnPageCache( swapperFactory(), 10_000, pageSize, NULL ); - indexFile = new File( folder.getRoot(), "index" ); + pageCache = createPageCache( pageSize ); List expectedData = new ArrayList<>(); for ( long i = 0; i < 100; i++ ) { @@ -588,7 +592,7 @@ public void checkpointCompleted() private void setFormatVersion( int pageSize, int formatVersion ) throws IOException { try ( PagedFile pagedFile = pageCache.map( indexFile, pageSize ); - PageCursor cursor = pagedFile.io( IdSpace.META_PAGE_ID, PagedFile.PF_SHARED_WRITE_LOCK ) ) + PageCursor cursor = pagedFile.io( IdSpace.META_PAGE_ID, PagedFile.PF_SHARED_WRITE_LOCK ) ) { assertTrue( cursor.next() ); cursor.putInt( formatVersion ); diff --git a/community/index/src/test/java/org/neo4j/index/gbptree/SeekCursorTest.java b/community/index/src/test/java/org/neo4j/index/gbptree/SeekCursorTest.java index 898f91ecd9a01..e2a3661a635df 100644 --- a/community/index/src/test/java/org/neo4j/index/gbptree/SeekCursorTest.java +++ b/community/index/src/test/java/org/neo4j/index/gbptree/SeekCursorTest.java @@ -36,10 +36,13 @@ import org.neo4j.io.pagecache.PageCursor; import org.neo4j.io.pagecache.impl.DelegatingPageCursor; +import static org.hamcrest.CoreMatchers.containsString; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; + import static org.neo4j.index.ValueMergers.overwrite; import static org.neo4j.index.gbptree.GenSafePointerPair.pointer; @@ -1688,6 +1691,62 @@ public void shouldCatchupRootWhenNodeHasTooNewGenerationWhileTraversingLeaves() assertTrue( triggered.getValue() ); } + @Test + public void shouldThrowTreeInconsistencyExceptionOnBadReadWithoutShouldRetryWhileTraversingTree() throws Exception + { + // GIVEN + int keyCount = 10000; + + // WHEN + cursor.setOffset( TreeNode.BYTE_POS_KEYCOUNT ); + cursor.putInt( keyCount ); // Bad key count + + // THEN + try ( SeekCursor seek = seekCursor( 0L, Long.MAX_VALUE ) ) + { + // Do nothing + } + catch ( TreeInconsistencyException e ) + { + assertThat( e.getMessage(), containsString( "keyCount:" + keyCount ) ); + } + } + + @Test + public void shouldThrowTreeInconsistencyExceptionOnBadReadWithoutShouldRetryWhileTraversingLeaves() throws Exception + { + // GIVEN + // a root with two leaves in old generation + int keyCount = 10000; + long i = 0L; + while ( numberOfRootSplits == 0 ) + { + insert( i, i * 10 ); + i++; + } + long rootId = cursor.getCurrentPageId(); + long leftChild = node.childAt( cursor, 0, stableGen, unstableGen ); + + // WHEN + PageCursorUtil.goTo( cursor, "test", GenSafePointerPair.pointer( leftChild ) ); + cursor.setOffset( node.BYTE_POS_KEYCOUNT ); + cursor.putInt( keyCount ); // Bad key count + PageCursorUtil.goTo( cursor, "test", rootId ); + + // THEN + try ( SeekCursor seek = seekCursor( 0L, Long.MAX_VALUE ) ) + { + while ( seek.next() ) + { + // Do nothing + } + } + catch ( TreeInconsistencyException e ) + { + assertThat( e.getMessage(), containsString( "keyCount:" + keyCount ) ); + } + } + private void checkpoint() { stableGen = unstableGen; diff --git a/community/index/src/test/java/org/neo4j/index/gbptree/SimpleLongLayout.java b/community/index/src/test/java/org/neo4j/index/gbptree/SimpleLongLayout.java index b15961917dbef..e0c35a3beb6a5 100644 --- a/community/index/src/test/java/org/neo4j/index/gbptree/SimpleLongLayout.java +++ b/community/index/src/test/java/org/neo4j/index/gbptree/SimpleLongLayout.java @@ -135,20 +135,32 @@ private static void writeString( PageCursor cursor, String string ) public void readMetaData( PageCursor cursor ) { String name = readString( cursor ); + if ( name == null ) + { + return; + } + if ( customNameAsMetaData != null ) { if ( !name.equals( customNameAsMetaData ) ) { - throw new MetadataMismatchException( "Name '" + name + + cursor.setCursorException( "Name '" + name + "' doesn't match expected '" + customNameAsMetaData + "'" ); + return; } } customNameAsMetaData = name; } - private String readString( PageCursor cursor ) + private static String readString( PageCursor cursor ) { int length = cursor.getInt(); + if ( length < 0 || length >= cursor.getCurrentPageSize() ) + { + cursor.setCursorException( "Unexpected length of string " + length ); + return null; + } + byte[] bytes = new byte[length]; cursor.getBytes( bytes ); return new String( bytes, UTF_8 ); 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 3c6a914bc4cfa..51ea751713702 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 @@ -264,7 +264,11 @@ public boolean shouldRetry() throws IOException public int copyTo( int sourceOffset, PageCursor targetCursor, int targetOffset, int lengthInBytes ) { adversary.injectFailure( IndexOutOfBoundsException.class ); - return delegate.copyTo( sourceOffset, targetCursor, targetOffset, lengthInBytes ); + + PageCursor targetCursorDelegate = targetCursor instanceof AdversarialWritePageCursor ? + ((AdversarialWritePageCursor) targetCursor).delegate : targetCursor; + + return delegate.copyTo( sourceOffset, targetCursorDelegate, targetOffset, lengthInBytes ); } @Override