Make the NeoServerRestartTest deterministic

As well as addressing review comments.

community/kernel/src/test/java/org/neo4j/kernel/impl/store/StoreFactoryTest.java

@@ -25,16 +25,10 @@
 import org.junit.Test;
 
 import java.io.File;
-import java.io.FileOutputStream;
-import java.io.FilenameFilter;
 import java.io.IOException;
-import java.nio.channels.FileChannel;
 import java.nio.file.OpenOption;
-import java.nio.file.StandardOpenOption;
-import java.util.Arrays;
 
 import org.neo4j.graphdb.factory.GraphDatabaseSettings;
-import org.neo4j.graphdb.mockfs.EphemeralFileSystemAbstraction;
 import org.neo4j.helpers.collection.MapUtil;
 import org.neo4j.io.fs.FileSystemAbstraction;
 import org.neo4j.io.pagecache.PageCache;
@@ -46,7 +40,6 @@
 import org.neo4j.logging.LogProvider;
 import org.neo4j.logging.NullLogProvider;
 import org.neo4j.test.rule.PageCacheRule;
-import org.neo4j.test.rule.fs.DefaultFileSystemRule;
 import org.neo4j.test.rule.fs.EphemeralFileSystemRule;
 
 import static java.nio.file.StandardOpenOption.DELETE_ON_CLOSE;
@@ -175,7 +168,6 @@ public void shouldCompleteInitializationOfStoresWithIncompleteHeaders() throws E
         StoreFactory storeFactory = storeFactory( Config.empty() );
         storeFactory.openAllNeoStores( true ).close();
         FileSystemAbstraction fs = fsRule.get();
-        File[] files = fs.listFiles( storeDir, ( file, s ) -> !s.endsWith( ".id" ) );
         for ( File f : fs.listFiles( storeDir ) )
         {
             fs.truncate( f, 0 );

community/server/src/main/java/org/neo4j/server/AbstractNeoServer.java

@@ -103,7 +103,7 @@ public abstract class AbstractNeoServer implements NeoServer
     private static final long MINIMUM_TIMEOUT = 1000L;
     /**
      * We add a second to the timeout if the user configures a 1-second timeout.
-     *
+     * <p>
      * This ensures the expiry time displayed to the user is always at least 1 second, even after it is rounded down.
      */
     private static final long ROUNDING_SECOND = 1000L;
@@ -153,16 +153,18 @@ public AbstractNeoServer( Config config, Database.Factory dbFactory,
         this.logProvider = logProvider;
         this.log = logProvider.getLog( getClass() );
 
-        HttpConnector httpConnector = ClientConnectorSettings.httpConnector( config, ClientConnectorSettings.HttpConnector.Encryption.NONE )
-                .orElseThrow( () ->
-                        new IllegalArgumentException( "An HTTP connector must be configured to run the server" ) );
+        HttpConnector httpConnector =
+                ClientConnectorSettings.httpConnector( config, ClientConnectorSettings.HttpConnector.Encryption.NONE )
+                        .orElseThrow( () ->
+                                new IllegalArgumentException(
+                                        "An HTTP connector must be configured to run the server" ) );
         httpListenAddress = config.get( httpConnector.listen_address );
         httpAdvertisedAddress = config.get( httpConnector.advertised_address );
 
         Optional<HttpConnector> httpsConnector =
                 ClientConnectorSettings.httpConnector( config, ClientConnectorSettings.HttpConnector.Encryption.TLS );
-        httpsListenAddress = httpsConnector.map( (connector) -> config.get( connector.listen_address ) );
-        httpsAdvertisedAddress = httpsConnector.map( (connector) -> config.get( connector.advertised_address ) );
+        httpsListenAddress = httpsConnector.map( ( connector ) -> config.get( connector.listen_address ) );
+        httpsAdvertisedAddress = httpsConnector.map( ( connector ) -> config.get( connector.advertised_address ) );
     }
 
     @Override
@@ -173,7 +175,8 @@ public void init()
             return;
         }
 
