[brooklyn-server] 02/02: fix rare bug in mutex code

[he...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

heneveld pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/brooklyn-server.git

commit c70d9ce56220b3f21204981de10f949f1fb7dec2
Author: Alex Heneveld <al...@cloudsoft.io>
AuthorDate: Mon May 15 10:10:42 2023 +0100

    fix rare bug in mutex code
    
    it could happen that one acquirer gets a view of a semaphore before it registers its usage,
    meanwhile another removes it
---
 .../org/apache/brooklyn/util/core/mutex/MutexSupport.java |  5 ++---
 .../brooklyn/util/core/mutex/SemaphoreWithOwners.java     | 15 +++++++--------
 2 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/core/src/main/java/org/apache/brooklyn/util/core/mutex/MutexSupport.java b/core/src/main/java/org/apache/brooklyn/util/core/mutex/MutexSupport.java
index aadff08bb5..92e30b16d9 100644
--- a/core/src/main/java/org/apache/brooklyn/util/core/mutex/MutexSupport.java
+++ b/core/src/main/java/org/apache/brooklyn/util/core/mutex/MutexSupport.java
@@ -102,10 +102,9 @@ public class MutexSupport implements WithMutexes {
 
     @Override
     public synchronized void releaseMutex(String mutexId) {
-        SemaphoreWithOwners s;
         if (log.isDebugEnabled())
             log.debug("Releasing mutex: "+mutexId+"@"+this);
-        synchronized (this) { s = semaphores.get(mutexId); }
+        SemaphoreWithOwners s = semaphores.get(mutexId);
         if (s==null) throw new IllegalStateException("No mutex known for '"+mutexId+"'");
         s.release();
         cleanupMutex(mutexId);
@@ -121,6 +120,6 @@ public class MutexSupport implements WithMutexes {
      * from manipulating the semaphore objects themselves. 
      */
     public synchronized Map<String,SemaphoreWithOwners> getAllSemaphores() {
-        return ImmutableMap.<String,SemaphoreWithOwners>copyOf(semaphores);
+        return ImmutableMap.copyOf(semaphores);
     }
 }
\ No newline at end of file
diff --git a/core/src/main/java/org/apache/brooklyn/util/core/mutex/SemaphoreWithOwners.java b/core/src/main/java/org/apache/brooklyn/util/core/mutex/SemaphoreWithOwners.java
index 0a1ab9b8f2..2182306f8d 100644
--- a/core/src/main/java/org/apache/brooklyn/util/core/mutex/SemaphoreWithOwners.java
+++ b/core/src/main/java/org/apache/brooklyn/util/core/mutex/SemaphoreWithOwners.java
@@ -18,17 +18,16 @@
  */
 package org.apache.brooklyn.util.core.mutex;
 
+import com.google.common.collect.ImmutableList;
+import org.apache.brooklyn.util.collections.MutableSet;
+import org.apache.brooklyn.util.exceptions.Exceptions;
+
 import java.util.ArrayList;
-import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Set;
 import java.util.concurrent.Semaphore;
 import java.util.concurrent.TimeUnit;
 
-import org.apache.brooklyn.util.exceptions.Exceptions;
-
-import com.google.common.collect.ImmutableList;
-
 /** a subclass of {@link Semaphore} 
  * which tracks who created and released the semaphores,
  * and which requires the same thread to release as created it. */
@@ -42,7 +41,7 @@ public class SemaphoreWithOwners extends Semaphore {
     }
     private static final long serialVersionUID = -5303474637353009454L;
     final private List<Thread> owningThreads = new ArrayList<Thread>();
-    final private Set<Thread> requestingThreads = new LinkedHashSet<Thread>();
+    final private Set<Thread> requestingThreads = MutableSet.of();
     
     @Override
     public void acquire() throws InterruptedException {
@@ -188,7 +187,7 @@ public class SemaphoreWithOwners extends Semaphore {
 
     /** true iff there are any owners or any requesters (callers blocked trying to acquire) */
     public synchronized boolean isInUse() {
-        return !owningThreads.isEmpty() || !requestingThreads.isEmpty();
+        return !requestingThreads.isEmpty() || !owningThreads.isEmpty();
     }
 
     /** true iff the calling thread is one of the owners */ 
@@ -224,7 +223,7 @@ public class SemaphoreWithOwners extends Semaphore {
     /** Indicate that the calling thread is going to acquire or tryAcquire, 
      * in order to set up the semaphore's isInUse() value appropriately for certain checks.
      * It *must* do so after invoking this method. */ 
-    public void indicateCallingThreadWillRequest() {
+    public synchronized void indicateCallingThreadWillRequest() {
         requestingThreads.add(Thread.currentThread());
     }