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 ac12cec73..ca5495441 100644 --- a/core/src/main/java/google/registry/model/server/Lock.java +++ b/core/src/main/java/google/registry/model/server/Lock.java @@ -16,6 +16,7 @@ package google.registry.model.server; import static com.google.common.base.Preconditions.checkArgument; import static google.registry.model.ofy.ObjectifyService.ofy; +import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm; import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import static google.registry.util.DateTimeUtils.isAtOrAfter; @@ -28,6 +29,7 @@ import com.googlecode.objectify.annotation.Id; import google.registry.model.ImmutableObject; import google.registry.model.annotations.NotBackedUp; import google.registry.model.annotations.NotBackedUp.Reason; +import google.registry.schema.server.LockDao; import google.registry.util.RequestStatusChecker; import google.registry.util.RequestStatusCheckerImpl; import java.io.Serializable; @@ -177,8 +179,7 @@ public class Lock extends ImmutableObject implements Serializable { // access to resources like GCS that can't be transactionally rolled back. Therefore, the lock // must be definitively acquired before it is used, even when called inside another transaction. AcquireResult acquireResult = - tm() - .transactNew( + tm().transactNew( () -> { DateTime now = tm().getTransactionTime(); @@ -207,6 +208,31 @@ public class Lock extends ImmutableObject implements Serializable { // contention) and // don't need to be backed up. ofy().saveWithoutBackup().entity(newLock); + + // create and save the lock to Cloud SQL + try { + jpaTm() + .transact( + () -> { + google.registry.schema.server.Lock cloudSqlLock = + google.registry.schema.server.Lock.create( + resourceName, + Optional.ofNullable(tld).orElse("GLOBAL"), + requestStatusChecker.getLogId(), + now, + leaseLength); + // cloudSqlLock should not already exist in Cloud SQL, but call delete + // just in case + // TODO: Remove this delete once dual read is added + LockDao.delete( + resourceName, Optional.ofNullable(tld).orElse("GLOBAL")); + LockDao.saveNew(cloudSqlLock); + }); + } catch (Exception e) { + logger.atSevere().withCause(e).log( + "Error saving lock to Cloud SQL: %s", newLock); + } + return AcquireResult.create(now, lock, newLock, lockState); }); @@ -218,8 +244,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. - tm() - .transact( + tm().transact( () -> { // To release a lock, check that no one else has already obtained it and if not // delete it. If the lock in Datastore was different then this lock is gone already; @@ -231,6 +256,19 @@ public class Lock extends ImmutableObject implements Serializable { // lock. logger.atInfo().log("Deleting lock: %s", lockId); ofy().deleteWithoutBackup().entity(Lock.this); + + // Remove the lock from Cloud SQL + try { + jpaTm() + .transact( + () -> + LockDao.delete( + resourceName, Optional.ofNullable(tld).orElse("GLOBAL"))); + } catch (Exception e) { + logger.atSevere().withCause(e).log( + "Error deleting lock from Cloud SQL: %s", loadedLock); + } + lockMetrics.recordRelease( resourceName, tld, new Duration(acquiredTime, tm().getTransactionTime())); } else { diff --git a/core/src/main/java/google/registry/schema/server/LockDao.java b/core/src/main/java/google/registry/schema/server/LockDao.java index 2ac8e3c62..2f2a05ca2 100644 --- a/core/src/main/java/google/registry/schema/server/LockDao.java +++ b/core/src/main/java/google/registry/schema/server/LockDao.java @@ -51,26 +51,29 @@ public class LockDao { * else empty. */ public static Optional load(String resourceName) { - checkArgumentNotNull(resourceName, "The resource name of the lock to load cannot be null"); - return Optional.ofNullable( - jpaTm() - .transact( - () -> - jpaTm().getEntityManager().find(Lock.class, new LockId(resourceName, GLOBAL)))); + return load(resourceName, GLOBAL); } /** - * Deletes the given {@link Lock} object from Cloud SQL. This method is idempotent and will simply - * return if the lock has already been deleted. + * Deletes the {@link Lock} object with the given resourceName and tld from Cloud SQL. This method + * is idempotent and will simply return if the lock has already been deleted. */ - public static void delete(Lock lock) { + public static void delete(String resourceName, String tld) { jpaTm() .transact( () -> { - Optional loadedLock = load(lock.resourceName, lock.tld); + Optional loadedLock = load(resourceName, tld); if (loadedLock.isPresent()) { jpaTm().getEntityManager().remove(loadedLock.get()); } }); } + + /** + * Deletes the global {@link Lock} object with the given resourceName from Cloud SQL. This method + * is idempotent and will simply return if the lock has already been deleted. + */ + public static void delete(String resourceName) { + delete(resourceName, GLOBAL); + } } diff --git a/core/src/test/java/google/registry/model/server/LockTest.java b/core/src/test/java/google/registry/model/server/LockTest.java index bb564e322..037e48776 100644 --- a/core/src/test/java/google/registry/model/server/LockTest.java +++ b/core/src/test/java/google/registry/model/server/LockTest.java @@ -28,6 +28,9 @@ import static org.mockito.Mockito.when; import google.registry.model.ofy.Ofy; import google.registry.model.server.Lock.LockState; +import google.registry.persistence.transaction.JpaTestRules; +import google.registry.persistence.transaction.JpaTestRules.JpaIntegrationWithCoverageRule; +import google.registry.schema.server.LockDao; import google.registry.testing.AppEngineRule; import google.registry.testing.FakeClock; import google.registry.testing.InjectRule; @@ -56,21 +59,28 @@ public class LockTest { @Rule public final AppEngineRule appEngine = AppEngineRule.builder().withDatastore().build(); @Rule public final InjectRule inject = new InjectRule(); + @Rule + public final JpaIntegrationWithCoverageRule jpaRule = + new JpaTestRules.Builder().withClock(clock).buildIntegrationWithCoverageRule(); + private Optional acquire(String tld, Duration leaseLength, LockState expectedLockState) { Lock.lockMetrics = mock(LockMetrics.class); Optional lock = Lock.acquire(RESOURCE_NAME, tld, leaseLength, requestStatusChecker, true); verify(Lock.lockMetrics).recordAcquire(RESOURCE_NAME, tld, expectedLockState); verifyNoMoreInteractions(Lock.lockMetrics); + assertThat(LockDao.load(RESOURCE_NAME, tld)).isPresent(); Lock.lockMetrics = null; return lock; } private void release(Lock lock, String expectedTld, long expectedMillis) { + assertThat(LockDao.load(RESOURCE_NAME, expectedTld)).isPresent(); Lock.lockMetrics = mock(LockMetrics.class); lock.release(); verify(Lock.lockMetrics) .recordRelease(RESOURCE_NAME, expectedTld, Duration.millis(expectedMillis)); verifyNoMoreInteractions(Lock.lockMetrics); + assertThat(LockDao.load(RESOURCE_NAME, expectedTld)).isEmpty(); Lock.lockMetrics = null; } diff --git a/core/src/test/java/google/registry/schema/integration/SqlIntegrationTestSuite.java b/core/src/test/java/google/registry/schema/integration/SqlIntegrationTestSuite.java index 0c3a8a26a..725e4227c 100644 --- a/core/src/test/java/google/registry/schema/integration/SqlIntegrationTestSuite.java +++ b/core/src/test/java/google/registry/schema/integration/SqlIntegrationTestSuite.java @@ -18,6 +18,7 @@ import com.google.common.truth.Expect; import google.registry.model.common.CursorTest; import google.registry.model.domain.DomainBaseSqlTest; import google.registry.model.registry.RegistryLockDaoTest; +import google.registry.model.server.LockTest; import google.registry.persistence.transaction.JpaEntityCoverage; import google.registry.schema.cursor.CursorDaoTest; import google.registry.schema.registrar.RegistrarDaoTest; @@ -64,10 +65,11 @@ import org.junit.runners.Suite.SuiteClasses; CreateReservedListCommandTest.class, CursorDaoTest.class, CursorTest.class, + DomainBaseSqlTest.class, DomainLockUtilsTest.class, LockDaoTest.class, LockDomainCommandTest.class, - DomainBaseSqlTest.class, + LockTest.class, PremiumListDaoTest.class, RegistrarDaoTest.class, RegistryLockDaoTest.class, diff --git a/core/src/test/java/google/registry/schema/server/LockDaoTest.java b/core/src/test/java/google/registry/schema/server/LockDaoTest.java index 478c5009c..e54c9b1c1 100644 --- a/core/src/test/java/google/registry/schema/server/LockDaoTest.java +++ b/core/src/test/java/google/registry/schema/server/LockDaoTest.java @@ -103,7 +103,7 @@ public class LockDaoTest { LockDao.saveNew(lock); Optional returnedLock = LockDao.load("testResource", "tld"); assertThat(returnedLock.get().expirationTime).isEqualTo(lock.expirationTime); - LockDao.delete(lock); + LockDao.delete("testResource", "tld"); returnedLock = LockDao.load("testResource", "tld"); assertThat(returnedLock.isPresent()).isFalse(); } @@ -115,15 +115,13 @@ public class LockDaoTest { LockDao.saveNew(lock); Optional returnedLock = LockDao.load("testResource"); assertThat(returnedLock.get().expirationTime).isEqualTo(lock.expirationTime); - LockDao.delete(lock); + LockDao.delete("testResource"); returnedLock = LockDao.load("testResource"); assertThat(returnedLock.isPresent()).isFalse(); } @Test public void delete_succeedsLockDoesntExist() { - Lock lock = - Lock.createGlobal("testResource", "testLogId", fakeClock.nowUtc(), Duration.millis(2)); - LockDao.delete(lock); + LockDao.delete("testResource"); } }