diff --git a/core/src/main/java/google/registry/dns/RefreshDnsAction.java b/core/src/main/java/google/registry/dns/RefreshDnsAction.java index 36f0e914b..1be7b6855 100644 --- a/core/src/main/java/google/registry/dns/RefreshDnsAction.java +++ b/core/src/main/java/google/registry/dns/RefreshDnsAction.java @@ -60,18 +60,21 @@ public final class RefreshDnsAction implements Runnable { if (!domainOrHostName.contains(".")) { throw new BadRequestException("URL parameter 'name' must be fully qualified"); } - switch (type) { - case DOMAIN: - loadAndVerifyExistence(Domain.class, domainOrHostName); - tm().transact(() -> requestDomainDnsRefresh(domainOrHostName)); - break; - case HOST: - verifyHostIsSubordinate(loadAndVerifyExistence(Host.class, domainOrHostName)); - tm().transact(() -> requestHostDnsRefresh(domainOrHostName)); - break; - default: - throw new BadRequestException("Unsupported type: " + type); - } + tm().transact( + () -> { + switch (type) { + case DOMAIN: + loadAndVerifyExistence(Domain.class, domainOrHostName); + requestDomainDnsRefresh(domainOrHostName); + break; + case HOST: + verifyHostIsSubordinate(loadAndVerifyExistence(Host.class, domainOrHostName)); + requestHostDnsRefresh(domainOrHostName); + break; + default: + throw new BadRequestException("Unsupported type: " + type); + } + }); } private diff --git a/core/src/main/java/google/registry/flows/domain/DomainClaimsCheckFlow.java b/core/src/main/java/google/registry/flows/domain/DomainClaimsCheckFlow.java index 22fc86885..d707fcbae 100644 --- a/core/src/main/java/google/registry/flows/domain/DomainClaimsCheckFlow.java +++ b/core/src/main/java/google/registry/flows/domain/DomainClaimsCheckFlow.java @@ -23,6 +23,7 @@ import static google.registry.flows.domain.DomainFlowUtils.validateDomainNameWit import static google.registry.flows.domain.DomainFlowUtils.verifyClaimsPeriodNotEnded; import static google.registry.flows.domain.DomainFlowUtils.verifyNotInPredelegation; import static google.registry.model.domain.launch.LaunchPhase.CLAIMS; +import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; @@ -108,7 +109,8 @@ public final class DomainClaimsCheckFlow implements Flow { verifyClaimsPeriodNotEnded(tld, now); } } - Optional claimKey = ClaimsListDao.get().getClaimKey(parsedDomain.parts().get(0)); + Optional claimKey = + tm().transact(() -> ClaimsListDao.get().getClaimKey(parsedDomain.parts().get(0))); launchChecksBuilder.add( LaunchCheck.create( LaunchCheckName.create(claimKey.isPresent(), domainName), claimKey.orElse(null))); diff --git a/core/src/main/java/google/registry/flows/poll/PollAckFlow.java b/core/src/main/java/google/registry/flows/poll/PollAckFlow.java index eb8aad97a..7d16c5cb8 100644 --- a/core/src/main/java/google/registry/flows/poll/PollAckFlow.java +++ b/core/src/main/java/google/registry/flows/poll/PollAckFlow.java @@ -108,7 +108,7 @@ public final class PollAckFlow implements TransactionalFlow { // acked, then we return a special status code indicating that. Note that the query will // include the message being acked. - int messageCount = tm().transact(() -> getPollMessageCount(registrarId, now)); + int messageCount = getPollMessageCount(registrarId, now); if (messageCount <= 0) { return responseBuilder.setResultFromCode(SUCCESS_WITH_NO_MESSAGES).build(); } diff --git a/core/src/main/java/google/registry/flows/poll/PollFlowUtils.java b/core/src/main/java/google/registry/flows/poll/PollFlowUtils.java index 5f7064234..c494cc9d7 100644 --- a/core/src/main/java/google/registry/flows/poll/PollFlowUtils.java +++ b/core/src/main/java/google/registry/flows/poll/PollFlowUtils.java @@ -30,13 +30,12 @@ public final class PollFlowUtils { /** Returns the number of poll messages for the given registrar that are not in the future. */ public static int getPollMessageCount(String registrarId, DateTime now) { - return tm().transact(() -> createPollMessageQuery(registrarId, now).count()).intValue(); + return (int) createPollMessageQuery(registrarId, now).count(); } /** Returns the first (by event time) poll message not in the future for this registrar. */ public static Optional getFirstPollMessage(String registrarId, DateTime now) { - return tm().transact( - () -> createPollMessageQuery(registrarId, now).orderBy("eventTime").first()); + return createPollMessageQuery(registrarId, now).orderBy("eventTime").first(); } /** diff --git a/core/src/main/java/google/registry/flows/poll/PollRequestFlow.java b/core/src/main/java/google/registry/flows/poll/PollRequestFlow.java index 06c8e3194..55f331d5c 100644 --- a/core/src/main/java/google/registry/flows/poll/PollRequestFlow.java +++ b/core/src/main/java/google/registry/flows/poll/PollRequestFlow.java @@ -20,6 +20,7 @@ import static google.registry.flows.poll.PollFlowUtils.getPollMessageCount; import static google.registry.model.eppoutput.Result.Code.SUCCESS_WITH_ACK_MESSAGE; import static google.registry.model.eppoutput.Result.Code.SUCCESS_WITH_NO_MESSAGES; import static google.registry.model.poll.PollMessageExternalKeyConverter.makePollMessageExternalId; +import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import google.registry.flows.EppException; import google.registry.flows.EppException.ParameterValueSyntaxErrorException; @@ -31,7 +32,6 @@ import google.registry.model.eppoutput.EppResponse; import google.registry.model.poll.MessageQueueInfo; import google.registry.model.poll.PollMessage; import google.registry.model.poll.PollMessageExternalKeyConverter; -import google.registry.util.Clock; import java.util.Optional; import javax.inject.Inject; import org.joda.time.DateTime; @@ -52,7 +52,6 @@ public final class PollRequestFlow implements Flow { @Inject ExtensionManager extensionManager; @Inject @RegistrarId String registrarId; @Inject @PollMessageId String messageId; - @Inject Clock clock; @Inject EppResponse.Builder responseBuilder; @Inject PollRequestFlow() {} @@ -63,24 +62,28 @@ public final class PollRequestFlow implements Flow { if (!messageId.isEmpty()) { throw new UnexpectedMessageIdException(); } + // Return the oldest message from the queue. - DateTime now = clock.nowUtc(); - Optional maybePollMessage = getFirstPollMessage(registrarId, now); - if (!maybePollMessage.isPresent()) { - return responseBuilder.setResultFromCode(SUCCESS_WITH_NO_MESSAGES).build(); - } - PollMessage pollMessage = maybePollMessage.get(); - return responseBuilder - .setResultFromCode(SUCCESS_WITH_ACK_MESSAGE) - .setMessageQueueInfo( - new MessageQueueInfo.Builder() - .setQueueDate(pollMessage.getEventTime()) - .setMsg(pollMessage.getMsg()) - .setQueueLength(getPollMessageCount(registrarId, now)) - .setMessageId(makePollMessageExternalId(pollMessage)) - .build()) - .setMultipleResData(pollMessage.getResponseData()) - .build(); + return tm().transact( + () -> { + DateTime now = tm().getTransactionTime(); + Optional maybePollMessage = getFirstPollMessage(registrarId, now); + if (!maybePollMessage.isPresent()) { + return responseBuilder.setResultFromCode(SUCCESS_WITH_NO_MESSAGES).build(); + } + PollMessage pollMessage = maybePollMessage.get(); + return responseBuilder + .setResultFromCode(SUCCESS_WITH_ACK_MESSAGE) + .setMessageQueueInfo( + new MessageQueueInfo.Builder() + .setQueueDate(pollMessage.getEventTime()) + .setMsg(pollMessage.getMsg()) + .setQueueLength(getPollMessageCount(registrarId, now)) + .setMessageId(makePollMessageExternalId(pollMessage)) + .build()) + .setMultipleResData(pollMessage.getResponseData()) + .build(); + }); } /** Unexpected message id. */ diff --git a/core/src/main/java/google/registry/model/EppResourceUtils.java b/core/src/main/java/google/registry/model/EppResourceUtils.java index 2f1f7bf40..164626cf6 100644 --- a/core/src/main/java/google/registry/model/EppResourceUtils.java +++ b/core/src/main/java/google/registry/model/EppResourceUtils.java @@ -156,7 +156,9 @@ public final class EppResourceUtils { T resource = useCache ? EppResource.loadCached(key) - : tm().transact(() -> tm().loadByKeyIfPresent(key).orElse(null)); + // This transaction is buried very deeply inside many outer nested calls, hence merits + // the use of reTransact() for now pending a substantial refactoring. + : tm().reTransact(() -> tm().loadByKeyIfPresent(key).orElse(null)); if (resource == null || isAtOrAfter(now, resource.getDeletionTime())) { return Optional.empty(); } diff --git a/core/src/main/java/google/registry/model/tmch/ClaimsList.java b/core/src/main/java/google/registry/model/tmch/ClaimsList.java index 7a89843b5..8098c7e1e 100644 --- a/core/src/main/java/google/registry/model/tmch/ClaimsList.java +++ b/core/src/main/java/google/registry/model/tmch/ClaimsList.java @@ -206,13 +206,11 @@ public class ClaimsList extends ImmutableObject { if (labelsToKeys != null) { return Optional.ofNullable(labelsToKeys.get(label)); } - return tm().transact( - () -> - tm().createQueryComposer(ClaimsEntry.class) - .where("revisionId", EQ, revisionId) - .where("domainLabel", EQ, label) - .first() - .map(ClaimsEntry::getClaimKey)); + return tm().createQueryComposer(ClaimsEntry.class) + .where("revisionId", EQ, revisionId) + .where("domainLabel", EQ, label) + .first() + .map(ClaimsEntry::getClaimKey); } public static ClaimsList create( 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 56e7303fc..491a537d2 100644 --- a/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManagerImpl.java +++ b/core/src/main/java/google/registry/persistence/transaction/JpaTransactionManagerImpl.java @@ -158,6 +158,11 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager { () -> transactNoRetry(work, isolationLevel), JpaRetries::isFailedTxnRetriable); } + @Override + public T reTransact(Supplier work) { + return transact(work); + } + @Override public T transact(Supplier work) { return transact(work, null); @@ -229,6 +234,11 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager { isolationLevel); } + @Override + public void reTransact(Runnable work) { + transact(work); + } + @Override public void transact(Runnable work) { transact(work, null); 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 9b946dabd..cf38a6b1f 100644 --- a/core/src/main/java/google/registry/persistence/transaction/TransactionManager.java +++ b/core/src/main/java/google/registry/persistence/transaction/TransactionManager.java @@ -56,12 +56,44 @@ public interface TransactionManager { */ T transact(Supplier 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 + * 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, + * 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); + /** Executes the work in a transaction. */ void transact(Runnable work); /** Executes the work in a transaction at the given {@link TransactionIsolationLevel}. */ void transact(Runnable 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 + * 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. + */ + void reTransact(Runnable work); + /** Returns the time associated with the start of this particular transaction attempt. */ DateTime getTransactionTime(); diff --git a/core/src/test/java/google/registry/model/tmch/ClaimsListDaoTest.java b/core/src/test/java/google/registry/model/tmch/ClaimsListDaoTest.java index 0f2be1ff6..c714c118d 100644 --- a/core/src/test/java/google/registry/model/tmch/ClaimsListDaoTest.java +++ b/core/src/test/java/google/registry/model/tmch/ClaimsListDaoTest.java @@ -15,11 +15,11 @@ package google.registry.model.tmch; import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.Truth8.assertThat; import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import static org.junit.jupiter.api.Assertions.assertThrows; import com.google.common.collect.ImmutableMap; -import com.google.common.truth.Truth8; import google.registry.persistence.transaction.JpaTestExtensions; import google.registry.persistence.transaction.JpaTestExtensions.JpaIntegrationWithCoverageExtension; import google.registry.testing.FakeClock; @@ -113,17 +113,21 @@ public class ClaimsListDaoTest { tm().transact(() -> tm().insert(claimsList)); ClaimsList fromDatabase = ClaimsListDao.get(); // At first, we haven't loaded any entries - assertThat(fromDatabase.claimKeyCache.getIfPresent("label1")).isNull(); - Truth8.assertThat(fromDatabase.getClaimKey("label1")).hasValue("key1"); + assertThat(tm().transact(() -> fromDatabase.claimKeyCache.getIfPresent("label1"))).isNull(); + assertThat(tm().transact(() -> fromDatabase.getClaimKey("label1"))).hasValue("key1"); // After retrieval, the key exists - Truth8.assertThat(fromDatabase.claimKeyCache.getIfPresent("label1")).hasValue("key1"); - assertThat(fromDatabase.claimKeyCache.getIfPresent("label2")).isNull(); + assertThat(tm().transact(() -> fromDatabase.claimKeyCache.getIfPresent("label1"))) + .hasValue("key1"); + assertThat(tm().transact(() -> fromDatabase.claimKeyCache.getIfPresent("label2"))).isNull(); // Loading labels-to-keys should still work - assertThat(fromDatabase.getLabelsToKeys()).containsExactly("label1", "key1", "label2", "key2"); + assertThat(tm().transact(() -> fromDatabase.getLabelsToKeys())) + .containsExactly("label1", "key1", "label2", "key2"); // We should also cache nonexistent values - assertThat(fromDatabase.claimKeyCache.getIfPresent("nonexistent")).isNull(); - Truth8.assertThat(fromDatabase.getClaimKey("nonexistent")).isEmpty(); - Truth8.assertThat(fromDatabase.claimKeyCache.getIfPresent("nonexistent")).isEmpty(); + assertThat(tm().transact(() -> fromDatabase.claimKeyCache.getIfPresent("nonexistent"))) + .isNull(); + assertThat(tm().transact(() -> fromDatabase.getClaimKey("nonexistent"))).isEmpty(); + assertThat(tm().transact(() -> fromDatabase.claimKeyCache.getIfPresent("nonexistent"))) + .isEmpty(); } private void assertClaimsListEquals(ClaimsList left, ClaimsList right) { 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 3bfbcfef3..c51dff7e5 100644 --- a/core/src/test/java/google/registry/persistence/transaction/ReplicaSimulatingJpaTransactionManager.java +++ b/core/src/test/java/google/registry/persistence/transaction/ReplicaSimulatingJpaTransactionManager.java @@ -116,6 +116,11 @@ public class ReplicaSimulatingJpaTransactionManager implements JpaTransactionMan isolationLevel); } + @Override + public T reTransact(Supplier work) { + return transact(work); + } + @Override public T transact(Supplier work) { return transact(work, null); @@ -141,6 +146,11 @@ public class ReplicaSimulatingJpaTransactionManager implements JpaTransactionMan isolationLevel); } + @Override + public void reTransact(Runnable work) { + transact(work); + } + @Override public void transact(Runnable work) { transact(work, null); diff --git a/core/src/test/java/google/registry/tmch/TmchDnlActionTest.java b/core/src/test/java/google/registry/tmch/TmchDnlActionTest.java index d9cd340eb..9253ee2c2 100644 --- a/core/src/test/java/google/registry/tmch/TmchDnlActionTest.java +++ b/core/src/test/java/google/registry/tmch/TmchDnlActionTest.java @@ -17,6 +17,7 @@ package google.registry.tmch; import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth8.assertThat; +import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -41,7 +42,8 @@ class TmchDnlActionTest extends TmchActionTestCase { @Test void testDnl() throws Exception { - assertThat(ClaimsListDao.get().getClaimKey("xn----7sbejwbn3axu3d")).isEmpty(); + assertThat(tm().transact(() -> ClaimsListDao.get().getClaimKey("xn----7sbejwbn3axu3d"))) + .isEmpty(); when(httpUrlConnection.getInputStream()) .thenReturn(new ByteArrayInputStream(TmchTestData.loadBytes("dnl/dnl-latest.csv").read())) .thenReturn(new ByteArrayInputStream(TmchTestData.loadBytes("dnl/dnl-latest.sig").read())); @@ -54,8 +56,8 @@ class TmchDnlActionTest extends TmchActionTestCase { ClaimsList claimsList = ClaimsListDao.get(); assertThat(claimsList.getTmdbGenerationTime()) .isEqualTo(DateTime.parse("2013-11-24T23:15:37.4Z")); - assertThat(claimsList.getClaimKey("xn----7sbejwbn3axu3d")) + assertThat(tm().transact(() -> claimsList.getClaimKey("xn----7sbejwbn3axu3d"))) .hasValue("2013112500/7/4/8/dIHW0DiuybvhdP8kIz"); - assertThat(claimsList.getClaimKey("lolcat")).isEmpty(); + assertThat(tm().transact(() -> claimsList.getClaimKey("lolcat"))).isEmpty(); } } diff --git a/core/src/test/java/google/registry/tools/UploadClaimsListCommandTest.java b/core/src/test/java/google/registry/tools/UploadClaimsListCommandTest.java index 48433bc4a..de42fd95a 100644 --- a/core/src/test/java/google/registry/tools/UploadClaimsListCommandTest.java +++ b/core/src/test/java/google/registry/tools/UploadClaimsListCommandTest.java @@ -16,6 +16,7 @@ package google.registry.tools; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth8.assertThat; +import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import static org.junit.jupiter.api.Assertions.assertThrows; import google.registry.model.tmch.ClaimsList; @@ -46,11 +47,11 @@ class UploadClaimsListCommandTest extends CommandTestCase claimsList.getClaimKey("example"))) .hasValue("2013041500/2/6/9/rJ1NrDO92vDsAzf7EQzgjX4R0000000001"); - assertThat(claimsList.getClaimKey("another-example")) + assertThat(tm().transact(() -> claimsList.getClaimKey("another-example"))) .hasValue("2013041500/6/A/5/alJAqG2vI2BmCv5PfUvuDkf40000000002"); - assertThat(claimsList.getClaimKey("anotherexample")) + assertThat(tm().transact(() -> claimsList.getClaimKey("anotherexample"))) .hasValue("2013041500/A/C/7/rHdC4wnrWRvPY6nneCVtQhFj0000000003"); }