Make EphemeralFileSystemAbstraction AutoCloseable

Make ephemeral fs to implement AutoCloseable and move resource cleanup into close() instead of relying on finalization. That will make usage pattern more explicit and will avoid cases when yet opened resources get cleaned up because file system itself get garbage collected.
neo4j · Nov 21, 2016 · 686df3c · 686df3c
1 parent 52c2347
commit 686df3c
Show file tree

Hide file tree

Showing 18 changed files with 76 additions and 63 deletions.
diff --git a/community/io/src/test/java/org/neo4j/graphdb/mockfs/EphemeralFileSystemAbstraction.java b/community/io/src/test/java/org/neo4j/graphdb/mockfs/EphemeralFileSystemAbstraction.java
@@ -76,7 +76,7 @@
 import static java.nio.file.StandardCopyOption.REPLACE_EXISTING;
 import static java.util.Arrays.asList;
 
-public class EphemeralFileSystemAbstraction implements FileSystemAbstraction
+public class EphemeralFileSystemAbstraction implements FileSystemAbstraction, AutoCloseable
 {
     private final Clock clock;
 
@@ -135,7 +135,8 @@ public void crash()
         files.values().forEach( EphemeralFileSystemAbstraction.EphemeralFileData::crash );
     }
 
-    public synchronized void shutdown()
+    @Override
+    public synchronized void close()
     {
         for ( EphemeralFileData file : files.values() )
         {
@@ -150,13 +151,6 @@ public synchronized void shutdown()
         thirdPartyFileSystems.clear();
     }
 
-    @Override
-    protected void finalize() throws Throwable
-    {
-        shutdown();
-        super.finalize();
-    }
-
     public void assertNoOpenFiles() throws Exception
     {
         FileStillOpenException exception = null;

diff --git a/community/io/src/test/java/org/neo4j/io/pagecache/PageCacheTestSupport.java b/community/io/src/test/java/org/neo4j/io/pagecache/PageCacheTestSupport.java
@@ -106,7 +106,7 @@ public void tearDown() throws IOException
 
         if ( fs instanceof EphemeralFileSystemAbstraction )
         {
-            ((EphemeralFileSystemAbstraction) fs).shutdown();
+            ((EphemeralFileSystemAbstraction) fs).close();
         }
     }
 

diff --git a/community/io/src/test/java/org/neo4j/io/pagecache/impl/SingleFilePageSwapperTest.java b/community/io/src/test/java/org/neo4j/io/pagecache/impl/SingleFilePageSwapperTest.java
@@ -71,7 +71,7 @@ public void setUp() throws IOException
     @After
     public void tearDown()
     {
-        fs.shutdown();
+        fs.close();
     }
 
     protected PageSwapperFactory swapperFactory()

diff --git a/...ity/io/src/test/java/org/neo4j/io/pagecache/randomharness/RandomPageCacheTestHarness.java b/...ity/io/src/test/java/org/neo4j/io/pagecache/randomharness/RandomPageCacheTestHarness.java
@@ -421,7 +421,7 @@ private void runIteration( long timeout, TimeUnit unit ) throws Exception
 
             if ( this.fs instanceof EphemeralFileSystemAbstraction )
             {
-                ((EphemeralFileSystemAbstraction) this.fs).shutdown();
+                ((EphemeralFileSystemAbstraction) this.fs).close();
                 this.fs = new EphemeralFileSystemAbstraction();
             }
             else

diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/api/LabelRecoveryTest.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/api/LabelRecoveryTest.java
@@ -34,6 +34,19 @@
 
 public class LabelRecoveryTest
 {
+    public final EphemeralFileSystemAbstraction fs = new EphemeralFileSystemAbstraction();
+    private GraphDatabaseService database;
+
+    @After
+    public void tearDown()
+    {
+        if ( database != null )
+        {
+            database.shutdown();
+        }
+        fs.close();
+    }
+
     /**
      * Reading a node command might leave a node record which referred to
      * labels in one or more dynamic records as marked as heavy even if that
@@ -87,17 +100,4 @@ public void shouldRecoverNodeWithDynamicLabelRecords() throws Exception
             }
         }
     }
-
-    @After
-    public void tearDown()
-    {
-        if ( database != null )
-        {
-            database.shutdown();
-        }
-        fs.shutdown();
-    }
-
-    public final EphemeralFileSystemAbstraction fs = new EphemeralFileSystemAbstraction();
-    private GraphDatabaseService database;
 }
diff --git a/...kernel/src/test/java/org/neo4j/kernel/impl/api/integrationtest/KernelIntegrationTest.java b/...kernel/src/test/java/org/neo4j/kernel/impl/api/integrationtest/KernelIntegrationTest.java
@@ -36,7 +36,6 @@
 import org.neo4j.kernel.api.exceptions.KernelException;
 import org.neo4j.kernel.api.exceptions.TransactionFailureException;
 import org.neo4j.kernel.api.security.AnonymousContext;
-import org.neo4j.kernel.api.security.SecurityContext;
 import org.neo4j.kernel.impl.api.index.IndexingService;
 import org.neo4j.kernel.impl.core.ThreadToStatementContextBridge;
 import org.neo4j.kernel.internal.GraphDatabaseAPI;
@@ -139,7 +138,7 @@ public void setup()
     public void cleanup() throws Exception
     {
         stopDb();
-        fs.shutdown();
+        fs.close();
     }
 
     protected void startDb()

diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/core/JumpingFileSystemAbstraction.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/core/JumpingFileSystemAbstraction.java
@@ -261,6 +261,6 @@ public int write( ByteBuffer src, long position ) throws IOException
 
     public void shutdown()
     {
-        ephemeralFileSystem.shutdown();
+        ephemeralFileSystem.close();
     }
 }
diff --git a/...nity/kernel/src/test/java/org/neo4j/kernel/impl/recovery/RecoveryRequiredCheckerTest.java b/...nity/kernel/src/test/java/org/neo4j/kernel/impl/recovery/RecoveryRequiredCheckerTest.java
@@ -22,6 +22,7 @@
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.RuleChain;
 
 import java.io.File;
 import java.io.IOException;
@@ -34,25 +35,29 @@
 import org.neo4j.test.TestGraphDatabaseFactory;
 import org.neo4j.test.rule.PageCacheRule;
 import org.neo4j.test.rule.TestDirectory;
+import org.neo4j.test.rule.fs.EphemeralFileSystemRule;
 
 import static org.hamcrest.CoreMatchers.is;
 import static org.junit.Assert.assertThat;
 
 public class RecoveryRequiredCheckerTest
 
 {
-    private final EphemeralFileSystemAbstraction fileSystem = new EphemeralFileSystemAbstraction();
-    @Rule
-    public final PageCacheRule pageCacheRule = new PageCacheRule();
+    private final EphemeralFileSystemRule fileSystemRule = new EphemeralFileSystemRule();
+    private final PageCacheRule pageCacheRule = new PageCacheRule();
+    private final TestDirectory testDirectory = TestDirectory.testDirectory( fileSystemRule.get() );
+
     @Rule
-    public TestDirectory testDirectory = TestDirectory.testDirectory( fileSystem );
+    public RuleChain ruleChain = RuleChain.outerRule( pageCacheRule ).around( fileSystemRule ).around( testDirectory );
 
+    private EphemeralFileSystemAbstraction fileSystem;
     private File storeDir;
 
     @Before
     public void setup()
     {
         storeDir = testDirectory.graphDbDir();
+        fileSystem = fileSystemRule.get();
         new TestGraphDatabaseFactory().setFileSystem( fileSystem ).newImpermanentDatabase( storeDir ).shutdown();
     }
 

diff --git a/...nel/src/test/java/org/neo4j/kernel/impl/store/IdGeneratorRebuildFailureEmulationTest.java b/...nel/src/test/java/org/neo4j/kernel/impl/store/IdGeneratorRebuildFailureEmulationTest.java
@@ -213,11 +213,11 @@ void disposeAndAssertNoOpenFiles() throws Exception
             //Collection<String> open = openFiles();
             //assertTrue( "Open files: " + open, open.isEmpty() );
             assertNoOpenFiles();
-            super.shutdown();
+            super.close();
         }
 
         @Override
-        public void shutdown()
+        public void close()
         {
             // no-op, it's pretty odd to have EphemeralFileSystemAbstraction implement Lifecycle by default
         }

diff --git a/...ava/org/neo4j/kernel/impl/transaction/log/BatchingTransactionAppenderConcurrencyTest.java b/...ava/org/neo4j/kernel/impl/transaction/log/BatchingTransactionAppenderConcurrencyTest.java
@@ -322,7 +322,7 @@ private Lifecycle asLifecycle( final EphemeralFileSystemAbstraction efs )
             @Override
             public void shutdown() throws Throwable
             {
-                efs.shutdown();
+                efs.close();
             }
         };
     }

diff --git a/community/kernel/src/test/java/org/neo4j/metatest/TestEphemeralFileChannel.java b/community/kernel/src/test/java/org/neo4j/metatest/TestEphemeralFileChannel.java
@@ -95,7 +95,7 @@ public void smoke() throws Exception
         channel.read( buffer, 15 );
         buffer.flip();
         assertEquals( longValue, buffer.getLong() );
-        fs.shutdown();
+        fs.close();
     }
 
     @Test
@@ -117,7 +117,7 @@ public void absoluteVersusRelative() throws Exception
         // THEN
         assertEquals( bytes.length, nrOfReadBytes );
         assertTrue( Arrays.equals( bytes, readBytes ) );
-        fs.shutdown();
+        fs.close();
     }
 
     @Test
@@ -156,6 +156,6 @@ public void listFiles() throws Exception
         assertEquals( asSet( subdir1, file1, file2 ), asSet( fs.listFiles( dir1 ) ) );
         assertEquals( asSet( file3 ), asSet( fs.listFiles( dir2 ) ) );
         assertEquals( asSet( file4 ), asSet( fs.listFiles( subdir1 ) ) );
-        fs.shutdown();
+        fs.close();
     }
 }
diff --git a/community/kernel/src/test/java/org/neo4j/test/rule/NeoStoresRule.java b/community/kernel/src/test/java/org/neo4j/test/rule/NeoStoresRule.java
@@ -101,7 +101,7 @@ protected void after( boolean successful ) throws Throwable
         }
         if ( efs != null )
         {
-            efs.shutdown();
+            efs.close();
         }
     }
 

diff --git a/community/kernel/src/test/java/org/neo4j/test/rule/fs/EphemeralFileSystemRule.java b/community/kernel/src/test/java/org/neo4j/test/rule/fs/EphemeralFileSystemRule.java
@@ -46,7 +46,7 @@ public class EphemeralFileSystemRule extends ExternalResource implements Supplie
     @Override
     protected void after()
     {
-        fs.shutdown();
+        fs.close();
     }
 
     @Override
@@ -64,15 +64,15 @@ public EphemeralFileSystemAbstraction snapshot( Runnable action )
         }
         finally
         {
-            fs.shutdown();
+            fs.close();
             fs = snapshot;
         }
         return fs;
     }
 
     public void clear()
     {
-        fs.shutdown();
+        fs.close();
         fs = new EphemeralFileSystemAbstraction();
     }
 
@@ -100,7 +100,7 @@ public void crash()
 
     public void shutdown()
     {
-        fs.shutdown();
+        fs.close();
     }
 
     public void assertNoOpenFiles() throws Exception

diff --git a/community/security/src/test/java/org/neo4j/server/security/auth/AuthProceduresIT.java b/community/security/src/test/java/org/neo4j/server/security/auth/AuthProceduresIT.java
@@ -231,7 +231,7 @@ public void setup() throws InvalidAuthTokenException, IOException
     public void cleanup() throws Exception
     {
         db.shutdown();
-        fs.shutdown();
+        fs.close();
     }
 
     private GraphDatabaseService createGraphDatabase( EphemeralFileSystemAbstraction fs ) throws IOException

diff --git a/...src/test/java/org/neo4j/causalclustering/core/consensus/log/segmented/ReaderPoolTest.java b/...src/test/java/org/neo4j/causalclustering/core/consensus/log/segmented/ReaderPoolTest.java
@@ -20,6 +20,7 @@
 package org.neo4j.causalclustering.core.consensus.log.segmented;
 
 import org.hamcrest.Matchers;
+import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
 
@@ -56,6 +57,12 @@ public void before()
         fsa.mkdirs( base );
     }
 
+    @After
+    public void tearDown()
+    {
+        fsa.close();
+    }
+
     @Test
     public void shouldReacquireReaderFromPool() throws Exception
     {

diff --git a/...st/java/org/neo4j/causalclustering/core/consensus/log/segmented/RecoveryProtocolTest.java b/...st/java/org/neo4j/causalclustering/core/consensus/log/segmented/RecoveryProtocolTest.java
@@ -20,6 +20,7 @@
 package org.neo4j.causalclustering.core.consensus.log.segmented;
 
 import org.junit.Before;
+import org.junit.Rule;
 import org.junit.Test;
 
 import java.io.File;
@@ -32,6 +33,7 @@
 import org.neo4j.io.fs.StoreChannel;
 import org.neo4j.kernel.impl.transaction.log.PhysicalFlushableChannel;
 import org.neo4j.logging.NullLogProvider;
+import org.neo4j.test.rule.fs.EphemeralFileSystemRule;
 import org.neo4j.time.Clocks;
 
 import static org.junit.Assert.assertEquals;
@@ -41,13 +43,15 @@
 
 public class RecoveryProtocolTest
 {
-    private EphemeralFileSystemAbstraction fsa = new EphemeralFileSystemAbstraction();
+    @Rule
+    public final EphemeralFileSystemRule fileSystemRule = new EphemeralFileSystemRule();
+
+    private EphemeralFileSystemAbstraction fsa = fileSystemRule.get();
     private ChannelMarshal<ReplicatedContent> contentMarshal = new DummyRaftableContentSerializer();
     private final File root = new File( "root" );
     private FileNames fileNames = new FileNames( root );
     private SegmentHeader.Marshal headerMarshal = new SegmentHeader.Marshal();
-    private ReaderPool readerPool = new ReaderPool( 0, getInstance(), fileNames, fsa,
-            Clocks.fakeClock() );
+    private ReaderPool readerPool = new ReaderPool( 0, getInstance(), fileNames, fsa, Clocks.fakeClock() );
 
     @Before
     public void setup()

diff --git a/enterprise/ha/src/test/java/org/neo4j/kernel/ha/id/HaIdGeneratorFactoryTest.java b/enterprise/ha/src/test/java/org/neo4j/kernel/ha/id/HaIdGeneratorFactoryTest.java
@@ -20,6 +20,7 @@
 package org.neo4j.kernel.ha.id;
 
 import org.junit.Before;
+import org.junit.Rule;
 import org.junit.Test;
 
 import java.io.File;
@@ -39,6 +40,7 @@
 import org.neo4j.kernel.impl.store.id.IdType;
 import org.neo4j.kernel.impl.store.id.configuration.CommunityIdTypeConfigurationProvider;
 import org.neo4j.logging.NullLogProvider;
+import org.neo4j.test.rule.fs.EphemeralFileSystemRule;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
@@ -55,6 +57,23 @@
 
 public class HaIdGeneratorFactoryTest
 {
+    @Rule
+    public final EphemeralFileSystemRule fileSystemRule = new EphemeralFileSystemRule();
+    private Master master;
+    private DelegateInvocationHandler<Master> masterDelegate;
+    private EphemeralFileSystemAbstraction fs;
+    private HaIdGeneratorFactory fac;
+
+    @Before
+    public void before()
+    {
+        master = mock( Master.class );
+        masterDelegate = new DelegateInvocationHandler<>( Master.class );
+        fs = fileSystemRule.get();
+        fac  = new HaIdGeneratorFactory( masterDelegate, NullLogProvider.getInstance(),
+                mock( RequestContextFactory.class ), fs, new CommunityIdTypeConfigurationProvider()  );
+    }
+
     @Test
     public void slaveIdGeneratorShouldReturnFromAssignedRange() throws Exception
     {
@@ -239,21 +258,6 @@ public void shouldNotUseForbiddenMinusOneIdFromIdBatches() throws Exception
         assertEquals( VALUE_REPRESENTING_NULL, iterartor.next() );
     }
 
-    private Master master;
-    private DelegateInvocationHandler<Master> masterDelegate;
-    private EphemeralFileSystemAbstraction fs;
-    private HaIdGeneratorFactory fac;
-
-    @Before
-    public void before()
-    {
-        master = mock( Master.class );
-        masterDelegate = new DelegateInvocationHandler<>( Master.class );
-        fs = new EphemeralFileSystemAbstraction();
-        fac  = new HaIdGeneratorFactory( masterDelegate, NullLogProvider.getInstance(),
-                mock( RequestContextFactory.class ), fs, new CommunityIdTypeConfigurationProvider()  );
-    }
-
     @SuppressWarnings( "unchecked" )
     private Response<IdAllocation> response( IdAllocation firstValue, IdAllocation... additionalValues )
     {

diff --git a/...ise/security/src/test/java/org/neo4j/server/security/enterprise/auth/BoltInteraction.java b/...ise/security/src/test/java/org/neo4j/server/security/enterprise/auth/BoltInteraction.java
@@ -194,7 +194,7 @@ public void tearDown() throws Throwable
         }
         subjects.clear();
         server.graphDatabaseService().shutdown();
-        fileSystem.shutdown();
+        fileSystem.close();
     }
 
     @Override