Make StoreLocker Closeable

neo4j · Oct 20, 2016 · 98a668b · 98a668b
1 parent 0e6c0bf
commit 98a668b
Show file tree

Hide file tree

Showing 10 changed files with 77 additions and 114 deletions.
diff --git a/community/dbms/src/main/java/org/neo4j/commandline/dbms/DumpCommand.java b/community/dbms/src/main/java/org/neo4j/commandline/dbms/DumpCommand.java
@@ -39,13 +39,11 @@
 import org.neo4j.graphdb.factory.GraphDatabaseSettings;
 import org.neo4j.helpers.Args;
 import org.neo4j.kernel.StoreLockException;
-import org.neo4j.kernel.impl.transaction.command.Command;
 import org.neo4j.kernel.internal.StoreLocker;
 import org.neo4j.server.configuration.ConfigLoader;
 
 import static java.lang.String.format;
 import static java.util.Arrays.asList;
-
 import static org.neo4j.dbms.DatabaseManagementSystemSettings.database_path;
 import static org.neo4j.helpers.collection.MapUtil.stringMap;
 import static org.neo4j.kernel.impl.util.Converters.identity;
@@ -117,12 +115,13 @@ public void execute( String[] args ) throws IncorrectUsage, CommandFailed
         }
         catch ( CannotWriteException e )
         {
-            throw new CommandFailed( "you do not have permission to dump the database -- is Neo4j running as a " +
+            throw new CommandFailed(
-                    "different user?", e );
+                    "you do not have permission to dump the database -- is Neo4j running as a " + "different user?",
+                    e );
         }
     }
 
