From a91ed0f1ad19632bdbccd6c39d07d2801271c710 Mon Sep 17 00:00:00 2001 From: Lai Jiang Date: Thu, 24 Aug 2023 14:35:59 -0400 Subject: [PATCH] Allow nested transactions when per-transaction isolation level is on (#2121) It turns out that disallowing all nested transaction is impractical. So in this PR we make it possible to run nested transactions (which are not really nested as far as SQL is concerned, but rather lexically nested calls to tm().transact() which will NOT open new transactions when called within a transaction) as long as there is no conflict between the specified isolation levels between the enclosing and the enclosed transactions. Note that this will change the behavior of calling tm().transact() with no isolation level override, or a null override INSIDE a transaction. The lack of the override will allow the nested transaction to run at whatever level the enclosing transaction runs at, instead of at the default level specified in the config file. --- .../config/files/nomulus-config-unittest.yaml | 2 +- .../JpaTransactionManagerImpl.java | 15 +- .../JpaTransactionManagerImplTest.java | 134 +++++++++++++++--- 3 files changed, 127 insertions(+), 24 deletions(-) diff --git a/core/src/main/java/google/registry/config/files/nomulus-config-unittest.yaml b/core/src/main/java/google/registry/config/files/nomulus-config-unittest.yaml index d85705c88..74a05ea89 100644 --- a/core/src/main/java/google/registry/config/files/nomulus-config-unittest.yaml +++ b/core/src/main/java/google/registry/config/files/nomulus-config-unittest.yaml @@ -28,4 +28,4 @@ misc: transientFailureRetries: 3 hibernate: - perTransactionIsolation: false + perTransactionIsolation: true 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 a23fd75fb..56e7303fc 100644 --- a/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManagerImpl.java +++ b/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManagerImpl.java @@ -167,12 +167,17 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager { public T transactNoRetry( Supplier work, @Nullable TransactionIsolationLevel isolationLevel) { if (inTransaction()) { - if (getHibernatePerTransactionIsolationEnabled()) { - throw new IllegalStateException("Nested transaction detected"); - } else { - logger.atWarning().log("Nested transaction detected"); - return work.get(); + 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)); + } } + return work.get(); } TransactionInfo txnInfo = transactionInfo.get(); txnInfo.entityManager = emf.createEntityManager(); 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 6af3c0e50..8d6346a64 100644 --- a/core/src/test/java/google/registry/persistence/transaction/JpaTransactionManagerImplTest.java +++ b/core/src/test/java/google/registry/persistence/transaction/JpaTransactionManagerImplTest.java @@ -16,6 +16,7 @@ package google.registry.persistence.transaction; 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; import static google.registry.persistence.PersistenceModule.TransactionIsolationLevel.TRANSACTION_READ_UNCOMMITTED; import static google.registry.persistence.PersistenceModule.TransactionIsolationLevel.TRANSACTION_REPEATABLE_READ; import static google.registry.persistence.transaction.TransactionManagerFactory.tm; @@ -126,25 +127,122 @@ class JpaTransactionManagerImplTest { } @Test - void transact_nestedTransactions() { - // Unit tests always allows for per-transaction isolation level (and therefore throws when a - // nested transaction is detected). - if (RegistryConfig.getHibernatePerTransactionIsolationEnabled()) { - IllegalArgumentException e = - assertThrows( - IllegalArgumentException.class, - () -> - tm().transact( - () -> { - tm().transact(() -> {}); - })); - assertThat(e).hasMessageThat().isEqualTo("Nested transaction detected"); - } else { - tm().transact( - () -> { - tm().transact(() -> {}); - }); + 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( + 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"); + } + + @Test + void transact_nestedTransactions_perTransactionIsolationLevelDisabled() { + if (RegistryConfig.getHibernatePerTransactionIsolationEnabled()) { + return; + } + 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