Revert "Fixes race between starting transaction and shutdown"

This reverts commit 2c4d750.
neo4j · Jul 7, 2017 · b5ff11e · b5ff11e
1 parent 739eaae
commit b5ff11e
Show file tree

Hide file tree

Showing 12 changed files with 36 additions and 380 deletions.
diff --git a/community/kernel/src/main/java/org/neo4j/kernel/DatabaseAvailability.java b/community/kernel/src/main/java/org/neo4j/kernel/DatabaseAvailability.java
@@ -21,14 +21,13 @@
 
 import java.time.Clock;
 
-import org.neo4j.kernel.AvailabilityGuard.AvailabilityRequirement;
 import org.neo4j.kernel.impl.transaction.TransactionStats;
 import org.neo4j.kernel.lifecycle.Lifecycle;
 import org.neo4j.time.Clocks;
 
 import static java.util.concurrent.TimeUnit.MILLISECONDS;
 import static java.util.concurrent.locks.LockSupport.parkNanos;
-
+import static org.neo4j.kernel.AvailabilityGuard.AvailabilityRequirement;
 import static org.neo4j.kernel.AvailabilityGuard.availabilityRequirement;
 
 /**
@@ -93,5 +92,6 @@ private void awaitTransactionsClosedWithinTimeout()
     public void shutdown()
             throws Throwable
     {
+        // TODO: Starting database. Make sure none can access it through lock or CAS
     }
 }
diff --git a/community/kernel/src/main/java/org/neo4j/kernel/NeoStoreDataSource.java b/community/kernel/src/main/java/org/neo4j/kernel/NeoStoreDataSource.java
@@ -735,7 +735,7 @@ private NeoStoreKernelModule buildKernel( TransactionAppender appender,
                 availabilityGuard, tracers, storageEngine, procedures, transactionIdStore, clock, accessCapability ) );
 
         final Kernel kernel = new Kernel( kernelTransactions, hooks, databaseHealth, transactionMonitor, procedures,
-                config, availabilityGuard );
+                config );
 
         kernel.registerTransactionHook( transactionEventHandlers );
 

diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/Kernel.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/Kernel.java
@@ -20,13 +20,10 @@
 package org.neo4j.kernel.impl.api;
 
 import org.neo4j.graphdb.factory.GraphDatabaseSettings;
-import org.neo4j.kernel.AvailabilityGuard;
-import org.neo4j.kernel.AvailabilityGuard.UnavailableException;
 import org.neo4j.kernel.api.KernelAPI;
 import org.neo4j.kernel.api.KernelTransaction;
 import org.neo4j.kernel.api.TransactionHook;
 import org.neo4j.kernel.api.exceptions.ProcedureException;
-import org.neo4j.kernel.api.exceptions.Status;
 import org.neo4j.kernel.api.exceptions.TransactionFailureException;
 import org.neo4j.kernel.api.proc.CallableProcedure;
 import org.neo4j.kernel.api.proc.CallableUserAggregationFunction;
@@ -67,22 +64,17 @@ public class Kernel extends LifecycleAdapter implements KernelAPI
     private final DatabaseHealth health;
     private final TransactionMonitor transactionMonitor;
     private final Procedures procedures;
-    private final AvailabilityGuard availabilityGuard;
     private final long defaultTransactionTimeout;
-    private final long transactionStartTimeout;
 
     public Kernel( KernelTransactions transactionFactory, TransactionHooks hooks, DatabaseHealth health,
-            TransactionMonitor transactionMonitor, Procedures procedures, Config config,
-            AvailabilityGuard availabilityGuard )
+            TransactionMonitor transactionMonitor, Procedures procedures, Config config )
     {
         this.transactions = transactionFactory;
         this.hooks = hooks;
         this.health = health;
         this.transactionMonitor = transactionMonitor;
         this.procedures = procedures;
-        this.availabilityGuard = availabilityGuard;
         this.defaultTransactionTimeout = config.get( GraphDatabaseSettings.transaction_timeout ).toMillis();
-        this.transactionStartTimeout = config.get( GraphDatabaseSettings.transaction_start_timeout ).toMillis();
     }
 
     @Override
@@ -96,21 +88,9 @@ public KernelTransaction newTransaction( KernelTransaction.Type type, SecurityCo
             TransactionFailureException
     {
         health.assertHealthy( TransactionFailureException.class );
-
-        // Increment transaction monitor before checking the availability guard (which needs to be checked
-        // before starting the transaction). This is because the DatabaseAvailability does this in the other
-        // order, where it closes the availability guard and then awaits started transactions to finish.
+        KernelTransaction transaction = transactions.newInstance( type, securityContext, timeout );
         transactionMonitor.transactionStarted();
-        try
-        {
-            availabilityGuard.await( transactionStartTimeout );
-        }
-        catch ( UnavailableException e )
-        {
-            transactionMonitor.transactionFinished( false, false );
-            throw new TransactionFailureException( e.status(), e, e.getMessage() );
-        }
-        return transactions.newInstance( type, securityContext, timeout );
+        return transaction;
     }
 
     @Override

diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/KernelTransactions.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/KernelTransactions.java
@@ -322,7 +322,7 @@ private void assertCurrentThreadIsNotBlockingNewTransactions()
 
     private class KernelTransactionImplementationFactory implements Factory<KernelTransactionImplementation>
     {
-        private final Set<KernelTransactionImplementation> transactions;
+        private Set<KernelTransactionImplementation> transactions;
 
         KernelTransactionImplementationFactory( Set<KernelTransactionImplementation> transactions )
         {
@@ -345,7 +345,7 @@ public KernelTransactionImplementation newInstance()
 
     private class GlobalKernelTransactionPool extends LinkedQueuePool<KernelTransactionImplementation>
     {
-        private final Set<KernelTransactionImplementation> transactions;
+        private Set<KernelTransactionImplementation> transactions;
 
         GlobalKernelTransactionPool( Set<KernelTransactionImplementation> transactions,
                 Factory<KernelTransactionImplementation> factory )

diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/coreapi/CoreAPIAvailabilityGuard.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/coreapi/CoreAPIAvailabilityGuard.java
@@ -50,16 +50,11 @@ public void assertDatabaseAvailable()
         }
         catch ( AvailabilityGuard.UnavailableException e )
         {
-            throw convertUnavailabilityException( e );
+            if ( guard.isShutdown() )
+            {
+                throw new DatabaseShutdownException();
+            }
+            throw new org.neo4j.graphdb.TransactionFailureException( e.getMessage() );
         }
     }
-
-    public RuntimeException convertUnavailabilityException( Exception e )
-    {
-        if ( guard.isShutdown() )
-        {
-            return new DatabaseShutdownException();
-        }
-        return new org.neo4j.graphdb.TransactionFailureException( e.getMessage(), e );
-    }
 }
diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/factory/ClassicCoreSPI.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/factory/ClassicCoreSPI.java
@@ -154,8 +154,8 @@ public void shutdown()
     {
         try
         {
-            platform.availabilityGuard.shutdown();
             msgLog.log( "Shutdown started" );
+            platform.availabilityGuard.shutdown();
             platform.life.shutdown();
         }
         catch ( LifecycleException throwable )
@@ -170,6 +170,7 @@ public KernelTransaction beginTransaction( KernelTransaction.Type type, Security
     {
         try
         {
+            availability.assertDatabaseAvailable();
             KernelTransaction kernelTx = dataSource.kernelAPI.get().newTransaction( type, securityContext, timeout );
             kernelTx.registerCloseListener(
                     (txId) -> dataSource.threadToTransactionBridge.unbindTransactionFromCurrentThread() );
@@ -178,7 +179,7 @@ public KernelTransaction beginTransaction( KernelTransaction.Type type, Security
         }
         catch ( TransactionFailureException e )
         {
-            throw availability.convertUnavailabilityException( e );
+            throw new org.neo4j.graphdb.TransactionFailureException( e.getMessage(), e );
         }
     }
 

diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/factory/GraphDatabaseFacade.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/factory/GraphDatabaseFacade.java
@@ -105,7 +105,6 @@
 import org.neo4j.storageengine.api.EntityType;
 
 import static java.lang.String.format;
-
 import static org.neo4j.collection.primitive.PrimitiveLongCollections.map;
 import static org.neo4j.helpers.collection.Iterators.emptyIterator;
 import static org.neo4j.kernel.api.security.SecurityContext.AUTH_DISABLED;

diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/factory/PlatformModule.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/factory/PlatformModule.java
@@ -183,7 +183,8 @@ public PlatformModule( File providedStoreDir, Config config, DatabaseInfo databa
         // Anyways please fix this.
         dependencies.satisfyDependency( dataSourceManager );
 
-        availabilityGuard = dependencies.satisfyDependency( createAvailabilityGuard() );
+        availabilityGuard = dependencies.satisfyDependency(
+                new AvailabilityGuard( clock, logging.getInternalLog( AvailabilityGuard.class ) ) );
 
         transactionMonitor = dependencies.satisfyDependency( createTransactionStats() );
 
@@ -199,11 +200,6 @@ public PlatformModule( File providedStoreDir, Config config, DatabaseInfo databa
         publishPlatformInfo( dependencies.resolveDependency( UsageData.class ) );
     }
 
-    protected AvailabilityGuard createAvailabilityGuard()
-    {
-        return new AvailabilityGuard( clock, logging.getInternalLog( AvailabilityGuard.class ) );
-    }
-
     protected SystemNanoClock createClock()
     {
         return Clocks.nanoClock();

diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/proc/ProcedureGDBFacadeSPI.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/proc/ProcedureGDBFacadeSPI.java
@@ -186,6 +186,7 @@ public KernelTransaction beginTransaction( KernelTransaction.Type type, Security
     {
         try
         {
+            availability.assertDatabaseAvailable();
             KernelTransaction kernelTx = sourceModule.kernelAPI.get().newTransaction( type, this.securityContext, timeout );
             kernelTx.registerCloseListener(
                     ( txId ) -> sourceModule.threadToTransactionBridge.unbindTransactionFromCurrentThread() );
@@ -194,7 +195,7 @@ public KernelTransaction beginTransaction( KernelTransaction.Type type, Security
         }
         catch ( TransactionFailureException e )
         {
-            throw availability.convertUnavailabilityException( e );
+            throw new org.neo4j.graphdb.TransactionFailureException( e.getMessage(), e );
         }
     }
 }
diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/api/KernelTest.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/api/KernelTest.java
@@ -19,21 +19,13 @@
  */
 package org.neo4j.kernel.impl.api;
 
