Make Neo4jTransactionalContext termination safe

* ask the underlying transaction about it's termination status instead of having a separate termination flag * make transaction field volatile because it can be accessed from a different thread during termination * allow closing after termination because it is still required to free all kernel transaction resources
neo4j · Apr 13, 2017 · 5a09123 · 5a09123
1 parent c62d5ab
commit 5a09123
Show file tree

Hide file tree

Showing 7 changed files with 253 additions and 21 deletions.
diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/coreapi/InternalTransaction.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/coreapi/InternalTransaction.java
@@ -19,13 +19,20 @@
  */
 package org.neo4j.kernel.impl.coreapi;
 
+import java.util.Optional;
+
 import org.neo4j.graphdb.Transaction;
 import org.neo4j.kernel.api.KernelTransaction;
+import org.neo4j.kernel.api.exceptions.Status;
 import org.neo4j.kernel.api.security.SecurityContext;
 
 public interface InternalTransaction extends Transaction
 {
     KernelTransaction.Type transactionType();
+
     SecurityContext securityContext();
+
     KernelTransaction.Revertable overrideWith( SecurityContext context );
+
+    Optional<Status> terminationReason();
 }
diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/coreapi/PlaceboTransaction.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/coreapi/PlaceboTransaction.java
@@ -19,6 +19,7 @@
  */
 package org.neo4j.kernel.impl.coreapi;
 
+import java.util.Optional;
 import java.util.function.Supplier;
 
 import org.neo4j.graphdb.Lock;
@@ -97,4 +98,10 @@ public KernelTransaction.Revertable overrideWith( SecurityContext context )
     {
         return currentTransaction.overrideWith( context );
     }
+
+    @Override
+    public Optional<Status> terminationReason()
+    {
+        return currentTransaction.getReasonIfTerminated();
+    }
 }
diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/coreapi/TopLevelTransaction.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/coreapi/TopLevelTransaction.java
@@ -19,6 +19,7 @@
  */
 package org.neo4j.kernel.impl.coreapi;
 
+import java.util.Optional;
 import java.util.function.Supplier;
 
 import org.neo4j.graphdb.ConstraintViolationException;
@@ -142,4 +143,10 @@ public KernelTransaction.Revertable overrideWith( SecurityContext context )
     {
         return transaction.overrideWith( context );
     }
+
+    @Override
+    public Optional<Status> terminationReason()
+    {
+        return transaction.getReasonIfTerminated();
+    }
 }
diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/query/Neo4jTransactionalContext.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/query/Neo4jTransactionalContext.java
@@ -19,11 +19,9 @@
  */
 package org.neo4j.kernel.impl.query;
 
-import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.function.Supplier;
 
 import org.neo4j.graphdb.Lock;
-import org.neo4j.graphdb.NotInTransactionException;
 import org.neo4j.graphdb.PropertyContainer;
 import org.neo4j.graphdb.TransactionTerminatedException;
 import org.neo4j.io.pagecache.tracing.cursor.PageCursorTracer;
@@ -43,8 +41,6 @@
 import org.neo4j.kernel.impl.coreapi.PropertyContainerLocker;
 import org.neo4j.kernel.impl.query.statistic.StatisticProvider;
 
-import static org.neo4j.kernel.api.exceptions.Status.Transaction.TransactionTerminated;
-
 public class Neo4jTransactionalContext implements TransactionalContext
 {
     private final GraphDatabaseQueryService graph;
@@ -57,10 +53,13 @@ public class Neo4jTransactionalContext implements TransactionalContext
     private final SecurityContext securityContext;
     private final ExecutingQuery executingQuery;
 
-    private InternalTransaction transaction;
+    /**
+     * Current transaction.
+     * Field can be read from a different thread in {@link #terminate()}.
+     */
+    private volatile InternalTransaction transaction;
     private Statement statement;
     private boolean isOpen = true;
-    private AtomicBoolean terminated = new AtomicBoolean(false);
 
     private long pageHits;
     private long pageMisses;
@@ -141,22 +140,13 @@ public void close( boolean success )
         }
     }
 
-    @Override // TODO: Make the state of this class a state machine that is a single value and maybe CAS state
-    // transitions
+    @Override
     public void terminate()
     {
-        if ( isOpen )
+        InternalTransaction currentTransaction = transaction;
+        if ( currentTransaction != null )
         {
-            terminated.set(true);
-            try
-            {
-                transaction.terminate();
-                isOpen = false;
-            }
-            catch ( NotInTransactionException e )
-            {
-                // Ok then. Nothing to do
-            }
+            currentTransaction.terminate();
         }
     }
 
