Remove reentrant lock no longer needed

4 files changed, 30 insertions, 50 deletions

diff --git a/zkfacade/src/main/java/com/yahoo/vespa/curator/Lock.java b/zkfacade/src/main/java/com/yahoo/vespa/curator/Lock.java
index da6e3109695..7fb6b88cf99 100644
--- a/zkfacade/src/main/java/com/yahoo/vespa/curator/Lock.java
+++ b/zkfacade/src/main/java/com/yahoo/vespa/curator/Lock.java
@@ -9,7 +9,6 @@ import org.apache.curator.framework.recipes.locks.InterProcessLock;
 
 import java.time.Duration;
 import java.util.concurrent.TimeUnit;
-import java.util.concurrent.locks.ReentrantLock;
 
 /**
  * A cluster-wide re-entrant mutex which is released on (the last symmetric) close.
@@ -21,13 +20,11 @@ import java.util.concurrent.locks.ReentrantLock;
  */
 public class Lock implements Mutex {
 
-    private final ReentrantLock lock;
     private final InterProcessLock mutex;
     private final String lockPath;
 
     public Lock(String lockPath, Curator curator) {
         this.lockPath = lockPath;
-        this.lock = new ReentrantLock(true);
         mutex = curator.createMutex(lockPath);
     }
 
@@ -35,37 +32,26 @@ public class Lock implements Mutex {
     public void acquire(Duration timeout) throws UncheckedTimeoutException {
         ThreadLockInfo threadLockInfo = getThreadLockInfo();
         threadLockInfo.invokingAcquire(timeout);
-        try {
-            if ( ! mutex.acquire(timeout.toMillis(), TimeUnit.MILLISECONDS)) {
-                threadLockInfo.acquireTimedOut();
-
-                throw new UncheckedTimeoutException("Timed out after waiting " + timeout +
-                                                    " to acquire lock '" + lockPath + "'");
-            }
-            threadLockInfo.lockAcquired();
 
-            if ( ! lock.tryLock()) { // Should be available to only this thread, while holding the above mutex.
-                release();
-                threadLockInfo.failedToAcquireReentrantLock();
-                throw new IllegalStateException("InterProcessMutex acquired, but guarded lock held by someone else, for lock '" + lockPath + "'");
-            }
-        }
-        catch (UncheckedTimeoutException | IllegalStateException e) {
-            throw e;
-        }
-        catch (Exception e) {
+        final boolean acquired;
+        try {
+            acquired = mutex.acquire(timeout.toMillis(), TimeUnit.MILLISECONDS);
+        } catch (Exception e) {
+            threadLockInfo.acquireFailed();
             throw new RuntimeException("Exception acquiring lock '" + lockPath + "'", e);
         }
+
+        if (!acquired) {
+            threadLockInfo.acquireTimedOut();
+            throw new UncheckedTimeoutException("Timed out after waiting " + timeout +
+                    " to acquire lock '" + lockPath + "'");
+        }
+        threadLockInfo.lockAcquired();
     }
 
     @Override
     public void close() {
-        try {
-            lock.unlock();
-        }
-        finally {
-            release();
-        }
+        release();
     }
 
     private void release() {
@@ -79,7 +65,7 @@ public class Lock implements Mutex {
     }
 
     private ThreadLockInfo getThreadLockInfo() {
-        return ThreadLockInfo.getCurrentThreadLockInfo(lockPath, lock);
+        return ThreadLockInfo.getCurrentThreadLockInfo(lockPath);
     }
 }
 
diff --git a/zkfacade/src/main/java/com/yahoo/vespa/curator/stats/LockCounters.java b/zkfacade/src/main/java/com/yahoo/vespa/curator/stats/LockCounters.java
index 9052db5ce5a..4d1b48ef7f3 100644
--- a/zkfacade/src/main/java/com/yahoo/vespa/curator/stats/LockCounters.java
+++ b/zkfacade/src/main/java/com/yahoo/vespa/curator/stats/LockCounters.java
@@ -11,20 +11,20 @@ import java.util.concurrent.atomic.AtomicInteger;
 public class LockCounters {
     final AtomicInteger invokeAcquireCount = new AtomicInteger(0);
     final AtomicInteger inCriticalRegionCount = new AtomicInteger(0);
+    final AtomicInteger acquireFailedCount = new AtomicInteger(0);
     final AtomicInteger acquireTimedOutCount = new AtomicInteger(0);
     final AtomicInteger lockAcquiredCount = new AtomicInteger(0);
     final AtomicInteger locksReleasedCount = new AtomicInteger(0);
 
-    final AtomicInteger failedToAcquireReentrantLockCount = new AtomicInteger(0);
     final AtomicInteger noLocksErrorCount = new AtomicInteger(0);
     final AtomicInteger timeoutOnReentrancyErrorCount = new AtomicInteger(0);
 
     public int invokeAcquireCount() { return invokeAcquireCount.get(); }
     public int inCriticalRegionCount() { return inCriticalRegionCount.get(); }
+    public int acquireFailedCount() { return acquireFailedCount.get(); }
     public int acquireTimedOutCount() { return acquireTimedOutCount.get(); }
     public int lockAcquiredCount() { return lockAcquiredCount.get(); }
     public int locksReleasedCount() { return locksReleasedCount.get(); }
-    public int failedToAcquireReentrantLockCount() { return failedToAcquireReentrantLockCount.get(); }
     public int noLocksErrorCount() { return noLocksErrorCount.get(); }
     public int timeoutOnReentrancyErrorCount() { return timeoutOnReentrancyErrorCount.get(); }
 }
diff --git a/zkfacade/src/main/java/com/yahoo/vespa/curator/stats/LockInfo.java b/zkfacade/src/main/java/com/yahoo/vespa/curator/stats/LockInfo.java
index 37481a6b1c7..1e46b1cf668 100644
--- a/zkfacade/src/main/java/com/yahoo/vespa/curator/stats/LockInfo.java
+++ b/zkfacade/src/main/java/com/yahoo/vespa/curator/stats/LockInfo.java
@@ -17,7 +17,6 @@ public class LockInfo {
 
     private final Thread thread;
     private final String lockPath;
-    private final int threadHoldCountOnAcquire;
     private final Instant acquireInstant;
     private final Duration timeout;
 
@@ -25,12 +24,12 @@ public class LockInfo {
     private volatile Optional<Instant> terminalStateInstant = Optional.empty();
     private volatile Optional<String> stackTrace = Optional.empty();
 
-    public static LockInfo invokingAcquire(Thread thread, String lockPath, int holdCount, Duration timeout) {
-        return new LockInfo(thread, lockPath, holdCount, timeout);
+    public static LockInfo invokingAcquire(Thread thread, String lockPath, Duration timeout) {
+        return new LockInfo(thread, lockPath, timeout);
     }
 
     public enum LockState {
-        ACQUIRING(false), TIMED_OUT(true), ACQUIRED(false), FAILED_TO_REENTER(true), RELEASED(true);
+        ACQUIRING(false), ACQUIRE_FAILED(true), TIMED_OUT(true), ACQUIRED(false), RELEASED(true);
 
         private final boolean terminal;
 
@@ -41,17 +40,15 @@ public class LockInfo {
 
     private volatile LockState lockState = LockState.ACQUIRING;
 
-    private LockInfo(Thread thread, String lockPath, int threadHoldCountOnAcquire, Duration timeout) {
+    private LockInfo(Thread thread, String lockPath, Duration timeout) {
         this.thread = thread;
         this.lockPath = lockPath;
-        this.threadHoldCountOnAcquire = threadHoldCountOnAcquire;
         this.acquireInstant = Instant.now();
         this.timeout = timeout;
     }
 
     public String getThreadName() { return thread.getName(); }
     public String getLockPath() { return lockPath; }
-    public int getThreadHoldCountOnAcquire() { return threadHoldCountOnAcquire; }
     public Instant getTimeAcquiredWasInvoked() { return acquireInstant; }
     public Duration getAcquireTimeout() { return timeout; }
     public LockState getLockState() { return lockState; }
@@ -100,8 +97,8 @@ public class LockInfo {
         this.stackTrace = Optional.of(stackTrace.toString());
     }
 
+    void acquireFailed() { setTerminalState(LockState.ACQUIRE_FAILED); }
     void timedOut() { setTerminalState(LockState.TIMED_OUT); }
-    void failedToAcquireReentrantLock() { setTerminalState(LockState.FAILED_TO_REENTER); }
     void released() { setTerminalState(LockState.RELEASED); }
 
     void lockAcquired() {
diff --git a/zkfacade/src/main/java/com/yahoo/vespa/curator/stats/ThreadLockInfo.java b/zkfacade/src/main/java/com/yahoo/vespa/curator/stats/ThreadLockInfo.java
index 0ca9377d360..9f92b6444be 100644
--- a/zkfacade/src/main/java/com/yahoo/vespa/curator/stats/ThreadLockInfo.java
+++ b/zkfacade/src/main/java/com/yahoo/vespa/curator/stats/ThreadLockInfo.java
@@ -12,7 +12,6 @@ import java.util.PriorityQueue;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentLinkedQueue;
 import java.util.concurrent.atomic.AtomicInteger;
-import java.util.concurrent.locks.ReentrantLock;
 import java.util.function.Consumer;
 
 /**
@@ -37,7 +36,6 @@ public class ThreadLockInfo {
 
     private final Thread thread;
     private final String lockPath;
-    private final ReentrantLock lock;
     private final LockCounters lockCountersForPath;
 
     /** The locks are reentrant so there may be more than 1 lock for this thread. */
@@ -54,19 +52,18 @@ public class ThreadLockInfo {
     }
 
     /** Returns the per-thread singleton ThreadLockInfo. */
-    public static ThreadLockInfo getCurrentThreadLockInfo(String lockPath, ReentrantLock lock) {
+    public static ThreadLockInfo getCurrentThreadLockInfo(String lockPath) {
         return locks.computeIfAbsent(
                 Thread.currentThread(),
                 currentThread -> {
                     LockCounters lockCounters = countersByLockPath.computeIfAbsent(lockPath, ignored -> new LockCounters());
-                    return new ThreadLockInfo(currentThread, lockPath, lock, lockCounters);
+                    return new ThreadLockInfo(currentThread, lockPath, lockCounters);
                 });
     }
 
-    ThreadLockInfo(Thread currentThread, String lockPath, ReentrantLock lock, LockCounters lockCountersForPath) {
+    ThreadLockInfo(Thread currentThread, String lockPath, LockCounters lockCountersForPath) {
         this.thread = currentThread;
         this.lockPath = lockPath;
-        this.lock = lock;
         this.lockCountersForPath = lockCountersForPath;
     }
 
@@ -78,7 +75,12 @@ public class ThreadLockInfo {
     public void invokingAcquire(Duration timeout) {
         lockCountersForPath.invokeAcquireCount.incrementAndGet();
         lockCountersForPath.inCriticalRegionCount.incrementAndGet();
-        lockInfos.add(LockInfo.invokingAcquire(thread, lockPath, lock.getHoldCount(), timeout));
+        lockInfos.add(LockInfo.invokingAcquire(thread, lockPath, timeout));
+    }
+
+    /** Mutable method (see class doc) */
+    public void acquireFailed() {
+        removeLastLockInfo(lockCountersForPath.acquireFailedCount, LockInfo::acquireFailed);
     }
 
     /** Mutable method (see class doc) */
@@ -98,11 +100,6 @@ public class ThreadLockInfo {
     }
 
     /** Mutable method (see class doc) */
-    public void failedToAcquireReentrantLock() {
-        removeLastLockInfo(lockCountersForPath.failedToAcquireReentrantLockCount, LockInfo::failedToAcquireReentrantLock);
-    }
-
-    /** Mutable method (see class doc) */
     public void lockReleased() {
         removeLastLockInfo(lockCountersForPath.locksReleasedCount, LockInfo::released);
     }