Skip to content

Commit

Permalink
More testing for opening a partially created GBPTree
Browse files Browse the repository at this point in the history
This uncovered paths that weren't guarded when opening a GBPTree normally
via its constructor (previous testing was only for its static readHeader method).
Also added GBPTreePartialCreateFuzzIT that, through randomized testing,
has the ability to uncover even more cases in the future.
  • Loading branch information
tinwelint committed Jan 17, 2018
1 parent 990597f commit 8f2acbd
Show file tree
Hide file tree
Showing 3 changed files with 205 additions and 30 deletions.
Expand Up @@ -44,6 +44,7 @@

import static java.lang.String.format;

import static org.neo4j.helpers.Exceptions.withMessage;
import static org.neo4j.index.internal.gbptree.Generation.generation;
import static org.neo4j.index.internal.gbptree.Generation.stableGeneration;
import static org.neo4j.index.internal.gbptree.Generation.unstableGeneration;
Expand Down Expand Up @@ -420,6 +421,7 @@ public GBPTree( PageCache pageCache, File indexFile, Layout<KEY,VALUE> layout, i
setRoot( rootId, Generation.unstableGeneration( generation ) );
this.layout = layout;

boolean success = false;
try
{
this.pagedFile = openOrCreate( pageCache, indexFile, tentativePageSize, layout );
Expand Down Expand Up @@ -447,19 +449,19 @@ public GBPTree( PageCache pageCache, File indexFile, Layout<KEY,VALUE> layout, i
forceState();
cleaning = createCleanupJob( needsCleaning );
recoveryCleanupWorkCollector.add( cleaning );
success = true;
}
catch ( Throwable t )
{
appendTreeInformation( t );
try
throw t;
}
finally
{
if ( !success )
{
close();
}
catch ( Throwable e )
{
t.addSuppressed( e );
}
throw t;
}
}

Expand Down Expand Up @@ -511,23 +513,24 @@ private static <KEY, VALUE> PagedFile openExistingIndexFile( PageCache pageCache
PagedFile pagedFile = pageCache.map( indexFile, pageCache.pageSize() );
// This index already exists, verify meta data aligns with expectations

boolean success = false;
try
{
int pageSize = readMeta( layout, pagedFile );
pagedFile = mapWithCorrectPageSize( pageCache, indexFile, pagedFile, pageSize );
success = true;
return pagedFile;
}
catch ( Throwable t )
catch ( IllegalStateException e )
{
try
throw new IOException( "Index is not fully initialized since it's missing the meta page", e );
}
finally
{
if ( !success )
{
pagedFile.close();
}
catch ( IOException e )
{
t.addSuppressed( e );
}
throw t;
}
}

Expand All @@ -552,7 +555,7 @@ private PagedFile createNewIndexFile( PageCache pageCache, File indexFile, int p

private void loadState( PagedFile pagedFile, Header.Reader headerReader ) throws IOException
{
Pair<TreeState,TreeState> states = readStatePages( pagedFile );
Pair<TreeState,TreeState> states = loadStatePages( pagedFile );
TreeState state = TreeStatePair.selectNewestValidState( states );
try ( PageCursor cursor = pagedFile.io( state.pageId(), PagedFile.PF_SHARED_READ_LOCK ) )
{
Expand Down Expand Up @@ -586,25 +589,21 @@ public static void readHeader( PageCache pageCache, File indexFile, Layout<?,?>
{
try ( PagedFile pagedFile = openExistingIndexFile( pageCache, indexFile, layout ) )
{
Pair<TreeState,TreeState> states = readStatePages( pagedFile );
if ( states.getLeft().isEmpty() && states.getRight().isEmpty() )
{
throw new IllegalStateException( "Both state pages are empty" );
}

Pair<TreeState,TreeState> states = loadStatePages( pagedFile );
TreeState state = TreeStatePair.selectNewestValidState( states );
try ( PageCursor cursor = pagedFile.io( state.pageId(), PagedFile.PF_SHARED_READ_LOCK ) )
{
PageCursorUtil.goTo( cursor, "header data", state.pageId() );
doReadHeader( headerReader, cursor );
}
}
catch ( IllegalStateException e )
catch ( Throwable t )
{
// Thrown from PageCursorUtil.goTo, meaning that this index hasn't even been properly initialized
// and so should be treated as non-existent
throw new IOException( format( "GBPTree[file:%s, layout:%s] seems to not be fully initialized since it's missing meta pages",
indexFile, layout ), e );
// Decorate outgoing exceptions with basic tree information. This is similar to how the constructor
// appends its information, but the constructor has read more information at that point so this one
// is a bit more sparse on information.
withMessage( t, t.getMessage() + " | " + format( "GBPTree[file:%s, layout:%s]", indexFile, layout ) );
throw t;
}
}

Expand Down Expand Up @@ -688,6 +687,31 @@ private static TreeState other( Pair<TreeState,TreeState> states, TreeState stat
return states.getLeft() == state ? states.getRight() : states.getLeft();
}

/**
* Basically {@link #readStatePages(PagedFile)} with some more checks, suitable for when first opening an index file,
* not while running it and check pointing.
*
* @param pagedFile {@link PagedFile} to read the state pages from.
* @return both read state pages.
* @throws IOException if state pages are missing (file is smaller than that) or if they are both empty.
*/
private static Pair<TreeState,TreeState> loadStatePages( PagedFile pagedFile ) throws IOException
{
try
{
Pair<TreeState,TreeState> states = readStatePages( pagedFile );
if ( states.getLeft().isEmpty() && states.getRight().isEmpty() )
{
throw new IOException( "Index is not fully initialized since its state pages are empty" );
}
return states;
}
catch ( IllegalStateException e )
{
throw new IOException( "Index is not fully initialized since it's missing state pages", e );
}
}

private static Pair<TreeState,TreeState> readStatePages( PagedFile pagedFile ) throws IOException
{
Pair<TreeState,TreeState> states;
Expand Down Expand Up @@ -959,7 +983,7 @@ public void close() throws IOException

private void internalIndexClose() throws IOException
{
if ( !changesSinceLastCheckpoint && !cleaning.needed() )
if ( cleaning != null && !changesSinceLastCheckpoint && !cleaning.needed() )
{
clean = true;
forceState();
Expand Down
@@ -0,0 +1,115 @@
/*
* Copyright (c) 2002-2018 "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 <http://www.gnu.org/licenses/>.
*/
package org.neo4j.index.internal.gbptree;

import org.junit.Rule;
import org.junit.Test;

import java.io.File;
import java.io.IOException;
import java.util.concurrent.ThreadLocalRandom;

import org.neo4j.io.fs.DefaultFileSystemAbstraction;
import org.neo4j.io.fs.FileSystemAbstraction;
import org.neo4j.io.pagecache.PageCache;
import org.neo4j.io.pagecache.impl.SingleFilePageSwapperFactory;
import org.neo4j.io.pagecache.impl.muninn.MuninnPageCache;
import org.neo4j.io.pagecache.tracing.PageCacheTracer;
import org.neo4j.io.pagecache.tracing.cursor.PageCursorTracerSupplier;
import org.neo4j.test.rule.PageCacheAndDependenciesRule;
import org.neo4j.test.rule.fs.DefaultFileSystemRule;

import static org.junit.Assert.assertNotEquals;

import static java.lang.ProcessBuilder.Redirect.INHERIT;
import static java.util.Arrays.asList;
import static org.neo4j.graphdb.config.Configuration.EMPTY;
import static org.neo4j.index.internal.gbptree.GBPTree.NO_HEADER_READER;
import static org.neo4j.io.ByteUnit.kibiBytes;

/**
* Tests functionality around process crashing, or similar, when having started, but not completed creation of an index file,
* i.e. opening an index file for the first time.
*
* This test is an asset in finding potentially new issues regarding partially created index files over time.
* It will not guarantee, in one run, that every case has been covered. There are other specific test cases for that.
* When this test finds a new issue that should be encoded into a proper unit test in {@link GBPTreeTest} or similar.
*/
public class GBPTreePartialCreateFuzzIT
{
@Rule
public final PageCacheAndDependenciesRule storage = new PageCacheAndDependenciesRule( DefaultFileSystemRule::new, getClass() );

@Test
public void shouldDetectAndThrowIOExceptionOnPartiallyCreatedFile() throws Exception
{
// given a crashed-on-open index
File file = storage.directory().file( "index" );
Process process = new ProcessBuilder( asList( "java", "-cp", System.getProperty( "java.class.path" ), getClass().getName(),
file.getAbsolutePath() ) ).redirectError( INHERIT ).redirectOutput( INHERIT ).start();
Thread.sleep( ThreadLocalRandom.current().nextInt( 1_000 ) );
int exitCode = process.destroyForcibly().waitFor();

// then reading it should either work or throw IOException
try ( PageCache pageCache = storage.pageCache() )
{
SimpleLongLayout layout = new SimpleLongLayout();

// check readHeader
try
{
GBPTree.readHeader( pageCache, file, layout, NO_HEADER_READER );
}
catch ( IOException e )
{
// It's OK if the process was destroyed
assertNotEquals( 0, exitCode );
}

// check constructor
try
{
new GBPTreeBuilder<>( pageCache, file, layout ).build().close();
}
catch ( IOException e )
{
// It's OK if the process was destroyed
assertNotEquals( 0, exitCode );
}
}
}

public static void main( String[] args ) throws IOException
{
// Just start and immediately close. The process spawning this subprocess will kill it in the middle of all this
File file = new File( args[0] );
try ( FileSystemAbstraction fs = new DefaultFileSystemAbstraction() )
{
SingleFilePageSwapperFactory swapper = new SingleFilePageSwapperFactory();
swapper.open( fs, EMPTY );
try ( PageCache pageCache = new MuninnPageCache( swapper, 10, (int) kibiBytes( 8 ),
PageCacheTracer.NULL, PageCursorTracerSupplier.NULL ) )
{
fs.deleteFile( file );
new GBPTreeBuilder<>( pageCache, file, new SimpleLongLayout() ).build().close();
}
}
}
}
Expand Up @@ -56,6 +56,7 @@
import org.neo4j.collection.primitive.PrimitiveLongCollections;
import org.neo4j.collection.primitive.PrimitiveLongSet;
import org.neo4j.cursor.RawCursor;
import org.neo4j.function.ThrowingConsumer;
import org.neo4j.graphdb.mockfs.EphemeralFileSystemAbstraction;
import org.neo4j.index.internal.gbptree.GBPTree.Monitor;
import org.neo4j.io.pagecache.DelegatingPageCache;
Expand Down Expand Up @@ -687,7 +688,18 @@ public void readHeaderMustThrowIfFileDoesNotExist() throws Exception
}

@Test
public void readHeaderMustThrowIOExceptionIfFileIsEmpty() throws Exception
public void openWithReadHeaderMustThrowIOExceptionIfFileIsEmpty() throws Exception
{
openMustThrowIOExceptionIfFileIsEmpty( pageCache -> GBPTree.readHeader( pageCache, indexFile, layout, NO_HEADER_READER ) );
}

@Test
public void openWithConstructorMustThrowIOExceptionIfFileIsEmpty() throws Exception
{
openMustThrowIOExceptionIfFileIsEmpty( pageCache -> index( pageCache ).build() );
}

private void openMustThrowIOExceptionIfFileIsEmpty( ThrowingConsumer<PageCache,IOException> opener ) throws Exception
{
// given an existing empty file
PageCache pageCache = createPageCache( DEFAULT_PAGE_SIZE );
Expand All @@ -696,7 +708,7 @@ public void readHeaderMustThrowIOExceptionIfFileIsEmpty() throws Exception
// when
try
{
GBPTree.readHeader( pageCache, indexFile, layout, NO_HEADER_READER );
opener.accept( pageCache );
fail( "Should've thrown IOException" );
}
catch ( IOException e )
Expand All @@ -707,6 +719,18 @@ public void readHeaderMustThrowIOExceptionIfFileIsEmpty() throws Exception

@Test
public void readHeaderMustThrowIOExceptionIfSomeMetaPageIsMissing() throws Exception
{
openMustThrowIOExceptionIfSomeMetaPageIsMissing(
pageCache -> GBPTree.readHeader( pageCache, indexFile, layout, NO_HEADER_READER ) );
}

@Test
public void constructorMustThrowIOExceptionIfSomeMetaPageIsMissing() throws Exception
{
openMustThrowIOExceptionIfSomeMetaPageIsMissing( pageCache -> index( pageCache ).build() );
}

private void openMustThrowIOExceptionIfSomeMetaPageIsMissing( ThrowingConsumer<PageCache,IOException> opener ) throws Exception
{
// given an existing index with only the first page in it
PageCache pageCache = createPageCache( DEFAULT_PAGE_SIZE );
Expand All @@ -718,7 +742,7 @@ public void readHeaderMustThrowIOExceptionIfSomeMetaPageIsMissing() throws Excep
// when
try
{
GBPTree.readHeader( pageCache, indexFile, layout, NO_HEADER_READER );
opener.accept( pageCache );
fail( "Should've thrown IOException" );
}
catch ( IOException e )
Expand All @@ -729,6 +753,18 @@ public void readHeaderMustThrowIOExceptionIfSomeMetaPageIsMissing() throws Excep

@Test
public void readHeaderMustThrowIOExceptionIfStatePagesAreAllZeros() throws Exception
{
openMustThrowIOExceptionIfStatePagesAreAllZeros(
pageCache -> GBPTree.readHeader( pageCache, indexFile, layout, NO_HEADER_READER ) );
}

@Test
public void constructorMustThrowIOExceptionIfStatePagesAreAllZeros() throws Exception
{
openMustThrowIOExceptionIfStatePagesAreAllZeros( pageCache -> index( pageCache ).build() );
}

private void openMustThrowIOExceptionIfStatePagesAreAllZeros( ThrowingConsumer<PageCache,IOException> opener ) throws Exception
{
// given an existing index with all-zero state pages
PageCache pageCache = createPageCache( DEFAULT_PAGE_SIZE );
Expand All @@ -746,7 +782,7 @@ public void readHeaderMustThrowIOExceptionIfStatePagesAreAllZeros() throws Excep
// when
try
{
GBPTree.readHeader( pageCache, indexFile, layout, NO_HEADER_READER );
opener.accept( pageCache );
fail( "Should've thrown IOException" );
}
catch ( IOException e )
Expand Down

0 comments on commit 8f2acbd

Please sign in to comment.