@@ -243,9 +233,13 @@ public TransactionalContext getOrBeginNewIfClosed()
 
     private void checkNotTerminated()
     {
-        if ( terminated.get() )
+        InternalTransaction currentTransaction = transaction;
+        if ( currentTransaction != null )
         {
-            throw new TransactionTerminatedException( TransactionTerminated );
+            currentTransaction.terminationReason().ifPresent( status ->
+            {
+                throw new TransactionTerminatedException( status );
+            } );
         }
     }
 

diff --git a/community/kernel/src/test/java/org/neo4j/kernel/TestPlaceboTransaction.java b/community/kernel/src/test/java/org/neo4j/kernel/TestPlaceboTransaction.java
@@ -22,15 +22,21 @@
 import org.junit.Before;
 import org.junit.Test;
 
+import java.util.Optional;
+
 import org.neo4j.graphdb.Node;
 import org.neo4j.graphdb.Transaction;
 import org.neo4j.kernel.api.KernelTransaction;
 import org.neo4j.kernel.api.ReadOperations;
 import org.neo4j.kernel.api.Statement;
+import org.neo4j.kernel.api.exceptions.Status;
 import org.neo4j.kernel.impl.core.ThreadToStatementContextBridge;
 import org.neo4j.kernel.impl.coreapi.PlaceboTransaction;
 import org.neo4j.kernel.impl.locking.ResourceTypes;
 
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.times;
@@ -123,4 +129,21 @@ public void canAcquireWriteLock() throws Exception
         // then
         verify( readOps ).acquireExclusive( ResourceTypes.NODE, resource.getId() );
     }
+
+    @Test
+    public void shouldReturnTerminationReason()
+    {
+        KernelTransaction kernelTransaction = mock( KernelTransaction.class );
+        when( kernelTransaction.getReasonIfTerminated() ).thenReturn( Optional.empty() )
+                .thenReturn( Optional.of( Status.Transaction.Interrupted ) );
+
+        PlaceboTransaction tx = new PlaceboTransaction( () -> kernelTransaction, new ThreadToStatementContextBridge() );
+
+        Optional<Status> terminationReason1 = tx.terminationReason();
+        Optional<Status> terminationReason2 = tx.terminationReason();
+
+        assertFalse( terminationReason1.isPresent() );
+        assertTrue( terminationReason2.isPresent() );
+        assertEquals( Status.Transaction.Interrupted, terminationReason2.get() );
+    }
 }
diff --git a/community/kernel/src/test/java/org/neo4j/kernel/TopLevelTransactionTest.java b/community/kernel/src/test/java/org/neo4j/kernel/TopLevelTransactionTest.java
@@ -21,6 +21,8 @@
 
 import org.junit.Test;
 
+import java.util.Optional;
+
 import org.neo4j.graphdb.TransactionTerminatedException;
 import org.neo4j.graphdb.TransientDatabaseFailureException;
 import org.neo4j.graphdb.TransientFailureException;
@@ -32,8 +34,11 @@
 import org.neo4j.kernel.impl.coreapi.TopLevelTransaction;
 
 import static org.hamcrest.Matchers.instanceOf;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertSame;
 import static org.junit.Assert.assertThat;
+import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.doThrow;
@@ -131,4 +136,21 @@ public void shouldShowTransactionTerminatedExceptionAsTransient() throws Excepti
             assertSame( error, e.getCause() );
         }
     }
+
+    @Test
+    public void shouldReturnTerminationReason()
+    {
+        KernelTransaction kernelTransaction = mock( KernelTransaction.class );
+        when( kernelTransaction.getReasonIfTerminated() ).thenReturn( Optional.empty() )
+                .thenReturn( Optional.of( Status.Transaction.Terminated ) );
+
+        TopLevelTransaction tx = new TopLevelTransaction( kernelTransaction, new ThreadToStatementContextBridge() );
+
+        Optional<Status> terminationReason1 = tx.terminationReason();
+        Optional<Status> terminationReason2 = tx.terminationReason();
+
+        assertFalse( terminationReason1.isPresent() );
+        assertTrue( terminationReason2.isPresent() );
+        assertEquals( Status.Transaction.Terminated, terminationReason2.get() );
+    }
 }