From a2da391da75c7ae6637d51b02b3b768044adff70 Mon Sep 17 00:00:00 2001 From: brfrn169 Date: Tue, 10 Jun 2025 16:41:46 +0900 Subject: [PATCH] Allow multiple calls to rollback() --- ...eManagedDistributedTransactionManager.java | 14 +++- ...nagedTwoPhaseCommitTransactionManager.java | 14 +++- .../com/scalar/db/common/error/CoreError.java | 4 +- ...agedDistributedTransactionManagerTest.java | 51 +++++++++++- ...dTwoPhaseCommitTransactionManagerTest.java | 77 ++++++++++++++++++- 5 files changed, 146 insertions(+), 14 deletions(-) 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 52866cb405..259f8286c7 100644 --- a/core/src/main/java/com/scalar/db/common/StateManagedDistributedTransactionManager.java +++ b/core/src/main/java/com/scalar/db/common/StateManagedDistributedTransactionManager.java @@ -144,9 +144,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(); @@ -157,9 +160,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 1d79240d04..e5fd4a5691 100644 --- a/core/src/main/java/com/scalar/db/common/StateManagedTwoPhaseCommitTransactionManager.java +++ b/core/src/main/java/com/scalar/db/common/StateManagedTwoPhaseCommitTransactionManager.java @@ -181,9 +181,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(); @@ -194,9 +197,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 f6c02711b5..79116fd589 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(); } } }