Guarantee release cleanerLock in GBPTree

In CleanupJob, processing the job and releasing the cleaner lock
is separated into run and close such that a RecoveryCleanupWorkCollector
can close a CleanupJob and thus release the cleaner lock without
actually running the cleanup job. This makes it possible to open a dirty
GBPTree and write a new header without starting cleanup jobs and
not risking a deadlock between checkpoint (needed for writing header)
and cleanup job.

community/consistency-check/src/main/java/org/neo4j/consistency/internal/SchemaIndexExtensionLoader.java

@@ -79,9 +79,16 @@ private static class RecoveryPreventingCollector extends LifecycleAdapter implem
         @Override
         public void add( CleanupJob job )
         {
-            if ( job.needed() )
+            try
             {
-                throw new IllegalStateException( "Consistency checker should not do recovery" );
+                if ( job.needed() )
+                {
+                    throw new IllegalStateException( "Consistency checker should not do recovery" );
+                }
+            }
+            finally
+            {
+                job.close();
             }
         }
     }

community/index/src/main/java/org/neo4j/index/internal/gbptree/CleanupJob.java

@@ -42,6 +42,11 @@ public interface CleanupJob extends Runnable
      */
     Exception getCause();
 
+    /**
+     * Mark this job and cleanup all it's resources.
+     */
+    void close();
+
     /**
      * A {@link CleanupJob} that doesn't need cleaning, i.e. it's already clean.
      */
@@ -69,5 +74,10 @@ public Exception getCause()
         {
             return null;
         }
+
+        @Override
+        public void close()
+        {   // no-op
+        }
     };
 }

community/index/src/main/java/org/neo4j/index/internal/gbptree/GBPTreeCleanupJob.java

@@ -56,6 +56,12 @@ public Exception getCause()
         return failure;
     }
 
+    @Override
+    public void close()
+    {
+        gbpTreeLock.cleanerUnlock();
+    }
+
     @Override
     public void run()
     {
@@ -68,9 +74,5 @@ public void run()
         {
             failure = e;
         }
-        finally
-        {
-            gbpTreeLock.cleanerUnlock();
-        }
     }
 }

community/index/src/main/java/org/neo4j/index/internal/gbptree/GroupingRecoveryCleanupWorkCollector.java

@@ -80,7 +80,14 @@ private Runnable allJobs()
             CleanupJob job;
             while ( (job = jobs.poll()) != null )
             {
-                job.run();
+                try
+                {
+                    job.run();
+                }
+                finally
+                {
+                    job.close();
+                }
             }
         };
     }

community/index/src/main/java/org/neo4j/index/internal/gbptree/RecoveryCleanupWorkCollector.java