-    private <T> T parse( String[] args, String argument, Function<String, T> converter ) throws IncorrectUsage
+    private <T> T parse( String[] args, String argument, Function<String,T> converter ) throws IncorrectUsage
     {
         try
         {
@@ -138,8 +137,7 @@ private Path toDatabaseDirectory( String databaseName )
     {
         //noinspection unchecked
         return new ConfigLoader( asList( DatabaseManagementSystemSettings.class, GraphDatabaseSettings.class ) )
-                .loadOfflineConfig(
+                .loadOfflineConfig( Optional.of( homeDir.toFile() ),
-                        Optional.of( homeDir.toFile() ),
                         Optional.of( configDir.resolve( "neo4j.conf" ).toFile() ) )
                 .with( stringMap( DatabaseManagementSystemSettings.active_database.name(), databaseName ) )
                 .get( database_path ).toPath();

diff --git a/community/dbms/src/main/java/org/neo4j/commandline/dbms/StoreLockChecker.java b/community/dbms/src/main/java/org/neo4j/commandline/dbms/StoreLockChecker.java
@@ -40,7 +40,7 @@ Closeable withLock( Path databaseDirectory ) throws CommandFailed, CannotWriteEx
 
                 storeLocker.checkLock( databaseDirectory.toFile() );
 
-                return storeLocker::release;
+                return storeLocker;
             }
             else
             {

diff --git a/community/dbms/src/main/java/org/neo4j/commandline/dbms/Util.java b/community/dbms/src/main/java/org/neo4j/commandline/dbms/Util.java
@@ -33,11 +33,9 @@ public class Util
 {
     public static void checkLock( Path databaseDirectory ) throws CommandFailed
     {
-        try
+        try ( StoreLocker storeLocker = new StoreLocker( new DefaultFileSystemAbstraction() ) )
         {
-            StoreLocker storeLocker = new StoreLocker( new DefaultFileSystemAbstraction() );
             storeLocker.checkLock( databaseDirectory.toFile() );
-            storeLocker.release();
         }
         catch ( StoreLockException e )
         {

diff --git a/community/dbms/src/test/java/org/neo4j/commandline/dbms/DumpCommandTest.java b/community/dbms/src/test/java/org/neo4j/commandline/dbms/DumpCommandTest.java
@@ -19,6 +19,11 @@
  */
 package org.neo4j.commandline.dbms;
 
+import org.apache.commons.lang3.SystemUtils;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+
 import java.io.Closeable;
 import java.io.File;
 import java.io.IOException;
@@ -30,11 +35,6 @@
 import java.nio.file.Paths;
 import java.util.function.Predicate;
 
-import org.apache.commons.lang3.SystemUtils;
-import org.junit.Before;
-import org.junit.Rule;
-import org.junit.Test;
-
 import org.neo4j.commandline.admin.CommandFailed;
 import org.neo4j.commandline.admin.IncorrectUsage;
 import org.neo4j.dbms.archive.Dumper;
@@ -45,7 +45,6 @@
 import static java.lang.String.format;
 import static java.util.Arrays.asList;
 import static java.util.Collections.emptySet;
-
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.is;
 import static org.junit.Assert.assertThat;
@@ -57,7 +56,6 @@
 import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
-
 import static org.neo4j.dbms.DatabaseManagementSystemSettings.data_directory;
 import static org.neo4j.dbms.archive.TestUtils.withPermissions;
 
@@ -122,22 +120,18 @@ public void shouldRespectTheStoreLock() throws IOException, IncorrectUsage, Comm
     {
         Path databaseDirectory = homeDir.resolve( "data/databases/foo.db" );
         Files.createDirectories( databaseDirectory );
-        StoreLocker storeLocker = new StoreLocker( new DefaultFileSystemAbstraction() );
-        storeLocker.checkLock( databaseDirectory.toFile() );
 
-        try
+        try ( StoreLocker storeLocker = new StoreLocker( new DefaultFileSystemAbstraction() ) )
         {
+            storeLocker.checkLock( databaseDirectory.toFile() );
+
             execute( "foo.db" );
             fail( "expected exception" );
         }
         catch ( CommandFailed e )
         {
             assertThat( e.getMessage(), equalTo( "the database is in use -- stop Neo4j and try again" ) );
         }
-        finally
-        {
-            storeLocker.release();
-        }
     }
 
     @Test
@@ -184,30 +178,28 @@ public void shouldNotAccidentallyCreateTheDatabaseDirectoryAsASideEffectOfStoreL
     }
 
     @Test
-    public void shouldReportAHelpfulErrorIfWeDontHaveWritePermissionsForLock()
+    public void shouldReportAHelpfulErrorIfWeDontHaveWritePermissionsForLock() throws IOException, IncorrectUsage
-            throws IOException, IncorrectUsage
     {
         assumeFalse( "We haven't found a way to reliably tests permissions on Windows", SystemUtils.IS_OS_WINDOWS );
 
         Path databaseDirectory = homeDir.resolve( "data/databases/foo.db" );
         Files.createDirectories( databaseDirectory );
-        StoreLocker storeLocker = new StoreLocker( new DefaultFileSystemAbstraction() );
-        storeLocker.checkLock( databaseDirectory.toFile() );
 
-        try ( Closeable ignored =
+        try ( StoreLocker storeLocker = new StoreLocker( new DefaultFileSystemAbstraction() ) )
-                      withPermissions( databaseDirectory.resolve( StoreLocker.STORE_LOCK_FILENAME ), emptySet() ) )
         {
-            execute( "foo.db" );
+            storeLocker.checkLock( databaseDirectory.toFile() );
-            fail( "expected exception" );
+
-        }
+            try ( Closeable ignored = withPermissions( databaseDirectory.resolve( StoreLocker.STORE_LOCK_FILENAME ),
-        catch ( CommandFailed e )
+                    emptySet() ) )
-        {
+            {
-            assertThat( e.getMessage(), equalTo( "you do not have permission to dump the database -- is Neo4j " +
+                execute( "foo.db" );
-                    "running as a different user?" ) );
+                fail( "expected exception" );
-        }
+            }
-        finally
+            catch ( CommandFailed e )
-        {
+            {
-            storeLocker.release();
+                assertThat( e.getMessage(), equalTo( "you do not have permission to dump the database -- is Neo4j " +
+                        "running as a different user?" ) );
+            }
         }
     }
 
@@ -273,8 +265,8 @@ public void shouldGiveAClearErrorIfTheArchiveAlreadyExists() throws IOException,
     @Test
     public void shouldGiveAClearMessageIfTheDatabaseDoesntExist() throws IOException, IncorrectUsage
     {
-        doThrow( new NoSuchFileException( homeDir.resolve( "data/databases/foo.db" ).toString() ) )
+        doThrow( new NoSuchFileException( homeDir.resolve( "data/databases/foo.db" ).toString() ) ).when( dumper )
-                .when( dumper ).dump( any(), any(), any() );
+                .dump( any(), any(), any() );
         try
         {
             execute( "foo.db" );
@@ -303,8 +295,7 @@ public void shouldGiveAClearMessageIfTheArchivesParentDoesntExist() throws IOExc
     }
 
     @Test
-    public void
+    public void shouldWrapIOExceptionsCarefulllyBecauseCriticalInformationIsOftenEncodedInTheirNameButMissingFromTheirMessage()
-    shouldWrapIOExceptionsCarefulllyBecauseCriticalInformationIsOftenEncodedInTheirNameButMissingFromTheirMessage()
             throws IOException, IncorrectUsage
     {
         doThrow( new IOException( "the-message" ) ).when( dumper ).dump( any(), any(), any() );
@@ -327,8 +318,9 @@ private void execute( final String database ) throws IncorrectUsage, CommandFail
 
     private void assertCanLockStore( Path databaseDirectory ) throws IOException
     {
-        StoreLocker storeLocker = new StoreLocker( new DefaultFileSystemAbstraction() );
+        try ( StoreLocker storeLocker = new StoreLocker( new DefaultFileSystemAbstraction() ) )
-        storeLocker.checkLock( databaseDirectory.toFile() );
+        {
-        storeLocker.release();
+            storeLocker.checkLock( databaseDirectory.toFile() );
+        }
     }
 }
diff --git a/community/kernel/src/main/java/org/neo4j/kernel/internal/StoreLocker.java b/community/kernel/src/main/java/org/neo4j/kernel/internal/StoreLocker.java
@@ -19,6 +19,7 @@
  */
 package org.neo4j.kernel.internal;
 
+import java.io.Closeable;
 import java.io.File;
 import java.io.IOException;
 import java.nio.channels.FileLock;
@@ -28,7 +29,7 @@
 import org.neo4j.io.fs.StoreChannel;
 import org.neo4j.kernel.StoreLockException;
 
-public class StoreLocker
+public class StoreLocker implements Closeable
 {
     public static final String STORE_LOCK_FILENAME = "store_lock";
 
@@ -46,6 +47,9 @@ public StoreLocker( FileSystemAbstraction fileSystemAbstraction )
      * Obtains lock on store file so that we can ensure the store is not shared between database instances
      * <p>
      * Creates store dir if necessary, creates store lock file if necessary
+     * <p>
+     * Please note that this lock is only valid for as long the {@link #storeLockFileChannel} lives, so make sure the
+     * lock cannot be garbage collected as long as the lock should be valid.
      *
      * @throws StoreLockException if lock could not be acquired
      */
@@ -86,11 +90,12 @@ public void checkLock( File storeDir ) throws StoreLockException
     private StoreLockException storeLockException( String message, Exception e )
     {
         String help = "Please ensure no other process is using this database, and that the directory is writable " +
-                      "(required even for read-only access)";
+                "(required even for read-only access)";
         return new StoreLockException( message + ". " + help, e );
     }
 
-    public void release() throws IOException
+    @Override
+    public void close() throws IOException
     {
         if ( storeLockFileLock != null )
         {

diff --git a/community/kernel/src/main/java/org/neo4j/kernel/internal/StoreLockerLifecycleAdapter.java b/community/kernel/src/main/java/org/neo4j/kernel/internal/StoreLockerLifecycleAdapter.java
@@ -43,6 +43,6 @@ public void start() throws Throwable
     @Override
     public void stop() throws Throwable
     {
-        storeLocker.release();
+        storeLocker.close();
     }
 }
diff --git a/community/kernel/src/main/java/org/neo4j/unsafe/batchinsert/internal/BatchInserterImpl.java b/community/kernel/src/main/java/org/neo4j/unsafe/batchinsert/internal/BatchInserterImpl.java
@@ -976,7 +976,7 @@ public void shutdown()
 
             try
             {
-                storeLocker.release();
+                storeLocker.close();
             }
             catch ( IOException e )
             {

diff --git a/community/kernel/src/test/java/org/neo4j/kernel/internal/StoreLockerTest.java b/community/kernel/src/test/java/org/neo4j/kernel/internal/StoreLockerTest.java
@@ -60,9 +60,8 @@ public boolean fileExists( File fileName )
                 return true;
             }
         };
-        StoreLocker storeLocker = new StoreLocker( fileSystemAbstraction );
 
-        try
+        try ( StoreLocker storeLocker = new StoreLocker( fileSystemAbstraction ) )
         {
             storeLocker.checkLock( target.directory( "unused" ) );
 
@@ -72,10 +71,6 @@ public boolean fileExists( File fileName )
         {
             fail();
         }
-        finally
-        {
-            storeLocker.release();
-        }
     }
 
     @Test
@@ -89,17 +84,12 @@ public boolean fileExists( File fileName )
                 return false;
             }
         };
-        StoreLocker storeLocker = new StoreLocker( fileSystemAbstraction );
 
-        try
+        try ( StoreLocker storeLocker = new StoreLocker( fileSystemAbstraction ) )
         {
             storeLocker.checkLock( target.directory( "unused" ) );
             // Ok
         }
-        finally
-        {
-            storeLocker.release();
-        }
     }
 
     @Test
@@ -119,23 +109,21 @@ public boolean fileExists( File fileName )
                 return false;
             }
         };
-        StoreLocker storeLocker = new StoreLocker( fileSystemAbstraction );
+
         File storeDir = target.directory( "unused" );
 
-        try
+        try ( StoreLocker storeLocker = new StoreLocker( fileSystemAbstraction ) )
         {
             storeLocker.checkLock( storeDir );
             fail();
         }
         catch ( StoreLockException e )
         {
-            String msg = format( "Unable to create path for store dir: %s. Please ensure no other process is using this database, and that the directory is writable (required even for read-only access)", storeDir );
+            String msg = format( "Unable to create path for store dir: %s. " +
+                    "Please ensure no other process is using this database, and that " +
+                    "the directory is writable (required even for read-only access)", storeDir );
             assertThat( e.getMessage(), is( msg ) );
         }
-        finally
-        {
-            storeLocker.release();
-        }
     }
 
     @Test
@@ -155,24 +143,22 @@ public boolean fileExists( File fileName )
                 return false;
             }
         };
-        StoreLocker storeLocker = new StoreLocker( fileSystemAbstraction );
+
         File storeDir = target.directory( "unused" );
 
-        try
+        try ( StoreLocker storeLocker = new StoreLocker( fileSystemAbstraction ) )
         {
             storeLocker.checkLock( storeDir );
             fail();
         }
         catch ( StoreLockException e )
         {
-            String msg = format( "Unable to obtain lock on store lock file: %s. Please ensure no other process is using this database, and that the directory is writable (required even for read-only access)", new File( storeDir,
+            String msg = format( "Unable to obtain lock on store lock file: %s. " +
-                    STORE_LOCK_FILENAME ) );
+                            "Please ensure no other process is using this database, and that the " +
+                            "directory is writable (required even for read-only access)",
+                    new File( storeDir, STORE_LOCK_FILENAME ) );
             assertThat( e.getMessage(), is( msg ) );
         }
-        finally
-        {
-            storeLocker.release();
-        }
     }
 
     @Test
@@ -199,20 +185,16 @@ public FileLock tryLock() throws IOException
                 };
             }
         };
-        StoreLocker storeLocker = new StoreLocker( fileSystemAbstraction );
 
-        try
+        try ( StoreLocker storeLocker = new StoreLocker( fileSystemAbstraction ) )
         {
             storeLocker.checkLock( target.directory( "unused" ) );
             fail();
         }
         catch ( StoreLockException e )
         {
-            assertThat( e.getMessage(), containsString( "Store and its lock file has been locked by another process" ) );
+            assertThat( e.getMessage(),
-        }
+                    containsString( "Store and its lock file has been locked by another process" ) );
-        finally
-        {
-            storeLocker.release();
         }
     }