Dont use unmatched calls to block/unblock

neo4j · Jul 1, 2016 · 21cc721 · 21cc721
1 parent 805197c
commit 21cc721
Show file tree

Hide file tree

Showing 3 changed files with 53 additions and 43 deletions.
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
@@ -300,15 +300,17 @@ public void blockNewTransactions()
 
     /**
      * Allow new transactions to be started again if current thread is the one who called
-     * {@link #blockNewTransactions()}. If current thread is not the one that called {@link #blockNewTransactions()}
-     * or it has not been called at all then NO_OP.
+     * {@link #blockNewTransactions()}.
+     *
+     * @throws IllegalStateException if current thread is not the one that called {@link #blockNewTransactions()}.
      */
     public void unblockNewTransactions()
     {
-        if ( newTransactionsLock.isWriteLockedByCurrentThread() )
+        if ( !newTransactionsLock.writeLock().isHeldByCurrentThread() )
         {
-            newTransactionsLock.writeLock().unlock();
+            throw new IllegalStateException( "This thread did not block transactions previously" );
         }
+        newTransactionsLock.writeLock().unlock();
     }
 
     private void assertCurrentThreadIsNotBlockingNewTransactions()

diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/api/KernelTransactionsTest.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/api/KernelTransactionsTest.java
@@ -25,6 +25,7 @@
 
 import java.util.concurrent.Callable;
 import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutionException;
 import java.util.concurrent.Executors;
 import java.util.concurrent.Future;
 import java.util.concurrent.ThreadLocalRandom;
@@ -70,7 +71,6 @@
 import static org.hamcrest.CoreMatchers.not;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
-import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertSame;
 import static org.junit.Assert.assertThat;
 import static org.junit.Assert.assertTrue;
@@ -289,7 +289,7 @@ public void blockNewTransactions() throws Exception
     }
 
     @Test
-    public void unblockNewTransactionsFromWrongThreadDoesNothing() throws Exception
+    public void unblockNewTransactionsFromWrongThreadThrows() throws Exception
     {
         KernelTransactions kernelTransactions = newKernelTransactions();
         kernelTransactions.blockNewTransactions();
@@ -302,7 +302,15 @@ public void unblockNewTransactionsFromWrongThreadDoesNothing() throws Exception
 
         Future<?> wrongUnblocker = unblockTxsInSeparateThread( kernelTransactions );
 
-        assertDone( wrongUnblocker );
+        try
+        {
+            wrongUnblocker.get( 2, TimeUnit.SECONDS );
+        }
+        catch ( Exception e )
+        {
+            assertThat( e, instanceOf( ExecutionException.class ) );
+            assertThat( e.getCause(), instanceOf( IllegalStateException.class ) );
+        }
         assertNotDone( txOpener );
 
         kernelTransactions.unblockNewTransactions();
@@ -420,11 +428,6 @@ private void await( CountDownLatch latch ) throws InterruptedException
         assertTrue( latch.await( 1, MINUTES ) );
     }
 
-    private static void assertDone( Future<?> future ) throws Exception
-    {
-        assertNull( future.get( 2, TimeUnit.SECONDS ) );
-    }
-
     private static void assertNotDone( Future<?> future )
     {
         try

diff --git a/...rise/com/src/main/java/org/neo4j/com/storecopy/TransactionCommittingResponseUnpacker.java b/...rise/com/src/main/java/org/neo4j/com/storecopy/TransactionCommittingResponseUnpacker.java
@@ -339,49 +339,54 @@ private void applyQueuedTransactionsIfNeeded() throws IOException
             return;
         }
 
-        /**
-         * Problem 1 (Not really a problem but we thought it where)
-         *  - chunk of batch is smaller than safe zone
-         *  - tx started after activeTransactions() is called
-         *  is safe because those transactions will see the latest state of store before chunk is applied and
-         *  because chunk is smaller than safe zone we are guarantied to not see two different states of any record
-         *  when applying the chunk.
-         *
-         *    activeTransactions() is called
-         *    |        start committing chunk
-         * ---|----+---|--|------> TIME
-         *         |      |
-         *         |      Start applying chunk
-         *         New tx starts here. Does not get terminated because not among active transactions, this is safe.
-         *
-         * Problem 2
-         *  - chunk of batch is larger than safe zone
-         *  - tx started after activeTransactions() but before apply
-         *
-         *    activeTransactions() is called
-         *    |        start committing chunk
-         * ---|--------|+-|------> TIME
-         *              | |
-         *              | Start applying chunk
-         *              New tx starts here. Does not get terminated because not among active transactions, but will read
-         *              outdated data and can be affected by reuse contamination.
+        /*
+          Problem 1 (Not really a problem but we thought it where)
+           - chunk of batch is smaller than safe zone
+           - tx started after activeTransactions() is called
+           is safe because those transactions will see the latest state of store before chunk is applied and
+           because chunk is smaller than safe zone we are guarantied to not see two different states of any record
+           when applying the chunk.
+
+             activeTransactions() is called
+             |        start committing chunk
+          ---|----+---|--|------> TIME
+                  |      |
+                  |      Start applying chunk
+                  New tx starts here. Does not get terminated because not among active transactions, this is safe.
+
+          Problem 2
+           - chunk of batch is larger than safe zone
+           - tx started after activeTransactions() but before apply
+
+             activeTransactions() is called
+             |        start committing chunk
+          ---|--------|+-|------> TIME
+                       | |
+                       | Start applying chunk
+                       New tx starts here. Does not get terminated because not among active transactions, but will read
+                       outdated data and can be affected by reuse contamination.
          */
 
         // We stop new transactions from starting to avoid problem 1
         if ( batchSizeExceedsSafeZone() )
         {
             // Problem 2
             kernelTransactions.blockNewTransactions();
+            try
+            {
+                markUnsafeTransactionsForTermination();
+                applyQueuedTransactions();
+            }
+            finally
+            {
+                kernelTransactions.unblockNewTransactions();
+            }
         }
-        try
+        else
         {
             markUnsafeTransactionsForTermination();
             applyQueuedTransactions();
         }
-        finally
-        {
-            kernelTransactions.unblockNewTransactions();
-        }
     }
 
     private boolean batchSizeExceedsSafeZone()