-        this.database = life.add( dependencyResolver.satisfyDependency(dbFactory.newDatabase( config, dependencies)) );
+        this.database =
+                life.add( dependencyResolver.satisfyDependency( dbFactory.newDatabase( config, dependencies ) ) );
 
         this.authManagerSupplier = dependencyResolver.provideDependency( AuthManager.class );
         this.userManagerSupplier = dependencyResolver.provideDependency( UserManagerSupplier.class );
@@ -187,6 +190,9 @@ public void init()
             registerModule( moduleClass );
         }
 
+        serverComponents = new ServerComponentsLifecycleAdapter();
+        life.add( serverComponents );
+
         this.initialized = true;
     }
 
@@ -196,41 +202,6 @@ public void start() throws ServerStartupException
         init();
         try
         {
-            serverComponents = new LifecycleAdapter()
-            {
-                @Override
-                public void start()
-                        throws Throwable
-                {
-                    DiagnosticsManager diagnosticsManager = resolveDependency( DiagnosticsManager.class );
-                    Log diagnosticsLog = diagnosticsManager.getTargetLog();
-                    diagnosticsLog.info( "--- SERVER STARTED START ---" );
-                    databaseActions = createDatabaseActions();
-
-                    transactionFacade = createTransactionalActions();
-
-                    cypherExecutor = new CypherExecutor( database, logProvider );
-
-                    configureWebServer();
-
-                    cypherExecutor.start();
-
-                    startModules();
-
-                    startWebServer();
-
-                    diagnosticsLog.info( "--- SERVER STARTED END ---" );
-                }
-
-                @Override
-                public void stop()
-                        throws Throwable
-                {
-                    stopWebServer();
-                    stopModules();
-                }
-            };
-            life.add( serverComponents );
             life.start();
 
         }
@@ -261,7 +232,7 @@ private TransactionFacade createTransactionalActions()
         final Clock clock = Clocks.systemClock();
 
         transactionRegistry =
-            new TransactionHandleRegistry( clock, timeoutMillis, logProvider );
+                new TransactionHandleRegistry( clock, timeoutMillis, logProvider );
 
         // ensure that this is > 0
         long runEvery = round( timeoutMillis / 2.0 );
@@ -275,7 +246,8 @@ private TransactionFacade createTransactionalActions()
         return new TransactionFacade(
                 new TransitionalPeriodTransactionMessContainer( database.getGraph() ),
                 dependencyResolver.resolveDependency( QueryExecutionEngine.class ),
-                dependencyResolver.resolveDependency( GraphDatabaseQueryService.class ), transactionRegistry, logProvider
+                dependencyResolver.resolveDependency( GraphDatabaseQueryService.class ), transactionRegistry,
+                logProvider
         );
     }
 
@@ -348,7 +320,8 @@ private void startWebServer() throws Exception
         }
     }
 
-    private void setUpHttpLogging() throws IOException {
+    private void setUpHttpLogging() throws IOException
+    {
         if ( !getConfig().get( http_logging_enabled ) )
         {
             return;
@@ -392,15 +365,17 @@ protected Optional<KeyStoreInformation> createKeyStore()
                     //noinspection deprecation
                     log.info( "No SSL certificate found, generating a self-signed certificate.." );
                     Certificates certFactory = new Certificates();
-                    certFactory.createSelfSignedCertificate( certificatePath, privateKeyPath, httpListenAddress.getHostname() );
+                    certFactory.createSelfSignedCertificate( certificatePath, privateKeyPath,
+                            httpListenAddress.getHostname() );
                 }
 
                 // Make sure both files were there, or were generated
                 if ( !certificatePath.exists() )
                 {
                     throw new ServerStartupException(
                             String.format(
-                                    "TLS private key found, but missing certificate at '%s'. Cannot start server without certificate.",
+                                    "TLS private key found, but missing certificate at '%s'. Cannot start server " +
+                                    "without certificate.",
 
                                     certificatePath ) );
                 }
@@ -435,7 +410,6 @@ protected Optional<KeyStoreInformation> createKeyStore()
     public void stop()
     {
         life.stop();
-        life.remove( serverComponents );
     }
 
     private void stopWebServer()
@@ -534,7 +508,7 @@ private boolean hasModule( Class<? extends ServerModule> clazz )
         return false;
     }
 
-    @SuppressWarnings("unchecked")
+    @SuppressWarnings( "unchecked" )
     private <T extends ServerModule> T getModule( Class<T> clazz )
     {
         for ( ServerModule sm : serverModules )
@@ -562,4 +536,38 @@ public DependencyResolver get()
             return db.getGraph().getDependencyResolver();
         }
     } );
