diff --git a/java/google/registry/dns/PublishDnsUpdatesAction.java b/java/google/registry/dns/PublishDnsUpdatesAction.java index 83fda2c24..6d758e7db 100644 --- a/java/google/registry/dns/PublishDnsUpdatesAction.java +++ b/java/google/registry/dns/PublishDnsUpdatesAction.java @@ -81,7 +81,7 @@ public final class PublishDnsUpdatesAction implements Runnable, Callable { // false. We need to make sure to take note of this error; otherwise, a failed lock might result // in the update task being dequeued and dropped. A message will already have been logged // to indicate the problem. - if (!executeWithLocks(this, null, tld, timeout, lockName)) { + if (!executeWithLocks(this, tld, timeout, lockName)) { throw new ServiceUnavailableException("Lock failure"); } } @@ -109,6 +109,7 @@ public final class PublishDnsUpdatesAction implements Runnable, Callable { } else { dnsMetrics.incrementPublishDomainRequests(tld, Status.ACCEPTED); writer.publishDomain(domain); + logger.infofmt("%s: published domain %s", tld, domain); } } for (String host : nullToEmpty(hosts)) { @@ -119,6 +120,7 @@ public final class PublishDnsUpdatesAction implements Runnable, Callable { } else { dnsMetrics.incrementPublishHostRequests(tld, Status.ACCEPTED); writer.publishHost(host); + logger.infofmt("%s: published host %s", tld, host); } } } diff --git a/java/google/registry/export/sheet/SyncRegistrarsSheetAction.java b/java/google/registry/export/sheet/SyncRegistrarsSheetAction.java index 1d94e927a..ce6975193 100644 --- a/java/google/registry/export/sheet/SyncRegistrarsSheetAction.java +++ b/java/google/registry/export/sheet/SyncRegistrarsSheetAction.java @@ -146,7 +146,7 @@ public class SyncRegistrarsSheetAction implements Runnable { return null; } }; - if (!Lock.executeWithLocks(runner, getClass(), "", timeout, sheetLockName)) { + if (!Lock.executeWithLocks(runner, null, timeout, sheetLockName)) { // If we fail to acquire the lock, it probably means lots of updates are happening at once, in // which case it should be safe to not bother. The task queue definition should *not* specify // max-concurrent-requests for this very reason. diff --git a/java/google/registry/model/server/Lock.java b/java/google/registry/model/server/Lock.java index b15fdc366..1b60df3b5 100644 --- a/java/google/registry/model/server/Lock.java +++ b/java/google/registry/model/server/Lock.java @@ -16,16 +16,10 @@ package google.registry.model.server; import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Throwables.throwIfUnchecked; -import static com.google.common.collect.Iterables.getFirst; -import static com.google.common.collect.Iterables.skip; -import static com.google.common.collect.Sets.newLinkedHashSet; import static google.registry.model.ofy.ObjectifyService.ofy; -import static google.registry.util.CollectionUtils.nullToEmpty; -import static google.registry.util.DateTimeUtils.START_OF_TIME; import static google.registry.util.DateTimeUtils.isAtOrAfter; import com.google.common.base.Strings; -import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSortedSet; import com.googlecode.objectify.VoidWork; import com.googlecode.objectify.Work; @@ -37,7 +31,6 @@ import google.registry.model.annotations.NotBackedUp.Reason; import google.registry.util.AppEngineTimeLimiter; import google.registry.util.FormattingLogger; import java.util.HashSet; -import java.util.LinkedHashSet; import java.util.Set; import java.util.concurrent.Callable; import java.util.concurrent.TimeUnit; @@ -65,14 +58,6 @@ public class Lock extends ImmutableObject { /** When the lock can be considered implicitly released. */ DateTime expirationTime; - /** - * Insertion-ordered set of classes requesting access to the lock. - * - *

A class can only acquire the lock if the queue is empty or if it is at the top of the - * queue. This allows us to prevent starvation between processes competing for the lock. - */ - LinkedHashSet queue = new LinkedHashSet<>(); - /** * Create a new {@link Lock} for the given resource name in the specified tld (which can be * null for cross-tld locks). @@ -80,15 +65,13 @@ public class Lock extends ImmutableObject { private static Lock create( String resourceName, @Nullable String tld, - DateTime expirationTime, - LinkedHashSet queue) { + DateTime expirationTime) { checkArgument(!Strings.isNullOrEmpty(resourceName), "resourceName cannot be null or empty"); Lock instance = new Lock(); // Add the tld to the Lock's id so that it is unique for locks acquiring the same resource // across different TLDs. instance.lockId = makeLockId(resourceName, tld); instance.expirationTime = expirationTime; - instance.queue = queue; return instance; } @@ -96,27 +79,8 @@ public class Lock extends ImmutableObject { return String.format("%s-%s", tld, resourceName); } - /** Join the queue waiting on this lock (unless you are already in the queue). */ - static void joinQueue( - final Class requester, - final String resourceName, - @Nullable final String tld) { - // This transaction doesn't use the clock, so it's fine to use the default. - ofy().transactNew(new VoidWork() { - @Override - public void vrun() { - Lock lock = ofy().load().type(Lock.class).id(makeLockId(resourceName, tld)).now(); - LinkedHashSet queue = (lock == null) - ? new LinkedHashSet() : newLinkedHashSet(lock.queue); - queue.add(requester.getCanonicalName()); - DateTime expirationTime = (lock == null) ? START_OF_TIME : lock.expirationTime; - ofy().saveWithoutBackup().entity(create(resourceName, tld, expirationTime, queue)); - }}); - } - /** Try to acquire a lock. Returns null if it can't be acquired. */ static Lock acquire( - final Class requester, final String resourceName, @Nullable final String tld, final Duration leaseLength) { @@ -127,42 +91,38 @@ public class Lock extends ImmutableObject { @Override public Lock run() { String lockId = makeLockId(resourceName, tld); + DateTime now = ofy().getTransactionTime(); + + // Checking if an unexpired lock still exists - if so, the lock can't be acquired. Lock lock = ofy().load().type(Lock.class).id(lockId).now(); - if (lock == null || isAtOrAfter(ofy().getTransactionTime(), lock.expirationTime)) { - String requesterName = (requester == null) ? "" : requester.getCanonicalName(); - String firstInQueue = - getFirst(nullToEmpty((lock == null) ? null : lock.queue), requesterName); - if (!firstInQueue.equals(requesterName)) { - // Another class is at the top of the queue; we can't acquire the lock. - logger.infofmt( - "Another class %s is first in queue (size %d) instead of requested %s for lock: %s", - firstInQueue, - lock.queue.size(), - requesterName, - lockId); - return null; - } - Lock newLock = create( - resourceName, - tld, - ofy().getTransactionTime().plus(leaseLength), - newLinkedHashSet((lock == null) - ? ImmutableList.of() : skip(lock.queue, 1))); - // Locks are not parented under an EntityGroupRoot (so as to avoid write contention) and - // don't need to be backed up. - ofy().saveWithoutBackup().entity(newLock); + if (lock != null && !isAtOrAfter(now, lock.expirationTime)) { logger.infofmt( - "acquire succeeded %s lock: %s", - newLock, + "Existing lock is still valid now %s (until %s) lock: %s", + now, + lock.expirationTime, lockId); - return newLock; + return null; } + + if (lock != null) { + logger.infofmt( + "Existing lock is timed out now %s (was valid until %s) lock: %s", + now, + lock.expirationTime, + lockId); + } + Lock newLock = create( + resourceName, + tld, + now.plus(leaseLength)); + // Locks are not parented under an EntityGroupRoot (so as to avoid write contention) and + // don't need to be backed up. + ofy().saveWithoutBackup().entity(newLock); logger.infofmt( - "Existing lock is still valid now %s (until %s) lock: %s", - ofy().getTransactionTime(), - lock.expirationTime, + "acquire succeeded %s lock: %s", + newLock, lockId); - return null; + return newLock; }}); } @@ -181,6 +141,12 @@ public class Lock extends ImmutableObject { logger.infofmt("Deleting lock: %s", lockId); ofy().deleteWithoutBackup().entity(Lock.this); } else { + logger.severefmt( + "The lock we acquired was transferred to someone else before we" + + " released it! Did action take longer than lease length?" + + " Our lock: %s, current lock: %s", + Lock.this, + loadedLock); logger.infofmt("Not deleting lock: %s - someone else has it: %s", lockId, loadedLock); } }}); @@ -190,10 +156,6 @@ public class Lock extends ImmutableObject { * Acquire one or more locks and execute a Void {@link Callable} on a thread that will be * killed if it doesn't complete before the lease expires. * - *

If the requester isn't null, this will join each lock's queue before attempting to acquire - * that lock. Clients that are concerned with starvation should specify a requester and those that - * aren't shouldn't. - * *

Note that locks are specific either to a given tld or to the entire system (in which case * tld should be passed as null). * @@ -201,14 +163,12 @@ public class Lock extends ImmutableObject { */ public static boolean executeWithLocks( final Callable callable, - @Nullable Class requester, @Nullable String tld, Duration leaseLength, String... lockNames) { try { return AppEngineTimeLimiter.create().callWithTimeout( - new LockingCallable( - callable, requester, Strings.emptyToNull(tld), leaseLength, lockNames), + new LockingCallable(callable, Strings.emptyToNull(tld), leaseLength, lockNames), leaseLength.minus(LOCK_TIMEOUT_FUDGE).getMillis(), TimeUnit.MILLISECONDS, true); @@ -221,20 +181,17 @@ public class Lock extends ImmutableObject { /** A {@link Callable} that acquires and releases a lock around a delegate {@link Callable}. */ private static class LockingCallable implements Callable { final Callable delegate; - final Class requester; @Nullable final String tld; final Duration leaseLength; final Set lockNames; LockingCallable( Callable delegate, - Class requester, String tld, Duration leaseLength, String... lockNames) { checkArgument(leaseLength.isLongerThan(LOCK_TIMEOUT_FUDGE)); this.delegate = delegate; - this.requester = requester; this.tld = tld; this.leaseLength = leaseLength; // Make sure we join locks in a fixed (lexicographical) order to avoid deadlock. @@ -246,10 +203,7 @@ public class Lock extends ImmutableObject { Set acquiredLocks = new HashSet<>(); try { for (String lockName : lockNames) { - if (requester != null) { - joinQueue(requester, lockName, tld); - } - Lock lock = acquire(requester, lockName, tld, leaseLength); + Lock lock = acquire(lockName, tld, leaseLength); if (lock == null) { logger.infofmt("Couldn't acquire lock: %s", lockName); return false; diff --git a/java/google/registry/rde/EscrowTaskRunner.java b/java/google/registry/rde/EscrowTaskRunner.java index 5d60490e9..bfa94e96b 100644 --- a/java/google/registry/rde/EscrowTaskRunner.java +++ b/java/google/registry/rde/EscrowTaskRunner.java @@ -109,7 +109,7 @@ class EscrowTaskRunner { return null; }}; String lockName = String.format("%s %s", task.getClass().getSimpleName(), registry.getTld()); - if (!Lock.executeWithLocks(lockRunner, null, tld, timeout, lockName)) { + if (!Lock.executeWithLocks(lockRunner, tld, timeout, lockName)) { // This will happen if either: a) the task is double-executed; b) the task takes a long time // to run and the retry task got executed while the first one is still running. In both // situations the safest thing to do is to just return 503 so the task gets retried later. diff --git a/java/google/registry/rde/RdeStagingReducer.java b/java/google/registry/rde/RdeStagingReducer.java index d4bc2fbdc..9a0687586 100644 --- a/java/google/registry/rde/RdeStagingReducer.java +++ b/java/google/registry/rde/RdeStagingReducer.java @@ -105,7 +105,7 @@ public final class RdeStagingReducer extends Reducer queue; org.joda.time.DateTime expirationTime; } class google.registry.model.server.ServerSecret { diff --git a/javatests/google/registry/model/server/LockTest.java b/javatests/google/registry/model/server/LockTest.java index c2f890e7a..d16897fd7 100644 --- a/javatests/google/registry/model/server/LockTest.java +++ b/javatests/google/registry/model/server/LockTest.java @@ -48,65 +48,47 @@ public class LockTest { @Test public void testReleasedExplicitly() throws Exception { - Lock lock = Lock.acquire(getClass(), RESOURCE_NAME, "", ONE_DAY); + Lock lock = Lock.acquire(RESOURCE_NAME, "", ONE_DAY); assertThat(lock).isNotNull(); // We can't get it again at the same time. - assertThat(Lock.acquire(getClass(), RESOURCE_NAME, "", ONE_DAY)).isNull(); + assertThat(Lock.acquire(RESOURCE_NAME, "", ONE_DAY)).isNull(); // But if we release it, it's available. lock.release(); - assertThat(Lock.acquire(getClass(), RESOURCE_NAME, "", ONE_DAY)).isNotNull(); + assertThat(Lock.acquire(RESOURCE_NAME, "", ONE_DAY)).isNotNull(); } @Test public void testReleasedAfterTimeout() throws Exception { FakeClock clock = new FakeClock(); inject.setStaticField(Ofy.class, "clock", clock); - assertThat(Lock.acquire(getClass(), RESOURCE_NAME, "", TWO_MILLIS)).isNotNull(); + assertThat(Lock.acquire(RESOURCE_NAME, "", TWO_MILLIS)).isNotNull(); // We can't get it again at the same time. - assertThat(Lock.acquire(getClass(), RESOURCE_NAME, "", TWO_MILLIS)).isNull(); + assertThat(Lock.acquire(RESOURCE_NAME, "", TWO_MILLIS)).isNull(); // A second later we still can't get the lock. clock.advanceOneMilli(); - assertThat(Lock.acquire(getClass(), RESOURCE_NAME, "", TWO_MILLIS)).isNull(); + assertThat(Lock.acquire(RESOURCE_NAME, "", TWO_MILLIS)).isNull(); // But two seconds later we can get it. clock.advanceOneMilli(); - assertThat(Lock.acquire(getClass(), RESOURCE_NAME, "", TWO_MILLIS)).isNotNull(); + assertThat(Lock.acquire(RESOURCE_NAME, "", TWO_MILLIS)).isNotNull(); } @Test public void testTldsAreIndependent() throws Exception { - Lock lockA = Lock.acquire(getClass(), RESOURCE_NAME, "a", ONE_DAY); + Lock lockA = Lock.acquire(RESOURCE_NAME, "a", ONE_DAY); assertThat(lockA).isNotNull(); // For a different tld we can still get a lock with the same name. - Lock lockB = Lock.acquire(getClass(), RESOURCE_NAME, "b", ONE_DAY); + Lock lockB = Lock.acquire(RESOURCE_NAME, "b", ONE_DAY); assertThat(lockB).isNotNull(); // We can't get lockB again at the same time. - assertThat(Lock.acquire(getClass(), RESOURCE_NAME, "b", ONE_DAY)).isNull(); + assertThat(Lock.acquire(RESOURCE_NAME, "b", ONE_DAY)).isNull(); // Releasing lockA has no effect on lockB (even though we are still using the "b" tld). lockA.release(); - assertThat(Lock.acquire(getClass(), RESOURCE_NAME, "b", ONE_DAY)).isNull(); - } - - @Test - public void testQueueing() throws Exception { - // This should work... there's nothing on the queue. - Lock lock = Lock.acquire(String.class, RESOURCE_NAME, "", TWO_MILLIS); - assertThat(lock).isNotNull(); - lock.release(); - // Queue up a request from "Object". - Lock.joinQueue(Object.class, RESOURCE_NAME, ""); - // We can't get the lock because the "requester" is different than what's on the queue. - assertThat(Lock.acquire(String.class, RESOURCE_NAME, "", TWO_MILLIS)).isNull(); - // But this will work because the requester is the same as what's on the queue. - lock = Lock.acquire(Object.class, RESOURCE_NAME, "", TWO_MILLIS); - assertThat(lock).isNotNull(); - lock.release(); - // Now the queue is empty again so we can get the lock. - assertThat(Lock.acquire(String.class, RESOURCE_NAME, "", TWO_MILLIS)).isNotNull(); + assertThat(Lock.acquire(RESOURCE_NAME, "b", ONE_DAY)).isNull(); } @Test public void testFailure_emptyResourceName() throws Exception { thrown.expect(IllegalArgumentException.class, "resourceName cannot be null or empty"); - Lock.acquire(String.class, "", "", TWO_MILLIS); + Lock.acquire("", "", TWO_MILLIS); } } diff --git a/javatests/google/registry/rde/EscrowTaskRunnerTest.java b/javatests/google/registry/rde/EscrowTaskRunnerTest.java index 2fe95ed60..8ed3f5e5c 100644 --- a/javatests/google/registry/rde/EscrowTaskRunnerTest.java +++ b/javatests/google/registry/rde/EscrowTaskRunnerTest.java @@ -120,7 +120,6 @@ public class EscrowTaskRunnerTest { task, registry, standardSeconds(30), CursorType.RDE_STAGING, standardDays(1)); return null; }}, - null, "lol", standardSeconds(30), lockName);