Remove queueing from Lock

It was buggy (didn't work) and was never actually used.

Why never actually used: for it to be used executeWithLock has to be called
with different requesters on the same lockId. That never happend in the code.

How it was buggy: Logically, the queue is deleted on release of the lock (meaning it was
meaningless the only time it mattered - when the lock isn't taken). In
addition, a different bug meant that having items in the queue prevented the
lock from being released forcing all other tasks to have to wait for lock
timeout even if the task that acquired the lock is long done.

Alternative: fix the queue. This would mean we don't want to delete the lock on release (since we want to keep the queue). Instead, we resave the same lock with expiration date being START_OF_TIME. In addition - we need to fix the .equals used to determine if the lock the same as the acquired lock - instead use some isSame function that ignores the queue.

Note: the queue is dangerous! An item (calling class / action) in the first place of a queue means no other calling class can get that lock. Everything is waiting for the first calling class to be re-run - but that might take a long time (depending on that action's rerun policy) and even might never happen (if for some reason that action decided it was no longer needed without acquiring the lock) - causing all other actions to stall forever!

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=163705463

java/google/registry/dns/PublishDnsUpdatesAction.java

@@ -81,7 +81,7 @@ public final class PublishDnsUpdatesAction implements Runnable, Callable<Void> {
     // 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<Void> {
         } 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<Void> {
         } else {
           dnsMetrics.incrementPublishHostRequests(tld, Status.ACCEPTED);
           writer.publishHost(host);
+          logger.infofmt("%s: published host %s", tld, host);
         }
       }
     }

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.

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.
-   *
-   * <p>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<String> 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,
+      DateTime expirationTime) {
-      LinkedHashSet<String> queue) {
     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<String> queue = (lock == null)
-            ? new LinkedHashSet<String>() : 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,27 +91,30 @@ 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)) {
+        if (lock != null && !isAtOrAfter(now, 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",
+              "Existing lock is still valid now %s (until %s) lock: %s",
-                firstInQueue,
+              now,
-                lock.queue.size(),
+              lock.expirationTime,
-                requesterName,
               lockId);
           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,
-              ofy().getTransactionTime().plus(leaseLength),
+            now.plus(leaseLength));
-              newLinkedHashSet((lock == null)
-                  ? ImmutableList.<String>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);
@@ -156,13 +123,6 @@ public class Lock extends ImmutableObject {
             newLock,
             lockId);
         return newLock;
-        }
-        logger.infofmt(
-            "Existing lock is still valid now %s (until %s) lock: %s",
-            ofy().getTransactionTime(),
-            lock.expirationTime,
-            lockId);
-        return null;
       }});
   }
 
@@ -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.
    *
-   * <p>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.
-   *
    * <p>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<Void> callable,
-      @Nullable Class<?> requester,
       @Nullable String tld,
       Duration leaseLength,
       String... lockNames) {
     try {
       return AppEngineTimeLimiter.create().callWithTimeout(
-          new LockingCallable(
+          new LockingCallable(callable, Strings.emptyToNull(tld), leaseLength, lockNames),
-              callable, requester, 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<Boolean> {
     final Callable<Void> delegate;
-    final Class<?> requester;
     @Nullable final String tld;
     final Duration leaseLength;
     final Set<String> lockNames;
 
     LockingCallable(
         Callable<Void> 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<Lock> acquiredLocks = new HashSet<>();
       try {
         for (String lockName : lockNames) {
-          if (requester != null) {
+          Lock lock = acquire(lockName, tld, leaseLength);
-            joinQueue(requester, lockName, tld);
-          }
-          Lock lock = acquire(requester, lockName, tld, leaseLength);
           if (lock == null) {
             logger.infofmt("Couldn't acquire lock: %s", lockName);
             return false;

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.

java/google/registry/rde/RdeStagingReducer.java

@@ -105,7 +105,7 @@ public final class RdeStagingReducer extends Reducer<PendingDeposit, DepositFrag
         return null;
       }};
     String lockName = String.format("RdeStaging %s", key.tld());
-    if (!Lock.executeWithLocks(lockRunner, null, null, lockTimeout, lockName)) {
+    if (!Lock.executeWithLocks(lockRunner, null, lockTimeout, lockName)) {
       logger.warningfmt("Lock in use: %s", lockName);
     }
   }

javatests/google/registry/export/sheet/SyncRegistrarsSheetActionTest.java

@@ -103,7 +103,7 @@ public class SyncRegistrarsSheetActionTest {
         runAction(null, "foobar");
         return null;
       }
-    }, SyncRegistrarsSheetAction.class, "", Duration.standardHours(1), lockName);
+    }, null, Duration.standardHours(1), lockName);
     assertThat(response.getPayload()).startsWith("LOCKED");
     verifyZeroInteractions(syncRegistrarsSheet);
   }

javatests/google/registry/model/schema.txt

@@ -841,7 +841,6 @@ class google.registry.model.server.KmsSecretRevision {
 }
 class google.registry.model.server.Lock {
   @Id java.lang.String lockId;
-  java.util.LinkedHashSet<java.lang.String> queue;
   org.joda.time.DateTime expirationTime;
 }
 class google.registry.model.server.ServerSecret {

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();
+    assertThat(Lock.acquire(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();
   }
 
   @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);
   }
 }

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);