diff --git a/core/src/main/java/com/scalar/db/common/StateManagedDistributedTransactionManager.java b/core/src/main/java/com/scalar/db/common/StateManagedDistributedTransactionManager.java index 1dad240f24..b2034765bb 100644 --- a/core/src/main/java/com/scalar/db/common/StateManagedDistributedTransactionManager.java +++ b/core/src/main/java/com/scalar/db/common/StateManagedDistributedTransactionManager.java @@ -146,9 +146,12 @@ public void commit() throws CommitException, UnknownTransactionStatusException { @Override public void rollback() throws RollbackException { - if (status == Status.COMMITTED || status == Status.ROLLED_BACK) { + if (status == Status.ROLLED_BACK) { + return; + } + if (status == Status.COMMITTED) { throw new IllegalStateException( - CoreError.TRANSACTION_ALREADY_COMMITTED_OR_ROLLED_BACK.buildMessage(status)); + CoreError.TRANSACTION_ALREADY_COMMITTED.buildMessage(status)); } try { super.rollback(); @@ -159,9 +162,12 @@ public void rollback() throws RollbackException { @Override public void abort() throws AbortException { - if (status == Status.COMMITTED || status == Status.ROLLED_BACK) { + if (status == Status.ROLLED_BACK) { + return; + } + if (status == Status.COMMITTED) { throw new IllegalStateException( - CoreError.TRANSACTION_ALREADY_COMMITTED_OR_ROLLED_BACK.buildMessage(status)); + CoreError.TRANSACTION_ALREADY_COMMITTED.buildMessage(status)); } try { super.abort(); diff --git a/core/src/main/java/com/scalar/db/common/StateManagedTwoPhaseCommitTransactionManager.java b/core/src/main/java/com/scalar/db/common/StateManagedTwoPhaseCommitTransactionManager.java index 8e59e13a50..57aa36c9a2 100644 --- a/core/src/main/java/com/scalar/db/common/StateManagedTwoPhaseCommitTransactionManager.java +++ b/core/src/main/java/com/scalar/db/common/StateManagedTwoPhaseCommitTransactionManager.java @@ -183,9 +183,12 @@ public void commit() throws CommitException, UnknownTransactionStatusException { @Override public void rollback() throws RollbackException { - if (status == Status.COMMITTED || status == Status.ROLLED_BACK) { + if (status == Status.ROLLED_BACK) { + return; + } + if (status == Status.COMMITTED) { throw new IllegalStateException( - CoreError.TRANSACTION_ALREADY_COMMITTED_OR_ROLLED_BACK.buildMessage(status)); + CoreError.TRANSACTION_ALREADY_COMMITTED.buildMessage(status)); } try { super.rollback(); @@ -196,9 +199,12 @@ public void rollback() throws RollbackException { @Override public void abort() throws AbortException { - if (status == Status.COMMITTED || status == Status.ROLLED_BACK) { + if (status == Status.ROLLED_BACK) { + return; + } + if (status == Status.COMMITTED) { throw new IllegalStateException( - CoreError.TRANSACTION_ALREADY_COMMITTED_OR_ROLLED_BACK.buildMessage(status)); + CoreError.TRANSACTION_ALREADY_COMMITTED.buildMessage(status)); } try { super.abort(); diff --git a/core/src/main/java/com/scalar/db/common/error/CoreError.java b/core/src/main/java/com/scalar/db/common/error/CoreError.java index 3998073243..4244b52064 100644 --- a/core/src/main/java/com/scalar/db/common/error/CoreError.java +++ b/core/src/main/java/com/scalar/db/common/error/CoreError.java @@ -214,10 +214,10 @@ public enum CoreError implements ScalarDbError { Category.USER_ERROR, "0042", "Invalid ID specified. ID: %d", "", ""), TRANSACTION_NOT_ACTIVE( Category.USER_ERROR, "0043", "The transaction is not active. Status: %s", "", ""), - TRANSACTION_ALREADY_COMMITTED_OR_ROLLED_BACK( + TRANSACTION_ALREADY_COMMITTED( Category.USER_ERROR, "0044", - "The transaction has already been committed or rolled back. Status: %s", + "The transaction has already been committed. Status: %s", "", ""), TRANSACTION_NOT_PREPARED( diff --git a/core/src/test/java/com/scalar/db/common/StateManagedDistributedTransactionManagerTest.java b/core/src/test/java/com/scalar/db/common/StateManagedDistributedTransactionManagerTest.java index 96c78262ea..6f1efc08e7 100644 --- a/core/src/test/java/com/scalar/db/common/StateManagedDistributedTransactionManagerTest.java +++ b/core/src/test/java/com/scalar/db/common/StateManagedDistributedTransactionManagerTest.java @@ -7,6 +7,8 @@ import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.only; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import com.scalar.db.api.Delete; @@ -21,6 +23,7 @@ import com.scalar.db.api.SerializableStrategy; import com.scalar.db.api.Update; import com.scalar.db.api.Upsert; +import com.scalar.db.exception.transaction.AbortException; import com.scalar.db.exception.transaction.CommitException; import com.scalar.db.exception.transaction.RollbackException; import com.scalar.db.exception.transaction.TransactionException; @@ -308,12 +311,56 @@ public void rollback_AfterCommit_ShouldThrowIllegalStateException() } @Test - public void rollback_Twice_ShouldThrowIllegalStateException() throws RollbackException { + public void rollback_Twice_ShouldNotThrowAnyExceptionAndCallWrappedTransactionRollbackOnlyOnce() + throws RollbackException { // Arrange transaction.rollback(); // Act Assert - assertThatThrownBy(() -> transaction.rollback()).isInstanceOf(IllegalStateException.class); + assertThatCode(() -> transaction.rollback()).doesNotThrowAnyException(); + + verify(wrappedTransaction, only()).rollback(); + } + + @Test + public void abort_ShouldNotThrowAnyException() { + // Arrange + + // Act Assert + assertThatCode(() -> transaction.abort()).doesNotThrowAnyException(); + } + + @Test + public void abort_AfterCommitFailed_ShouldNotThrowAnyException() + throws CommitException, UnknownTransactionStatusException { + // Arrange + doThrow(CommitException.class).when(wrappedTransaction).commit(); + assertThatThrownBy(() -> transaction.commit()).isInstanceOf(CommitException.class); + + // Act Assert + assertThatCode(() -> transaction.abort()).doesNotThrowAnyException(); + } + + @Test + public void abort_AfterCommit_ShouldThrowIllegalStateException() + throws CommitException, UnknownTransactionStatusException { + // Arrange + transaction.commit(); + + // Act Assert + assertThatThrownBy(() -> transaction.abort()).isInstanceOf(IllegalStateException.class); + } + + @Test + public void abort_Twice_ShouldNotThrowAnyExceptionAndCallWrappedTransactionAbortOnlyOnce() + throws AbortException { + // Arrange + transaction.abort(); + + // Act Assert + assertThatCode(() -> transaction.abort()).doesNotThrowAnyException(); + + verify(wrappedTransaction, only()).abort(); } } } diff --git a/core/src/test/java/com/scalar/db/common/StateManagedTwoPhaseCommitTransactionManagerTest.java b/core/src/test/java/com/scalar/db/common/StateManagedTwoPhaseCommitTransactionManagerTest.java index 85824b5d2c..e1e01722a9 100644 --- a/core/src/test/java/com/scalar/db/common/StateManagedTwoPhaseCommitTransactionManagerTest.java +++ b/core/src/test/java/com/scalar/db/common/StateManagedTwoPhaseCommitTransactionManagerTest.java @@ -6,6 +6,8 @@ import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.only; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import com.scalar.db.api.Delete; @@ -18,6 +20,7 @@ import com.scalar.db.api.TwoPhaseCommitTransactionManager; import com.scalar.db.api.Update; import com.scalar.db.api.Upsert; +import com.scalar.db.exception.transaction.AbortException; import com.scalar.db.exception.transaction.CommitException; import com.scalar.db.exception.transaction.PreparationException; import com.scalar.db.exception.transaction.RollbackException; @@ -385,12 +388,82 @@ public void rollback_AfterCommit_ShouldThrowIllegalStateException() } @Test - public void rollback_Twice_ShouldThrowIllegalStateException() throws RollbackException { + public void rollback_Twice_ShouldNotThrowAnyExceptionAndCallWrappedTransactionRollbackOnlyOnce() + throws RollbackException { // Arrange transaction.rollback(); // Act Assert - assertThatThrownBy(() -> transaction.rollback()).isInstanceOf(IllegalStateException.class); + assertThatCode(() -> transaction.rollback()).doesNotThrowAnyException(); + + verify(wrappedTransaction, only()).rollback(); + } + + @Test + public void abort_ShouldNotThrowAnyException() { + // Arrange + + // Act Assert + assertThatCode(() -> transaction.abort()).doesNotThrowAnyException(); + } + + @Test + public void abort_AfterCommitFailed_ShouldNotThrowAnyException() + throws PreparationException, CommitException, UnknownTransactionStatusException { + // Arrange + doThrow(CommitException.class).when(wrappedTransaction).commit(); + + transaction.prepare(); + assertThatThrownBy(() -> transaction.commit()).isInstanceOf(CommitException.class); + + // Act Assert + assertThatCode(() -> transaction.abort()).doesNotThrowAnyException(); + } + + @Test + public void abort_AfterPrepareFailed_ShouldNotThrowAnyException() throws PreparationException { + // Arrange + doThrow(PreparationException.class).when(wrappedTransaction).prepare(); + assertThatThrownBy(() -> transaction.prepare()).isInstanceOf(PreparationException.class); + + // Act Assert + assertThatCode(() -> transaction.abort()).doesNotThrowAnyException(); + } + + @Test + public void abort_AfterValidateFailed_ShouldNotThrowAnyException() + throws PreparationException, ValidationException { + // Arrange + doThrow(ValidationException.class).when(wrappedTransaction).validate(); + + transaction.prepare(); + assertThatThrownBy(() -> transaction.validate()).isInstanceOf(ValidationException.class); + + // Act Assert + assertThatCode(() -> transaction.abort()).doesNotThrowAnyException(); + } + + @Test + public void abort_AfterCommit_ShouldThrowIllegalStateException() + throws PreparationException, CommitException, UnknownTransactionStatusException { + // Arrange + transaction.prepare(); + transaction.commit(); + + // Act Assert + assertThatThrownBy(() -> transaction.abort()).isInstanceOf(IllegalStateException.class); + } + + @Test + public void abort_Twice_ShouldNotThrowAnyExceptionAndCallWrappedTransactionAbortOnlyOnce() + throws AbortException { + // Arrange + transaction.abort(); + + // Act Assert + assertThatCode(() -> transaction.abort()).doesNotThrowAnyException(); + + verify(wrappedTransaction, only()).abort(); } } }