diff --git a/core/src/main/java/google/registry/beam/common/RegistryJpaIO.java b/core/src/main/java/google/registry/beam/common/RegistryJpaIO.java index 599ae866a..7be467b40 100644 --- a/core/src/main/java/google/registry/beam/common/RegistryJpaIO.java +++ b/core/src/main/java/google/registry/beam/common/RegistryJpaIO.java @@ -209,7 +209,7 @@ public final class RegistryJpaIO { @ProcessElement public void processElement(OutputReceiver outputReceiver) { - tm().transactNoRetry( + tm().transact( () -> { query.stream().map(resultMapper::apply).forEach(outputReceiver::output); }); diff --git a/core/src/main/java/google/registry/config/RegistryConfig.java b/core/src/main/java/google/registry/config/RegistryConfig.java index 7e994a921..3a6a2295a 100644 --- a/core/src/main/java/google/registry/config/RegistryConfig.java +++ b/core/src/main/java/google/registry/config/RegistryConfig.java @@ -1548,9 +1548,9 @@ public final class RegistryConfig { return CONFIG_SETTINGS.get().hibernate.connectionIsolation; } - /** Returns true if per-transaction isolation level is enabled. */ - public static boolean getHibernatePerTransactionIsolationEnabled() { - return CONFIG_SETTINGS.get().hibernate.perTransactionIsolation; + /** Returns true if nested calls to {@code tm().transact()} are allowed. */ + public static boolean getHibernateAllowNestedTransactions() { + return CONFIG_SETTINGS.get().hibernate.allowNestedTransactions; } /** Returns true if hibernate.show_sql is enabled. */ diff --git a/core/src/main/java/google/registry/config/RegistryConfigSettings.java b/core/src/main/java/google/registry/config/RegistryConfigSettings.java index cd9dc3c39..efcfe14e6 100644 --- a/core/src/main/java/google/registry/config/RegistryConfigSettings.java +++ b/core/src/main/java/google/registry/config/RegistryConfigSettings.java @@ -113,7 +113,7 @@ public class RegistryConfigSettings { /** Configuration for Hibernate. */ public static class Hibernate { - public boolean perTransactionIsolation; + public boolean allowNestedTransactions; public String connectionIsolation; public String logSqlQueries; public String hikariConnectionTimeout; diff --git a/core/src/main/java/google/registry/config/files/default-config.yaml b/core/src/main/java/google/registry/config/files/default-config.yaml index 21cbc840a..83c944053 100644 --- a/core/src/main/java/google/registry/config/files/default-config.yaml +++ b/core/src/main/java/google/registry/config/files/default-config.yaml @@ -189,11 +189,13 @@ registryPolicy: sunriseDomainCreateDiscount: 0.15 hibernate: - # Make it possible to specify the isolation level for each transaction. If set - # to true, nested transactions will throw an exception. If set to false, a - # transaction with the isolation override specified will still execute at the - # default level (specified below). - perTransactionIsolation: true + # If set to false, calls to tm().transact() cannot be nested. If set to true, + # nested calls to tm().transact() are allowed, as long as they do not specify + # a transaction isolation level override. These nested transactions should + # either be refactored to non-nested transactions, or changed to + # tm().reTransact(), which explicitly allows nested transactions, but does not + # allow setting an isolation level override. + allowNestedTransactions: true # Make 'SERIALIZABLE' the default isolation level to ensure correctness. # diff --git a/core/src/main/java/google/registry/model/server/Lock.java b/core/src/main/java/google/registry/model/server/Lock.java index d1aeffb20..43524d627 100644 --- a/core/src/main/java/google/registry/model/server/Lock.java +++ b/core/src/main/java/google/registry/model/server/Lock.java @@ -25,9 +25,10 @@ import com.google.common.base.Strings; import com.google.common.flogger.FluentLogger; import google.registry.model.ImmutableObject; import google.registry.persistence.VKey; +import google.registry.persistence.transaction.TransactionManager.ThrowingRunnable; import java.io.Serializable; import java.util.Optional; -import java.util.function.Supplier; +import java.util.concurrent.Callable; import javax.annotation.Nullable; import javax.persistence.Column; import javax.persistence.Entity; @@ -184,7 +185,7 @@ public class Lock extends ImmutableObject implements Serializable { public static Optional acquire( String resourceName, @Nullable String tld, Duration leaseLength) { String scope = tld != null ? tld : GLOBAL; - Supplier lockAcquirer = + Callable lockAcquirer = () -> { DateTime now = tm().getTransactionTime(); @@ -221,7 +222,7 @@ public class Lock extends ImmutableObject implements Serializable { /** Release the lock. */ public void release() { // Just use the default clock because we aren't actually doing anything that will use the clock. - Supplier lockReleaser = + ThrowingRunnable lockReleaser = () -> { // To release a lock, check that no one else has already obtained it and if not // delete it. If the lock in the database was different, then this lock is gone already; @@ -246,7 +247,6 @@ public class Lock extends ImmutableObject implements Serializable { logger.atInfo().log( "Not deleting lock: %s - someone else has it: %s", lockId, loadedLock); } - return null; }; tm().transact(lockReleaser); } diff --git a/core/src/main/java/google/registry/persistence/transaction/DatabaseException.java b/core/src/main/java/google/registry/persistence/transaction/DatabaseException.java index 90f9e7564..695f55203 100644 --- a/core/src/main/java/google/registry/persistence/transaction/DatabaseException.java +++ b/core/src/main/java/google/registry/persistence/transaction/DatabaseException.java @@ -62,7 +62,7 @@ class DatabaseException extends PersistenceException { *

If the {@code original Throwable} has at least one {@link SQLException} in its chain of * causes, a {@link DatabaseException} is thrown; otherwise this does nothing. */ - static void tryWrapAndThrow(Throwable original) { + static void throwIfSqlException(Throwable original) { Throwable t = original; do { if (t instanceof SQLException) { diff --git a/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManager.java b/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManager.java index a383ee08d..848112711 100644 --- a/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManager.java +++ b/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManager.java @@ -16,7 +16,6 @@ package google.registry.persistence.transaction; import google.registry.persistence.PersistenceModule.TransactionIsolationLevel; import google.registry.persistence.VKey; -import java.util.function.Supplier; import javax.persistence.EntityManager; import javax.persistence.Query; import javax.persistence.TypedQuery; @@ -62,24 +61,6 @@ public interface JpaTransactionManager extends TransactionManager { */ Query query(String sqlString); - /** Executes the work in a transaction with no retries and returns the result. */ - T transactNoRetry(Supplier work); - - /** - * Executes the work in a transaction at the given {@link TransactionIsolationLevel} with no - * retries and returns the result. - */ - T transactNoRetry(Supplier work, TransactionIsolationLevel isolationLevel); - - /** Executes the work in a transaction with no retries. */ - void transactNoRetry(Runnable work); - - /** - * Executes the work in a transaction at the given {@link TransactionIsolationLevel} with no - * retries. - */ - void transactNoRetry(Runnable work, TransactionIsolationLevel isolationLevel); - /** Deletes the entity by its id, throws exception if the entity is not deleted. */ void assertDelete(VKey key); @@ -103,7 +84,4 @@ public interface JpaTransactionManager extends TransactionManager { /** Return the {@link TransactionIsolationLevel} used in the current transaction. */ TransactionIsolationLevel getCurrentTransactionIsolationLevel(); - - /** Asserts that the current transaction runs at the given level. */ - void assertTransactionIsolationLevel(TransactionIsolationLevel expectedLevel); } diff --git a/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManagerImpl.java b/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManagerImpl.java index ecb725f15..58e0a566f 100644 --- a/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManagerImpl.java +++ b/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManagerImpl.java @@ -15,11 +15,12 @@ package google.registry.persistence.transaction; import static com.google.common.base.Preconditions.checkArgument; +import static com.google.common.base.Throwables.throwIfUnchecked; import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.collect.ImmutableMap.toImmutableMap; import static com.google.common.collect.ImmutableSet.toImmutableSet; -import static google.registry.config.RegistryConfig.getHibernatePerTransactionIsolationEnabled; -import static google.registry.persistence.transaction.DatabaseException.tryWrapAndThrow; +import static google.registry.config.RegistryConfig.getHibernateAllowNestedTransactions; +import static google.registry.persistence.transaction.DatabaseException.throwIfSqlException; import static google.registry.util.PreconditionsUtils.checkArgumentNotNull; import static java.util.AbstractMap.SimpleEntry; import static java.util.stream.Collectors.joining; @@ -31,6 +32,7 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Streams; import com.google.common.flogger.FluentLogger; +import com.google.common.flogger.StackSize; import google.registry.model.ImmutableObject; import google.registry.persistence.JpaRetries; import google.registry.persistence.PersistenceModule.TransactionIsolationLevel; @@ -52,7 +54,7 @@ import java.util.Map; import java.util.NoSuchElementException; import java.util.Optional; import java.util.Set; -import java.util.function.Supplier; +import java.util.concurrent.Callable; import java.util.stream.Stream; import java.util.stream.StreamSupport; import javax.annotation.Nullable; @@ -76,6 +78,9 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager { private static final FluentLogger logger = FluentLogger.forEnclosingClass(); private static final Retrier retrier = new Retrier(new SystemSleeper(), 3); + private static final String NESTED_TRANSACTION_MESSAGE = + "Nested transaction detected. Try refactoring to avoid nested transactions. If unachievable," + + " use reTransact() in nested transactions"; // EntityManagerFactory is thread safe. private final EntityManagerFactory emf; @@ -138,21 +143,23 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager { } @Override - public void assertTransactionIsolationLevel(TransactionIsolationLevel expectedLevel) { - assertInTransaction(); - TransactionIsolationLevel currentLevel = getCurrentTransactionIsolationLevel(); - if (currentLevel != expectedLevel) { - throw new IllegalStateException( - String.format( - "Current transaction isolation level (%s) is not as expected (%s)", - currentLevel, expectedLevel)); + public T reTransact(Callable work) { + // This prevents inner transaction from retrying, thus avoiding a cascade retry effect. + if (inTransaction()) { + return transactNoRetry(work, null); } + return retrier.callWithRetry( + () -> transactNoRetry(work, null), JpaRetries::isFailedTxnRetriable); } @Override - public T transact(Supplier work, TransactionIsolationLevel isolationLevel) { - // This prevents inner transaction from retrying, thus avoiding a cascade retry effect. + public T transact(Callable work, TransactionIsolationLevel isolationLevel) { if (inTransaction()) { + if (!getHibernateAllowNestedTransactions()) { + throw new IllegalStateException(NESTED_TRANSACTION_MESSAGE); + } + logger.atWarning().withStackTrace(StackSize.MEDIUM).log(NESTED_TRANSACTION_MESSAGE); + // This prevents inner transaction from retrying, thus avoiding a cascade retry effect. return transactNoRetry(work, isolationLevel); } return retrier.callWithRetry( @@ -160,30 +167,32 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager { } @Override - public T reTransact(Supplier work) { - return transact(work); - } - - @Override - public T transact(Supplier work) { + public T transact(Callable work) { return transact(work, null); } - @Override public T transactNoRetry( - Supplier work, @Nullable TransactionIsolationLevel isolationLevel) { + Callable work, @Nullable TransactionIsolationLevel isolationLevel) { if (inTransaction()) { - if (isolationLevel != null && getHibernatePerTransactionIsolationEnabled()) { - TransactionIsolationLevel enclosingLevel = getCurrentTransactionIsolationLevel(); - if (isolationLevel != enclosingLevel) { - throw new IllegalStateException( - String.format( - "Isolation level conflict detected in nested transactions.\n" - + "Enclosing transaction: %s\nCurrent transaction: %s", - enclosingLevel, isolationLevel)); - } + // This check will no longer be necessary when the transact() method always throws + // inside a nested transaction, as the only way to pass a non-null isolation level + // is by calling the transact() method (and its variants), which would have already + // thrown before calling transactNoRetry() when inside a nested transaction. + // + // For now, we still need it, so we don't accidentally call a nested transact() with an + // isolation level override. This buys us time to detect nested transact() calls and either + // remove them or change the call site to reTransact(). + if (isolationLevel != null) { + throw new IllegalStateException( + "Transaction isolation level cannot be specified for nested transactions"); + } + try { + return work.call(); + } catch (Exception e) { + throwIfSqlException(e); + throwIfUnchecked(e); + throw new RuntimeException(e); } - return work.get(); } TransactionInfo txnInfo = transactionInfo.get(); txnInfo.entityManager = emf.createEntityManager(); @@ -191,43 +200,36 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager { try { txn.begin(); txnInfo.start(clock); - if (isolationLevel != null) { - if (getHibernatePerTransactionIsolationEnabled()) { - getEntityManager() - .createNativeQuery( - String.format("SET TRANSACTION ISOLATION LEVEL %s", isolationLevel.getMode())) - .executeUpdate(); - logger.atInfo().log("Running transaction at %s", isolationLevel); - } else { - logger.atWarning().log( - "Per-transaction isolation level disabled, but %s was requested", isolationLevel); - } + if (isolationLevel != null && isolationLevel != getDefaultTransactionIsolationLevel()) { + getEntityManager() + .createNativeQuery( + String.format("SET TRANSACTION ISOLATION LEVEL %s", isolationLevel.getMode())) + .executeUpdate(); + logger.atInfo().log( + "Overriding transaction isolation level from %s to %s", + getDefaultTransactionIsolationLevel(), isolationLevel); } - T result = work.get(); + T result = work.call(); txn.commit(); return result; - } catch (RuntimeException | Error e) { - // Error is unchecked! + } catch (Throwable e) { + // Catch a Throwable here so even Errors would lead to a rollback. try { txn.rollback(); logger.atWarning().log("Error during transaction; transaction rolled back."); - } catch (Throwable rollbackException) { + } catch (Exception rollbackException) { logger.atSevere().withCause(rollbackException).log("Rollback failed; suppressing error."); } - tryWrapAndThrow(e); - throw e; + throwIfSqlException(e); + throwIfUnchecked(e); + throw new RuntimeException(e); } finally { txnInfo.clear(); } } @Override - public T transactNoRetry(Supplier work) { - return transactNoRetry(work, null); - } - - @Override - public void transact(Runnable work, TransactionIsolationLevel isolationLevel) { + public void transact(ThrowingRunnable work, TransactionIsolationLevel isolationLevel) { transact( () -> { work.run(); @@ -237,28 +239,17 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager { } @Override - public void reTransact(Runnable work) { - transact(work); - } - - @Override - public void transact(Runnable work) { + public void transact(ThrowingRunnable work) { transact(work, null); } @Override - public void transactNoRetry(Runnable work, TransactionIsolationLevel isolationLevel) { - transactNoRetry( + public void reTransact(ThrowingRunnable work) { + reTransact( () -> { work.run(); return null; - }, - isolationLevel); - } - - @Override - public void transactNoRetry(Runnable work) { - transactNoRetry(work, null); + }); } @Override diff --git a/core/src/main/java/google/registry/persistence/transaction/TransactionManager.java b/core/src/main/java/google/registry/persistence/transaction/TransactionManager.java index cf38a6b1f..020b194fa 100644 --- a/core/src/main/java/google/registry/persistence/transaction/TransactionManager.java +++ b/core/src/main/java/google/registry/persistence/transaction/TransactionManager.java @@ -22,7 +22,7 @@ import google.registry.persistence.PersistenceModule.TransactionIsolationLevel; import google.registry.persistence.VKey; import java.util.NoSuchElementException; import java.util.Optional; -import java.util.function.Supplier; +import java.util.concurrent.Callable; import java.util.stream.Stream; import org.joda.time.DateTime; @@ -48,51 +48,51 @@ public interface TransactionManager { void assertInTransaction(); /** Executes the work in a transaction and returns the result. */ - T transact(Supplier work); + T transact(Callable work); /** * Executes the work in a transaction at the given {@link TransactionIsolationLevel} and returns * the result. */ - T transact(Supplier work, TransactionIsolationLevel isolationLevel); + T transact(Callable work, TransactionIsolationLevel isolationLevel); /** * Executes the work in a (potentially wrapped) transaction and returns the result. * *

Calls to this method are typically going to be in inner functions, that are called either as - * top-level transactions themselves or are nested inside of larger transactions (e.g. a + * top-level transactions themselves or are nested inside larger transactions (e.g. a * transactional flow). Invocations of reTransact must be vetted to occur in both situations and * with such complexity that it is not trivial to refactor out the nested transaction calls. New * code should be written in such a way as to avoid requiring reTransact in the first place. * - *

In the future we will be enforcing that {@link #transact(Supplier)} calls be top-level only, + *

In the future we will be enforcing that {@link #transact(Callable)} calls be top-level only, * with reTransact calls being the only ones that can potentially be an inner nested transaction * (which is a noop). Note that, as this can be a nested inner exception, there is no overload * provided to specify a (potentially conflicting) transaction isolation level. */ - T reTransact(Supplier work); + T reTransact(Callable work); /** Executes the work in a transaction. */ - void transact(Runnable work); + void transact(ThrowingRunnable work); /** Executes the work in a transaction at the given {@link TransactionIsolationLevel}. */ - void transact(Runnable work, TransactionIsolationLevel isolationLevel); + void transact(ThrowingRunnable work, TransactionIsolationLevel isolationLevel); /** * Executes the work in a (potentially wrapped) transaction and returns the result. * *

Calls to this method are typically going to be in inner functions, that are called either as - * top-level transactions themselves or are nested inside of larger transactions (e.g. a + * top-level transactions themselves or are nested inside larger transactions (e.g. a * transactional flow). Invocations of reTransact must be vetted to occur in both situations and * with such complexity that it is not trivial to refactor out the nested transaction calls. New * code should be written in such a way as to avoid requiring reTransact in the first place. * - *

In the future we will be enforcing that {@link #transact(Runnable)} calls be top-level only, - * with reTransact calls being the only ones that can potentially be an inner nested transaction - * (which is a noop). Note that, as this can be a nested inner exception, there is no overload * - * provided to specify a (potentially conflicting) transaction isolation level. + *

In the future we will be enforcing that {@link #transact(ThrowingRunnable)} calls be + * top-level only, with reTransact calls being the only ones that can potentially be an inner + * nested transaction (which is a noop). Note that, as this can be a nested inner exception, there + * is no overload provided to specify a (potentially conflicting) transaction isolation level. */ - void reTransact(Runnable work); + void reTransact(ThrowingRunnable work); /** Returns the time associated with the start of this particular transaction attempt. */ DateTime getTransactionTime(); @@ -216,4 +216,15 @@ public interface TransactionManager { /** Returns a QueryComposer which can be used to perform queries against the current database. */ QueryComposer createQueryComposer(Class entity); + + /** + * A runnable that allows for checked exceptions to be thrown. + * + *

This makes it easier to write lambdas without having to worry about wrapping and re-throwing + * checked excpetions as unchecked ones. + */ + @FunctionalInterface + interface ThrowingRunnable { + void run() throws Exception; + } } diff --git a/core/src/test/java/google/registry/flows/FlowRunnerTest.java b/core/src/test/java/google/registry/flows/FlowRunnerTest.java index 8d620ba4f..76c8732d4 100644 --- a/core/src/test/java/google/registry/flows/FlowRunnerTest.java +++ b/core/src/test/java/google/registry/flows/FlowRunnerTest.java @@ -31,7 +31,6 @@ import com.google.common.base.Splitter; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSortedMap; import com.google.common.testing.TestLogHandler; -import google.registry.config.RegistryConfig; import google.registry.flows.certs.CertificateChecker; import google.registry.model.eppcommon.Trid; import google.registry.model.eppoutput.EppOutput.ResponseOrGreeting; @@ -89,8 +88,8 @@ class FlowRunnerTest { @Override public ResponseOrGreeting run() { - tm().assertTransactionIsolationLevel( - isolationLevel.orElse(tm().getDefaultTransactionIsolationLevel())); + assertThat(tm().getCurrentTransactionIsolationLevel()) + .isEqualTo(isolationLevel.orElse(tm().getDefaultTransactionIsolationLevel())); return mock(EppResponse.class); } } @@ -136,10 +135,8 @@ class FlowRunnerTest { Optional.of(TransactionIsolationLevel.TRANSACTION_READ_UNCOMMITTED); flowRunner.flowClass = TestTransactionalFlow.class; flowRunner.flowProvider = () -> new TestTransactionalFlow(flowRunner.isolationLevelOverride); - if (RegistryConfig.getHibernatePerTransactionIsolationEnabled()) { - flowRunner.run(eppMetricBuilder); - assertThat(eppMetricBuilder.build().getCommandName()).hasValue("TestTransactional"); - } + flowRunner.run(eppMetricBuilder); + assertThat(eppMetricBuilder.build().getCommandName()).hasValue("TestTransactional"); } @Test diff --git a/core/src/test/java/google/registry/persistence/transaction/DatabaseExceptionTest.java b/core/src/test/java/google/registry/persistence/transaction/DatabaseExceptionTest.java index b958a4fa0..ef27a27cd 100644 --- a/core/src/test/java/google/registry/persistence/transaction/DatabaseExceptionTest.java +++ b/core/src/test/java/google/registry/persistence/transaction/DatabaseExceptionTest.java @@ -18,7 +18,7 @@ import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth8.assertThat; import static google.registry.persistence.transaction.DatabaseException.getSqlError; import static google.registry.persistence.transaction.DatabaseException.getSqlExceptionDetails; -import static google.registry.persistence.transaction.DatabaseException.tryWrapAndThrow; +import static google.registry.persistence.transaction.DatabaseException.throwIfSqlException; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.times; @@ -99,13 +99,13 @@ public class DatabaseExceptionTest { @Test void tryWrapAndThrow_notSQLException() { RuntimeException orig = new RuntimeException(new Exception()); - tryWrapAndThrow(orig); + throwIfSqlException(orig); } @Test void tryWrapAndThrow_hasSQLException() { Throwable orig = new Throwable(new SQLException()); - assertThrows(DatabaseException.class, () -> tryWrapAndThrow(orig)); + assertThrows(DatabaseException.class, () -> throwIfSqlException(orig)); } @Test diff --git a/core/src/test/java/google/registry/persistence/transaction/JpaTransactionManagerImplTest.java b/core/src/test/java/google/registry/persistence/transaction/JpaTransactionManagerImplTest.java index 454529965..d5bcf09e0 100644 --- a/core/src/test/java/google/registry/persistence/transaction/JpaTransactionManagerImplTest.java +++ b/core/src/test/java/google/registry/persistence/transaction/JpaTransactionManagerImplTest.java @@ -14,6 +14,7 @@ package google.registry.persistence.transaction; +import static com.google.common.base.Preconditions.checkState; import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.truth.Truth.assertThat; import static google.registry.persistence.PersistenceModule.TransactionIsolationLevel.TRANSACTION_READ_COMMITTED; @@ -28,6 +29,7 @@ import static google.registry.testing.TestDataHelper.fileClassPath; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.mockStatic; import static org.mockito.Mockito.spy; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; @@ -36,6 +38,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import google.registry.config.RegistryConfig; import google.registry.model.ImmutableObject; +import google.registry.persistence.PersistenceModule.TransactionIsolationLevel; import google.registry.persistence.VKey; import google.registry.persistence.transaction.JpaTestExtensions.JpaUnitTestExtension; import google.registry.testing.DatabaseHelper; @@ -43,7 +46,6 @@ import google.registry.testing.FakeClock; import java.io.Serializable; import java.math.BigInteger; import java.util.NoSuchElementException; -import java.util.function.Supplier; import javax.persistence.Entity; import javax.persistence.EntityManager; import javax.persistence.Id; @@ -53,6 +55,8 @@ import javax.persistence.PersistenceException; import javax.persistence.RollbackException; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; +import org.junit.jupiter.api.function.Executable; +import org.mockito.MockedStatic; /** * Unit tests for SQL only APIs defined in {@link JpaTransactionManagerImpl}. Note that the tests @@ -94,7 +98,7 @@ class JpaTransactionManagerImplTest { insertPerson(10); insertCompany("Foo"); insertCompany("Bar"); - tm().assertTransactionIsolationLevel(tm().getDefaultTransactionIsolationLevel()); + assertTransactionIsolationLevel(tm().getDefaultTransactionIsolationLevel()); }); assertPersonCount(1); assertPersonExist(10); @@ -105,145 +109,98 @@ class JpaTransactionManagerImplTest { @Test void transact_setIsolationLevel() { + // If not specified, run at the default isolation level. tm().transact( - () -> { - tm().assertTransactionIsolationLevel( - RegistryConfig.getHibernatePerTransactionIsolationEnabled() - ? TRANSACTION_READ_UNCOMMITTED - : tm().getDefaultTransactionIsolationLevel()); - return null; - }, + () -> assertTransactionIsolationLevel(tm().getDefaultTransactionIsolationLevel()), + null); + tm().transact( + () -> assertTransactionIsolationLevel(TRANSACTION_READ_UNCOMMITTED), TRANSACTION_READ_UNCOMMITTED); - // Make sure that we can start a new transaction on the same thread with a different isolation - // level. + // Make sure that we can start a new transaction on the same thread at a different level. tm().transact( - () -> { - tm().assertTransactionIsolationLevel( - RegistryConfig.getHibernatePerTransactionIsolationEnabled() - ? TRANSACTION_REPEATABLE_READ - : tm().getDefaultTransactionIsolationLevel()); - return null; - }, + () -> assertTransactionIsolationLevel(TRANSACTION_REPEATABLE_READ), TRANSACTION_REPEATABLE_READ); } @Test - void transact_nestedTransactions_perTransactionIsolationLevelEnabled() { - if (!RegistryConfig.getHibernatePerTransactionIsolationEnabled()) { - return; - } - // Nested transactions allowed (both at the default isolation level). - tm().transact( - () -> { - tm().assertTransactionIsolationLevel(tm().getDefaultTransactionIsolationLevel()); - tm().transact( - () -> { - tm().assertTransactionIsolationLevel( + void transact_nestedTransactions_disabled() { + try (MockedStatic config = mockStatic(RegistryConfig.class)) { + config.when(RegistryConfig::getHibernateAllowNestedTransactions).thenReturn(false); + // transact() not allowed in nested transactions. + IllegalStateException thrown = + assertThrows( + IllegalStateException.class, + () -> + tm().transact( + () -> { + assertTransactionIsolationLevel( tm().getDefaultTransactionIsolationLevel()); - }); - }); - // Nested transactions allowed (enclosed transaction does not have an override, using the - // enclosing transaction's level). - tm().transact( - () -> { - tm().assertTransactionIsolationLevel(TRANSACTION_READ_UNCOMMITTED); - tm().transact( - () -> { - tm().assertTransactionIsolationLevel(TRANSACTION_READ_UNCOMMITTED); - }); - }, - TRANSACTION_READ_UNCOMMITTED); - // Nested transactions allowed (Both have the same override). - tm().transact( - () -> { - tm().assertTransactionIsolationLevel(TRANSACTION_REPEATABLE_READ); - tm().transact( - () -> { - tm().assertTransactionIsolationLevel(TRANSACTION_REPEATABLE_READ); - }, - TRANSACTION_REPEATABLE_READ); - }, - TRANSACTION_REPEATABLE_READ); - // Nested transactions disallowed (enclosed transaction has an override that conflicts from the - // default). - IllegalStateException e = - assertThrows( - IllegalStateException.class, - () -> - tm().transact( - () -> { - tm().transact(() -> {}, TRANSACTION_READ_COMMITTED); - })); - assertThat(e).hasMessageThat().contains("conflict detected"); - // Nested transactions disallowed (conflicting overrides). - e = - assertThrows( - IllegalStateException.class, - () -> - tm().transact( - () -> { - tm().transact(() -> {}, TRANSACTION_READ_COMMITTED); - }, - TRANSACTION_REPEATABLE_READ)); - assertThat(e).hasMessageThat().contains("conflict detected"); + tm().transact(() -> null); + })); + assertThat(thrown).hasMessageThat().contains("Nested transaction detected"); + // reTransact() allowed in nested transactions. + tm().transact( + () -> { + assertTransactionIsolationLevel(tm().getDefaultTransactionIsolationLevel()); + tm().reTransact( + () -> + assertTransactionIsolationLevel( + tm().getDefaultTransactionIsolationLevel())); + }); + // reTransact() respects enclosing transaction's isolation level. + tm().transact( + () -> { + assertTransactionIsolationLevel(TRANSACTION_READ_UNCOMMITTED); + tm().reTransact( + () -> assertTransactionIsolationLevel(TRANSACTION_READ_UNCOMMITTED)); + }, + TRANSACTION_READ_UNCOMMITTED); + } } @Test - void transact_nestedTransactions_perTransactionIsolationLevelDisabled() { - if (RegistryConfig.getHibernatePerTransactionIsolationEnabled()) { - return; + void transact_nestedTransactions_enabled() { + try (MockedStatic config = mockStatic(RegistryConfig.class)) { + config.when(RegistryConfig::getHibernateAllowNestedTransactions).thenReturn(true); + // transact() allowed in nested transactions. + tm().transact( + () -> { + assertTransactionIsolationLevel(tm().getDefaultTransactionIsolationLevel()); + tm().reTransact( + () -> + assertTransactionIsolationLevel( + tm().getDefaultTransactionIsolationLevel())); + }); + // transact() not allowed in nested transactions if isolation level is specified. + IllegalStateException thrown = + assertThrows( + IllegalStateException.class, + () -> + tm().transact( + () -> { + assertTransactionIsolationLevel( + tm().getDefaultTransactionIsolationLevel()); + tm().transact(() -> null, TRANSACTION_READ_COMMITTED); + })); + assertThat(thrown).hasMessageThat().contains("cannot be specified"); + // reTransact() allowed in nested transactions. + tm().transact( + () -> { + assertTransactionIsolationLevel(tm().getDefaultTransactionIsolationLevel()); + tm().reTransact( + () -> + assertTransactionIsolationLevel( + tm().getDefaultTransactionIsolationLevel())); + }); + // reTransact() respects enclosing transaction's isolation level. + tm().transact( + () -> { + assertTransactionIsolationLevel(TRANSACTION_READ_UNCOMMITTED); + tm().reTransact( + () -> assertTransactionIsolationLevel(TRANSACTION_READ_UNCOMMITTED)); + }, + TRANSACTION_READ_UNCOMMITTED); } - tm().transact( - () -> { - tm().assertTransactionIsolationLevel(tm().getDefaultTransactionIsolationLevel()); - tm().transact( - () -> { - tm().assertTransactionIsolationLevel( - tm().getDefaultTransactionIsolationLevel()); - }); - }); - tm().transact( - () -> { - tm().assertTransactionIsolationLevel(tm().getDefaultTransactionIsolationLevel()); - tm().transact( - () -> { - tm().assertTransactionIsolationLevel( - tm().getDefaultTransactionIsolationLevel()); - }); - }, - TRANSACTION_READ_UNCOMMITTED); - tm().transact( - () -> { - tm().assertTransactionIsolationLevel(tm().getDefaultTransactionIsolationLevel()); - tm().transact( - () -> { - tm().assertTransactionIsolationLevel( - tm().getDefaultTransactionIsolationLevel()); - }, - TRANSACTION_READ_UNCOMMITTED); - }); - tm().transact( - () -> { - tm().assertTransactionIsolationLevel(tm().getDefaultTransactionIsolationLevel()); - tm().transact( - () -> { - tm().assertTransactionIsolationLevel( - tm().getDefaultTransactionIsolationLevel()); - }, - TRANSACTION_READ_UNCOMMITTED); - }, - TRANSACTION_READ_UNCOMMITTED); - tm().transact( - () -> { - tm().assertTransactionIsolationLevel(tm().getDefaultTransactionIsolationLevel()); - tm().transact( - () -> { - tm().assertTransactionIsolationLevel( - tm().getDefaultTransactionIsolationLevel()); - }, - TRANSACTION_READ_COMMITTED); - }, - TRANSACTION_READ_UNCOMMITTED); } @Test @@ -299,32 +256,55 @@ class JpaTransactionManagerImplTest { OptimisticLockException.class, () -> spyJpaTm.transact(() -> spyJpaTm.delete(theEntityKey))); verify(spyJpaTm, times(3)).delete(theEntityKey); - Supplier supplier = - () -> { - Runnable work = () -> spyJpaTm.delete(theEntityKey); - work.run(); - return null; - }; - assertThrows(OptimisticLockException.class, () -> spyJpaTm.transact(supplier)); + assertThrows( + OptimisticLockException.class, + () -> spyJpaTm.transact(() -> spyJpaTm.delete(theEntityKey))); verify(spyJpaTm, times(6)).delete(theEntityKey); } @Test - void transactNoRetry_doesNotRetryOptimisticLockException() { - JpaTransactionManager spyJpaTm = spy(tm()); - doThrow(OptimisticLockException.class).when(spyJpaTm).delete(any(VKey.class)); - spyJpaTm.transactNoRetry(() -> spyJpaTm.insert(theEntity)); - assertThrows( - OptimisticLockException.class, - () -> spyJpaTm.transactNoRetry(() -> spyJpaTm.delete(theEntityKey))); - verify(spyJpaTm, times(1)).delete(theEntityKey); - Supplier supplier = + void transactNoRetry_nested() { + JpaTransactionManagerImpl tm = (JpaTransactionManagerImpl) tm(); + // Calling transactNoRetry() without an isolation level override inside a transaction is fine. + tm.transact( () -> { - Runnable work = () -> spyJpaTm.delete(theEntityKey); - work.run(); + tm.transactNoRetry( + () -> { + assertTransactionIsolationLevel(tm.getDefaultTransactionIsolationLevel()); + return null; + }, + null); + }); + // Calling transactNoRetry() with an isolation level override inside a transaction is not + // allowed. + IllegalStateException thrown = + assertThrows( + IllegalStateException.class, + () -> tm.transact(() -> tm.transactNoRetry(() -> null, TRANSACTION_READ_UNCOMMITTED))); + assertThat(thrown).hasMessageThat().contains("cannot be specified"); + } + + @Test + void transactNoRetry_doesNotRetryOptimisticLockException() { + JpaTransactionManagerImpl spyJpaTm = spy((JpaTransactionManagerImpl) tm()); + doThrow(OptimisticLockException.class).when(spyJpaTm).delete(any(VKey.class)); + spyJpaTm.transactNoRetry( + () -> { + spyJpaTm.insert(theEntity); return null; - }; - assertThrows(OptimisticLockException.class, () -> spyJpaTm.transactNoRetry(supplier)); + }, + null); + Executable transaction = + () -> + spyJpaTm.transactNoRetry( + () -> { + spyJpaTm.delete(theEntityKey); + return null; + }, + null); + assertThrows(OptimisticLockException.class, transaction); + verify(spyJpaTm, times(1)).delete(theEntityKey); + assertThrows(OptimisticLockException.class, transaction); verify(spyJpaTm, times(2)).delete(theEntityKey); } @@ -338,13 +318,8 @@ class JpaTransactionManagerImplTest { assertThrows( RuntimeException.class, () -> spyJpaTm.transact(() -> spyJpaTm.delete(theEntityKey))); verify(spyJpaTm, times(3)).delete(theEntityKey); - Supplier supplier = - () -> { - Runnable work = () -> spyJpaTm.delete(theEntityKey); - work.run(); - return null; - }; - assertThrows(RuntimeException.class, () -> spyJpaTm.transact(supplier)); + assertThrows( + RuntimeException.class, () -> spyJpaTm.transact(() -> spyJpaTm.delete(theEntityKey))); verify(spyJpaTm, times(6)).delete(theEntityKey); } @@ -740,20 +715,13 @@ class JpaTransactionManagerImplTest { doThrow(OptimisticLockException.class).when(spyJpaTm).delete(any(VKey.class)); spyJpaTm.transact(() -> spyJpaTm.insert(theEntity)); - Supplier supplier = - () -> { - Runnable work = () -> spyJpaTm.delete(theEntityKey); - work.run(); - return null; - }; - assertThrows( OptimisticLockException.class, () -> spyJpaTm.transact( () -> { spyJpaTm.exists(theEntity); - spyJpaTm.transact(supplier); + spyJpaTm.transact(() -> spyJpaTm.delete(theEntityKey)); })); verify(spyJpaTm, times(3)).exists(theEntity); @@ -814,6 +782,16 @@ class JpaTransactionManagerImplTest { assertCompanyCount(0); } + private static void assertTransactionIsolationLevel(TransactionIsolationLevel expectedLevel) { + tm().assertInTransaction(); + TransactionIsolationLevel currentLevel = tm().getCurrentTransactionIsolationLevel(); + checkState( + currentLevel == expectedLevel, + "Current transaction isolation level (%s) is not as expected (%s)", + currentLevel, + expectedLevel); + } + private static int countTable(String tableName) { return tm().transact( () -> { diff --git a/core/src/test/java/google/registry/persistence/transaction/ReplicaSimulatingJpaTransactionManager.java b/core/src/test/java/google/registry/persistence/transaction/ReplicaSimulatingJpaTransactionManager.java index c51dff7e5..69d1c28f1 100644 --- a/core/src/test/java/google/registry/persistence/transaction/ReplicaSimulatingJpaTransactionManager.java +++ b/core/src/test/java/google/registry/persistence/transaction/ReplicaSimulatingJpaTransactionManager.java @@ -14,6 +14,9 @@ package google.registry.persistence.transaction; +import static com.google.common.base.Throwables.throwIfUnchecked; +import static google.registry.persistence.transaction.DatabaseException.throwIfSqlException; + import com.google.common.collect.ImmutableCollection; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; @@ -21,7 +24,7 @@ import google.registry.model.ImmutableObject; import google.registry.persistence.PersistenceModule.TransactionIsolationLevel; import google.registry.persistence.VKey; import java.util.Optional; -import java.util.function.Supplier; +import java.util.concurrent.Callable; import java.util.stream.Stream; import javax.persistence.EntityManager; import javax.persistence.Query; @@ -60,11 +63,6 @@ public class ReplicaSimulatingJpaTransactionManager implements JpaTransactionMan return delegate.getCurrentTransactionIsolationLevel(); } - @Override - public void assertTransactionIsolationLevel(TransactionIsolationLevel expectedLevel) { - delegate.assertTransactionIsolationLevel(expectedLevel); - } - @Override public EntityManager getStandaloneEntityManager() { return delegate.getStandaloneEntityManager(); @@ -101,9 +99,15 @@ public class ReplicaSimulatingJpaTransactionManager implements JpaTransactionMan } @Override - public T transact(Supplier work, TransactionIsolationLevel isolationLevel) { - if (delegate.inTransaction()) { - return work.get(); + public T transact(Callable work, TransactionIsolationLevel isolationLevel) { + if (inTransaction()) { + try { + return work.call(); + } catch (Exception e) { + throwIfSqlException(e); + throwIfUnchecked(e); + throw new RuntimeException(e); + } } return delegate.transact( () -> { @@ -111,33 +115,23 @@ public class ReplicaSimulatingJpaTransactionManager implements JpaTransactionMan .getEntityManager() .createNativeQuery("SET TRANSACTION READ ONLY") .executeUpdate(); - return work.get(); + return work.call(); }, isolationLevel); } @Override - public T reTransact(Supplier work) { + public T reTransact(Callable work) { return transact(work); } @Override - public T transact(Supplier work) { + public T transact(Callable work) { return transact(work, null); } @Override - public T transactNoRetry(Supplier work, TransactionIsolationLevel isolationLevel) { - return transact(work, isolationLevel); - } - - @Override - public T transactNoRetry(Supplier work) { - return transactNoRetry(work, null); - } - - @Override - public void transact(Runnable work, TransactionIsolationLevel isolationLevel) { + public void transact(ThrowingRunnable work, TransactionIsolationLevel isolationLevel) { transact( () -> { work.run(); @@ -147,25 +141,15 @@ public class ReplicaSimulatingJpaTransactionManager implements JpaTransactionMan } @Override - public void reTransact(Runnable work) { + public void reTransact(ThrowingRunnable work) { transact(work); } @Override - public void transact(Runnable work) { + public void transact(ThrowingRunnable work) { transact(work, null); } - @Override - public void transactNoRetry(Runnable work, TransactionIsolationLevel isolationLevel) { - transact(work, isolationLevel); - } - - @Override - public void transactNoRetry(Runnable work) { - transactNoRetry(work, null); - } - @Override public DateTime getTransactionTime() { return delegate.getTransactionTime();