-import org.junit.Rule;
 import org.junit.Test;
-import org.junit.rules.RuleChain;
-import org.mockito.InOrder;
 
 import java.io.File;
-import java.time.Clock;
 import java.util.Map;
 import java.util.function.Function;
 
 import org.neo4j.graphdb.Transaction;
-import org.neo4j.kernel.AvailabilityGuard;
-import org.neo4j.kernel.NeoStoreDataSource;
-import org.neo4j.kernel.api.KernelAPI;
-import org.neo4j.kernel.api.KernelTransaction;
 import org.neo4j.kernel.api.Statement;
 import org.neo4j.kernel.api.exceptions.InvalidTransactionTypeKernelException;
 import org.neo4j.kernel.configuration.Config;
@@ -44,34 +36,16 @@
 import org.neo4j.kernel.impl.factory.GraphDatabaseFacade;
 import org.neo4j.kernel.impl.factory.GraphDatabaseFacadeFactory;
 import org.neo4j.kernel.impl.factory.PlatformModule;
-import org.neo4j.kernel.impl.transaction.TransactionMonitor;
-import org.neo4j.kernel.impl.util.Dependencies;
 import org.neo4j.kernel.internal.GraphDatabaseAPI;
-import org.neo4j.logging.NullLog;
 import org.neo4j.test.ImpermanentGraphDatabase;
