Internalised marking for success in readTx & writeTx

This commit makes `Session#readTransaction()` and `Session#writeTransaction()`
automatically commit when transaction was not marked as failure and no
exception was thrown. This effectively removes the need to call `tx.success()`
as the last statement in a transaction.

Author: lutovich

driver/src/main/java/org/neo4j/driver/internal/NetworkSession.java

@@ -253,7 +253,21 @@ private synchronized <T> T transaction( AccessMode mode, TransactionWork<T> work
         {
             try ( Transaction tx = beginTransaction( mode ) )
             {
-                return work.execute( tx );
+                T result;
+                try
+                {
+                    result = work.execute( tx );
+                }
+                catch ( Throwable t )
+                {
+                    // mark transaction for failure if the given unit of work threw exception
+                    // this will override any success marks that were made by the unit of work
+                    tx.failure();
+                    throw t;
+                }
+                // given unit of work completed successfully, mark transaction for commit
+                tx.success();
+                return result;
             }
             catch ( Throwable newError )
             {

driver/src/main/java/org/neo4j/driver/v1/Config.java

@@ -29,7 +29,6 @@
 import org.neo4j.driver.v1.exceptions.ServiceUnavailableException;
 import org.neo4j.driver.v1.exceptions.SessionExpiredException;
 import org.neo4j.driver.v1.exceptions.TransientException;
-import org.neo4j.driver.v1.util.Function;
 import org.neo4j.driver.v1.util.Immutable;
 import org.neo4j.driver.v1.util.Resource;
 
@@ -449,10 +448,11 @@ public ConfigBuilder withConnectionTimeout( long value, TimeUnit unit )
         }
 
         /**
-         * Specify the maximum time transactions are allowed to retry via {@link Session#readTransaction(Function)} and
-         * {@link Session#writeTransaction(Function)} methods. These methods will retry the given unit of work on
-         * {@link ServiceUnavailableException}, {@link SessionExpiredException} and {@link TransientException} with
-         * exponential backoff using initial delay of 1 second.
+         * Specify the maximum time transactions are allowed to retry via
+         * {@link Session#readTransaction(TransactionWork)} and {@link Session#writeTransaction(TransactionWork)}
+         * methods. These methods will retry the given unit of work on {@link ServiceUnavailableException},
+         * {@link SessionExpiredException} and {@link TransientException} with exponential backoff using initial
+         * delay of 1 second.
          * <p>
          * Default value is 30 seconds.
          *

driver/src/main/java/org/neo4j/driver/v1/Session.java

@@ -84,6 +84,9 @@ public interface Session extends Resource, StatementRunner
 
     /**
      * Execute given unit of work in a  {@link AccessMode#READ read} transaction.
+     * <p>
+     * Transaction will automatically be committed unless exception is thrown from the unit of work itself or from
+     * {@link Transaction#close()} or transaction is explicitly marked for failure via {@link Transaction#failure()}.
      *
      * @param work the {@link TransactionWork} to be applied to a new read transaction.
      * @param <T> the return type of the given unit of work.
@@ -93,6 +96,9 @@ public interface Session extends Resource, StatementRunner
 
     /**
      * Execute given unit of work in a {@link AccessMode#WRITE write} transaction.
+     * <p>
+     * Transaction will automatically be committed unless exception is thrown from the unit of work itself or from
+     * {@link Transaction#close()} or transaction is explicitly marked for failure via {@link Transaction#failure()}.
      *
      * @param work the {@link TransactionWork} to be applied to a new write transaction.
      * @param <T> the return type of the given unit of work.

driver/src/test/java/org/neo4j/driver/v1/integration/SessionIT.java

@@ -55,6 +55,8 @@
 import static org.hamcrest.Matchers.greaterThan;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertThat;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
@@ -544,28 +546,297 @@ public void writeTxRetriedUntilFailure()
     }
 
     @Test
-    public void writeTxDoesNotCommitWhenMarkedForFailure()
+    public void readTxCommittedWithoutTxSuccess()
     {
-        try ( Session session = neo4j.driver().session() )
+        try ( Driver driver = newDriverWithoutRetries();
+              Session session = driver.session() )
         {
-            int answer = session.writeTransaction( new TransactionWork<Integer>()
+            assertNull( session.lastBookmark() );
+
+            long answer = session.readTransaction( new TransactionWork<Long>()
+            {
+                @Override
+                public Long execute( Transaction tx )
+                {
+                    return tx.run( "RETURN 42" ).single().get( 0 ).asLong();
+                }
+            } );
+            assertEquals( 42, answer );
+
+            // bookmark should be not-null after commit
+            assertNotNull( session.lastBookmark() );
+        }
+    }
+
+    @Test
+    public void writeTxCommittedWithoutTxSuccess()
+    {
+        try ( Driver driver = newDriverWithoutRetries() )
+        {
+            try ( Session session = driver.session() )
+            {
+                long answer = session.writeTransaction( new TransactionWork<Long>()
+                {
+                    @Override
+                    public Long execute( Transaction tx )
+                    {
+                        return tx.run( "CREATE (:Person {name: 'Thor Odinson'}) RETURN 42" ).single().get( 0 ).asLong();
+                    }
+                } );
+                assertEquals( 42, answer );
+            }
+
+            try ( Session session = driver.session() )
+            {
+                StatementResult result = session.run( "MATCH (p:Person {name: 'Thor Odinson'}) RETURN count(p)" );
+                assertEquals( 1, result.single().get( 0 ).asInt() );
+            }
+        }
+    }
+
+    @Test
+    public void readTxRolledBackWithTxFailure()
+    {
+        try ( Driver driver = newDriverWithoutRetries();
+              Session session = driver.session() )
+        {
+            assertNull( session.lastBookmark() );
+
+            long answer = session.readTransaction( new TransactionWork<Long>()
             {
                 @Override
-                public Integer execute( Transaction tx )
+                public Long execute( Transaction tx )
                 {
-                    tx.run( "CREATE (:Person {name: 'Natasha Romanoff'})" );
+                    StatementResult result = tx.run( "RETURN 42" );
                     tx.failure();
-                    return 42;
+                    return result.single().get( 0 ).asLong();
                 }
             } );
+            assertEquals( 42, answer );
+
+            // bookmark should remain null after rollback
+            assertNull( session.lastBookmark() );
+        }
+    }
+
+    @Test
+    public void writeTxRolledBackWithTxFailure()
+    {
+        try ( Driver driver = newDriverWithoutRetries() )
+        {
+            try ( Session session = driver.session() )
+            {
+                int answer = session.writeTransaction( new TransactionWork<Integer>()
+                {
+                    @Override
+                    public Integer execute( Transaction tx )
+                    {
+                        tx.run( "CREATE (:Person {name: 'Natasha Romanoff'})" );
+                        tx.failure();
+                        return 42;
+                    }
+                } );
+
+                assertEquals( 42, answer );
+            }
+
+            try ( Session session = driver.session() )
+            {
+                StatementResult result = session.run( "MATCH (p:Person {name: 'Natasha Romanoff'}) RETURN count(p)" );
+                assertEquals( 0, result.single().get( 0 ).asInt() );
+            }
+        }
+    }
+
+    @Test
+    public void readTxRolledBackWhenExceptionIsThrown()
+    {
+        try ( Driver driver = newDriverWithoutRetries();
+              Session session = driver.session() )
+        {
+            assertNull( session.lastBookmark() );
+
+            try
+            {
+                session.readTransaction( new TransactionWork<Long>()
+                {
+                    @Override
+                    public Long execute( Transaction tx )
+                    {
+                        StatementResult result = tx.run( "RETURN 42" );
+                        if ( result.single().get( 0 ).asLong() == 42 )
+                        {
+                            throw new IllegalStateException();
+                        }
+                        return 1L;
+                    }
+                } );
+                fail( "Exception expected" );
+            }
+            catch ( Exception e )
+            {
+                assertThat( e, instanceOf( IllegalStateException.class ) );
+            }
+
+            // bookmark should remain null after rollback
+            assertNull( session.lastBookmark() );
+        }
+    }
 
+    @Test
+    public void writeTxRolledBackWhenExceptionIsThrown()
+    {
+        try ( Driver driver = newDriverWithoutRetries() )
+        {
+            try ( Session session = driver.session() )
+            {
+                try
+                {
+                    session.writeTransaction( new TransactionWork<Integer>()
+                    {
+                        @Override
+                        public Integer execute( Transaction tx )
+                        {
+                            tx.run( "CREATE (:Person {name: 'Loki Odinson'})" );
+                            throw new IllegalStateException();
+                        }
+                    } );
+                    fail( "Exception expected" );
+                }
+                catch ( Exception e )
+                {
+                    assertThat( e, instanceOf( IllegalStateException.class ) );
+                }
+            }
+
+            try ( Session session = driver.session() )
+            {
+                StatementResult result = session.run( "MATCH (p:Person {name: 'Natasha Romanoff'}) RETURN count(p)" );
+                assertEquals( 0, result.single().get( 0 ).asInt() );
+            }
+        }
+    }
+
+    @Test
+    public void readTxRolledBackWhenMarkedBothSuccessAndFailure()
+    {
+        try ( Driver driver = newDriverWithoutRetries();
+              Session session = driver.session() )
+        {
+            assertNull( session.lastBookmark() );
+
+            long answer = session.readTransaction( new TransactionWork<Long>()
+            {
+                @Override
+                public Long execute( Transaction tx )
+                {
+                    StatementResult result = tx.run( "RETURN 42" );
+                    tx.success();
+                    tx.failure();
+                    return result.single().get( 0 ).asLong();
+                }
+            } );
             assertEquals( 42, answer );
+
+            // bookmark should remain null after rollback
+            assertNull( session.lastBookmark() );
         }
+    }
 
-        try ( Session session = neo4j.driver().session() )
+    @Test
+    public void writeTxRolledBackWhenMarkedBothSuccessAndFailure()
+    {
+        try ( Driver driver = newDriverWithoutRetries() )
         {
-            StatementResult result = session.run( "MATCH (p:Person {name: 'Natasha Romanoff'}) RETURN count(p)" );
-            assertEquals( 0, result.single().get( 0 ).asInt() );
+            try ( Session session = driver.session() )
+            {
+                int answer = session.writeTransaction( new TransactionWork<Integer>()
+                {
+                    @Override
+                    public Integer execute( Transaction tx )
+                    {
+                        tx.run( "CREATE (:Person {name: 'Natasha Romanoff'})" );
+                        tx.success();
+                        tx.failure();
+                        return 42;
+                    }
+                } );
+
+                assertEquals( 42, answer );
+            }
+
+            try ( Session session = driver.session() )
+            {
+                StatementResult result = session.run( "MATCH (p:Person {name: 'Natasha Romanoff'}) RETURN count(p)" );
+                assertEquals( 0, result.single().get( 0 ).asInt() );
+            }
+        }
+    }
+
+    @Test
+    public void readTxRolledBackWhenMarkedAsSuccessAndThrowsException()
+    {
+        try ( Driver driver = newDriverWithoutRetries();
+              Session session = driver.session() )
+        {
+            assertNull( session.lastBookmark() );
+
+            try
+            {
+                session.readTransaction( new TransactionWork<Long>()
+                {
+                    @Override
+                    public Long execute( Transaction tx )
+                    {
+                        tx.run( "RETURN 42" );
+                        tx.success();
+                        throw new IllegalStateException();
+                    }
+                } );
+                fail( "Exception expected" );
+            }
+            catch ( Exception e )
+            {
+                assertThat( e, instanceOf( IllegalStateException.class ) );
+            }
+
+            // bookmark should remain null after rollback
+            assertNull( session.lastBookmark() );
+        }
+    }
+
+    @Test
+    public void writeTxRolledBackWhenMarkedAsSuccessAndThrowsException()
+    {
+        try ( Driver driver = newDriverWithoutRetries() )
+        {
+            try ( Session session = driver.session() )
+            {
+                try
+                {
+                    session.writeTransaction( new TransactionWork<Integer>()
+                    {
+                        @Override
+                        public Integer execute( Transaction tx )
+                        {
+                            tx.run( "CREATE (:Person {name: 'Natasha Romanoff'})" );
+                            tx.success();
+                            throw new IllegalStateException();
+                        }
+                    } );
+                    fail( "Exception expected" );
+                }
+                catch ( Exception e )
+                {
+                    assertThat( e, instanceOf( IllegalStateException.class ) );
+                }
+            }
+
+            try ( Session session = driver.session() )
+            {
+                StatementResult result = session.run( "MATCH (p:Person {name: 'Natasha Romanoff'}) RETURN count(p)" );
+                assertEquals( 0, result.single().get( 0 ).asInt() );
+            }
         }
     }
 
@@ -668,6 +939,11 @@ public Void execute( Transaction tx )
         }
     }
 
+    private Driver newDriverWithoutRetries()
+    {
+        return newDriverWithFixedRetries( 0 );
+    }
+
     private Driver newDriverWithFixedRetries( int maxRetriesCount )
     {
         DriverFactory driverFactory = new DriverFactoryWithFixedRetryLogic( maxRetriesCount );