@@ -29,6 +29,8 @@
  * this, implementing class must be ready to receive new jobs through {@link #add(CleanupJob)}.
  * <p>
  * Jobs may be processed during {@link #add(CleanupJob) add} or {@link Lifecycle#start() start}.
+ * <p>
+ * Take full responsibility for closing added {@link CleanupJob CleanupJobs} as soon as possible after run.
  */
 public interface RecoveryCleanupWorkCollector extends Lifecycle
 {
@@ -48,7 +50,7 @@ public interface RecoveryCleanupWorkCollector extends Lifecycle
     /**
      * Ignore all clean jobs.
      */
-    RecoveryCleanupWorkCollector NULL = new NullRecoveryCleanupWorkCollector();
+    RecoveryCleanupWorkCollector IGNORE = new IgnoringRecoveryCleanupWorkCollector();
 
     /**
      * {@link RecoveryCleanupWorkCollector} which runs added {@link CleanupJob} as part of the {@link #add(CleanupJob)}
@@ -59,18 +61,26 @@ class ImmediateRecoveryCleanupWorkCollector extends LifecycleAdapter implements
         @Override
         public void add( CleanupJob job )
         {
-            job.run();
+            try
+            {
+                job.run();
+            }
+            finally
+            {
+                job.close();
+            }
         }
     }
 
     /**
      * {@link RecoveryCleanupWorkCollector} ignoring all {@link CleanupJob} added to it.
      */
-    class NullRecoveryCleanupWorkCollector extends LifecycleAdapter implements RecoveryCleanupWorkCollector
+    class IgnoringRecoveryCleanupWorkCollector extends LifecycleAdapter implements RecoveryCleanupWorkCollector
     {
         @Override
         public void add( CleanupJob job )
-        {   // no-op
+        {
+            job.close();
         }
     }
 }

community/index/src/test/java/org/neo4j/index/internal/gbptree/GBPTreeTest.java

@@ -625,6 +625,21 @@ private void verifyHeaderDataAfterClose( BiConsumer<GBPTree<MutableLong,MutableL
         verifyHeader( pageCache, expectedHeader );
     }
 
+    @Test( timeout = 10_000 )
+    public void writeHeaderInDirtyTreeMustNotDeadlock() throws Exception
+    {
+        PageCache pageCache = createPageCache( 256 );
+        makeDirty( pageCache );
+
+        Consumer<PageCursor> headerWriter = pc -> pc.putBytes( "failed".getBytes() );
+        try ( GBPTree<MutableLong,MutableLong> index = index( pageCache ).with( RecoveryCleanupWorkCollector.IGNORE ).build() )
+        {
+            index.checkpoint( IOLimiter.unlimited(), headerWriter );
+        }
+
+        verifyHeader( pageCache, "failed".getBytes() );
+    }
+
     private void verifyHeader( PageCache pageCache, byte[] expectedHeader ) throws IOException
     {
         // WHEN
@@ -1605,8 +1620,15 @@ public void start() throws Throwable
             CleanupJob job;
             while ( (job = jobs.poll()) != null )
             {
-                job.run();
-                startedJobs.add( job );
+                try
+                {
+                    job.run();
+                    startedJobs.add( job );
+                }
+                finally
+                {
+                    job.close();
+                }
             }
         }
 
@@ -1689,7 +1711,12 @@ private void maybeBlock() throws IOException
 
     private void makeDirty() throws IOException
     {
-        try ( GBPTree<MutableLong,MutableLong> index = index().build() )
+        makeDirty( createPageCache( DEFAULT_PAGE_SIZE ) );
+    }
+
+    private void makeDirty( PageCache pageCache ) throws IOException
+    {
+        try ( GBPTree<MutableLong,MutableLong> index = index( pageCache ).build() )
         {
             // Make dirty
             index.writer().close();

community/index/src/test/java/org/neo4j/index/internal/gbptree/GroupingRecoveryCleanupWorkCollectorTest.java

@@ -184,6 +184,11 @@ public Exception getCause()
             return null;
         }
 
+        @Override
+        public void close()
+        {   // no-op
+        }
+
         @Override
         public void run()
         {

community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/NativeSchemaNumberIndexPopulator.java

@@ -211,7 +211,7 @@ private void ensureTreeInstantiated() throws IOException
     {
         if ( tree == null )
         {
-            instantiateTree( RecoveryCleanupWorkCollector.NULL, NO_HEADER_WRITER );
+            instantiateTree( RecoveryCleanupWorkCollector.IGNORE, NO_HEADER_WRITER );
         }
     }
 

community/kernel/src/test/java/org/neo4j/kernel/impl/storemigration/participant/NativeLabelScanStoreMigratorTest.java

@@ -200,7 +200,7 @@ public void reportProgressOnNativeIndexPopulation() throws IOException
     private NativeLabelScanStore getNativeLabelScanStore( File dir, boolean readOnly )
     {
         return new NativeLabelScanStore( pageCache, dir, FullStoreChangeStream.EMPTY, readOnly, new Monitors(),
-                RecoveryCleanupWorkCollector.NULL );
+                RecoveryCleanupWorkCollector.IGNORE );
     }
 
     private void initializeNativeLabelScanStoreWithContent( File dir ) throws IOException