-import org.neo4j.test.rule.NeoStoreDataSourceRule;
-import org.neo4j.test.rule.PageCacheAndDependenciesRule;
+
 import static org.hamcrest.Matchers.containsString;
 import static org.junit.Assert.assertThat;
 import static org.junit.Assert.fail;
-import static org.mockito.Matchers.anyLong;
-import static org.mockito.Mockito.inOrder;
-import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.spy;
-import static org.mockito.Mockito.times;
-
 import static org.neo4j.kernel.api.schema.SchemaDescriptorFactory.forLabel;
-import static org.neo4j.kernel.api.security.SecurityContext.AUTH_DISABLED;
-import static org.neo4j.kernel.api.KernelTransaction.Type.explicit;
 
 public class KernelTest
 {
-    private final PageCacheAndDependenciesRule pageCacheRule = new PageCacheAndDependenciesRule();
-    private final NeoStoreDataSourceRule neoStoreRule = new NeoStoreDataSourceRule();
-
-    @Rule
-    public final RuleChain rules = RuleChain.outerRule( pageCacheRule ).around( neoStoreRule );
-
     @Test
     public void shouldNotAllowCreationOfConstraintsWhenInHA() throws Exception
     {
@@ -98,29 +72,6 @@ public void shouldNotAllowCreationOfConstraintsWhenInHA() throws Exception
         db.shutdown();
     }
 
-    @Test
-    public void shouldIncrementTransactionMonitorBeforeCheckingDatabaseAvailability() throws Exception
-    {
-        // GIVEN
-        AvailabilityGuard availabilityGuard = spy( new AvailabilityGuard( Clock.systemUTC(), NullLog.getInstance() ) );
-        TransactionMonitor transactionMonitor = mock( TransactionMonitor.class );
-        Dependencies dependencies = new Dependencies();
-        dependencies.satisfyDependencies( availabilityGuard, transactionMonitor );
-        NeoStoreDataSource dataSource = neoStoreRule.getDataSource( pageCacheRule.directory().absolutePath(),
-                pageCacheRule.fileSystem(), pageCacheRule.pageCache(), dependencies );
-        dataSource.start();
-        KernelAPI kernel = dataSource.getKernel();
-
-        // WHEN
-        try ( KernelTransaction tx = kernel.newTransaction( explicit, AUTH_DISABLED ) )
-        {
-            // THEN
-            InOrder order = inOrder( transactionMonitor, availabilityGuard );
-            order.verify( transactionMonitor, times( 1 ) ).transactionStarted();
-            order.verify( availabilityGuard, times( 1 ) ).await( anyLong() );
-        }
-    }
-
     @SuppressWarnings("deprecation")
     class FakeHaDatabase extends ImpermanentGraphDatabase
     {
@@ -145,8 +96,8 @@ protected SchemaWriteGuard createSchemaWriteGuard()
             new GraphDatabaseFacadeFactory( DatabaseInfo.COMMUNITY,  factory )
             {
                 @Override
-                protected PlatformModule createPlatform( File storeDir, Config config,
-                        GraphDatabaseFacadeFactory.Dependencies dependencies, GraphDatabaseFacade graphDatabaseFacade )
+                protected PlatformModule createPlatform( File storeDir, Config config, Dependencies dependencies,
+                        GraphDatabaseFacade graphDatabaseFacade )
                 {
                     return new ImpermanentPlatformModule( storeDir, config, databaseInfo, dependencies,
                             graphDatabaseFacade );