+
+    private class ServerComponentsLifecycleAdapter extends LifecycleAdapter
+    {
+        @Override
+        public void start() throws Throwable
+        {
+            DiagnosticsManager diagnosticsManager = resolveDependency( DiagnosticsManager.class );
+            Log diagnosticsLog = diagnosticsManager.getTargetLog();
+            diagnosticsLog.info( "--- SERVER STARTED START ---" );
+            databaseActions = createDatabaseActions();
+
+            transactionFacade = createTransactionalActions();
+
+            cypherExecutor = new CypherExecutor( database, logProvider );
+
+            configureWebServer();
+
+            cypherExecutor.start();
+
+            startModules();
+
+            startWebServer();
+
+            diagnosticsLog.info( "--- SERVER STARTED END ---" );
+        }
+
+        @Override
+        public void stop()
+                throws Throwable
+        {
+            stopWebServer();
+            stopModules();
+        }
+    }
 }

community/server/src/main/java/org/neo4j/server/ServerBootstrapper.java

@@ -49,7 +49,7 @@ public abstract class ServerBootstrapper implements Bootstrapper
     public static final int WEB_SERVER_STARTUP_ERROR_CODE = 1;
     public static final int GRAPH_DATABASE_STARTUP_ERROR_CODE = 2;
 
-    private NeoServer server;
+    private volatile NeoServer server;
     private Thread shutdownHook;
     private GraphDatabaseDependencies dependencies = GraphDatabaseDependencies.newDependencies();
     // in case we have errors loading/validating the configuration log to stdout

community/server/src/test/java/org/neo4j/server/NeoServerRestartTest.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2002-2016 "Neo Technology,"
+ * Copyright (c) 2002-2017 "Neo Technology,"
  * Network Engine for Objects in Lund AB [http://neotechnology.com]
  *
  * This file is part of Neo4j.
@@ -21,48 +21,99 @@
 
 import org.junit.Test;
 
+import java.io.File;
 import java.io.IOException;
-import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.Semaphore;
+import java.util.concurrent.atomic.AtomicBoolean;
 
-import org.neo4j.server.helpers.CommunityServerBuilder;
+import org.neo4j.io.pagecache.PageEvictionCallback;
+import org.neo4j.io.pagecache.PageSwapper;
+import org.neo4j.io.pagecache.impl.SingleFilePageSwapperFactory;
 import org.neo4j.test.ThreadTestUtils;
 
+import static org.junit.Assert.fail;
+
 public abstract class NeoServerRestartTest
 {
+    public static final String CUSTOM_SWAPPER = "CustomSwapper";
+    private static Semaphore semaphore;
+
+    static
+    {
+        semaphore = new Semaphore( 0 );
+    }
+
+    /**
+     * This test makes sure that the database is able to start after having been stopped during initialization.
+     *
+     * In order to make sure that the server is stopped during startup we create a separate thread that calls stop.
+     * In order to make sure that this thread does not call stop before the startup procedure has started we use a
+     * custom implementation of a PageSwapperFactory, which communicates with the thread that calls stop. We do this
+     * via a static semaphore.
+     * @throws IOException
+     * @throws InterruptedException
+     */
 
     @Test
-    public void shouldNotCorruptWhenImmediatelyStopped() throws IOException, InterruptedException
+    public void shouldBeAbleToRestartWhenStoppedDuringStartup() throws IOException, InterruptedException
     {
-        NeoServer server = getNeoServer();
+        // Make sure that the semaphore is in a clean state.
+        semaphore.drainPermits();
+        // Get a server that uses our custom swapper.
+        NeoServer server = getNeoServer( CUSTOM_SWAPPER );
 
-        CountDownLatch latch = new CountDownLatch( 1 );
-        ThreadTestUtils.fork( $waitThenStopServer( server, latch ) );
+        AtomicBoolean failure = new AtomicBoolean();
+        Thread serverStoppingThread = ThreadTestUtils.fork( stopServerAfterStartingHasStarted( server, failure ) );
         server.start();
-        //Wait for the server to stop.
-        latch.await();
-
-        server = getNeoServer();
+        // Wait for the server to stop.
+        serverStoppingThread.join();
+        // Check if the server stopped successfully.
+        if ( failure.get() )
+        {
+            fail( "Server failed to stop." );
+        }
+        // Verify that we can start the server again.
+        server = getNeoServer( CUSTOM_SWAPPER );
         server.start();
         server.stop();
     }
 
-    protected abstract NeoServer getNeoServer() throws IOException;
+    protected abstract NeoServer getNeoServer( String customPageSwapperName ) throws IOException;
 
-    private Runnable $waitThenStopServer( NeoServer server, CountDownLatch latch )
+    private Runnable stopServerAfterStartingHasStarted( NeoServer server, AtomicBoolean failure )
     {
-        return () -> {
+        return () ->
+        {
             try
             {
-                //Make sure that we have started starting the database. This value is empirically chosen and may need
-                // to be revisited in the future.
-                Thread.sleep( 200 );
+                // Make sure that we have started the startup procedure before calling stop.
+                semaphore.acquire();
                 server.stop();
-                latch.countDown();
             }
             catch ( Exception e )
             {
-                //This is ok, it is not what we are testing for.
+                failure.set( true );
+                e.printStackTrace();
             }
         };
     }
+
+    // This class is used to notify the test that the server has started its startup procedure.
+    public static class CustomSwapper extends SingleFilePageSwapperFactory
+    {
+        @Override
+        public String implementationName()
+        {
+            return CUSTOM_SWAPPER;
+        }
+
+        @Override
+        public PageSwapper createPageSwapper( File file, int filePageSize, PageEvictionCallback onEviction,
+                boolean createIfNotExist ) throws IOException
+        {
+            // This will be called early in the startup sequence. Notifies that we can call stop on the server.
+            semaphore.release();
+            return super.createPageSwapper( file, filePageSize, onEviction, createIfNotExist );
+        }
+    }
 }

community/server/src/test/java/org/neo4j/server/NeoServerRestartTestCommunity.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2002-2016 "Neo Technology,"
+ * Copyright (c) 2002-2017 "Neo Technology,"
  * Network Engine for Objects in Lund AB [http://neotechnology.com]
  *
  * This file is part of Neo4j.
@@ -21,13 +21,15 @@
 
 import java.io.IOException;
 
+import org.neo4j.graphdb.factory.GraphDatabaseSettings;
 import org.neo4j.server.helpers.CommunityServerBuilder;
 
 public class NeoServerRestartTestCommunity extends NeoServerRestartTest
 {
-    protected NeoServer getNeoServer() throws IOException
+    protected NeoServer getNeoServer( String customPageSwapperName ) throws IOException
     {
-        CommunityServerBuilder builder = CommunityServerBuilder.server();
+        CommunityServerBuilder builder = CommunityServerBuilder.server().withProperty( GraphDatabaseSettings
+                .pagecache_swapper.name(), customPageSwapperName );
         return builder.build();
     }
 }

community/server/src/test/resources/META-INF/services/org.neo4j.io.pagecache.PageSwapperFactory

@@ -0,0 +1 @@
+org.neo4j.server.NeoServerRestartTest$CustomSwapper