[Bug] Fix TOCTOU race condition in ZookeeperClientManager.getInstance()

ZookeeperClientManager.getInstance() uses a non-atomic check-then-act
pattern (containsKey + put) on a ConcurrentHashMap. When multiple threads
call getInstance() concurrently with the same ApplicationModel, each
thread can create a separate ZookeeperClientManager instance and register
duplicate destroy listeners, causing redundant Zookeeper client cleanup
and potential resource waste.

Fix: replace the containsKey/put pattern with computeIfAbsent to ensure
exactly one manager instance and one destroy listener per ApplicationModel.
Added testGetInstanceConcurrentRaceCondition to verify exactly 1 instance
is created across concurrent threads.

Made-with: Cursor

Author: mtlyzhang

dubbo-remoting/dubbo-remoting-zookeeper-curator5/src/main/java/org/apache/dubbo/remoting/zookeeper/curator5/ZookeeperClientManager.java

@@ -48,10 +48,9 @@ public class ZookeeperClientManager {
     public ZookeeperClientManager() {}
 
     public static ZookeeperClientManager getInstance(ApplicationModel applicationModel) {
-        if (!managerMap.containsKey(applicationModel) || managerMap.get(applicationModel) == null) {
+        return managerMap.computeIfAbsent(applicationModel, model -> {
             ZookeeperClientManager clientManager = new ZookeeperClientManager();
-            applicationModel.addDestroyListener(m -> {
-                // destroy zookeeper clients if any
+            model.addDestroyListener(m -> {
                 try {
                     clientManager.destroy();
                 } catch (Exception e) {
@@ -63,9 +62,8 @@ public static ZookeeperClientManager getInstance(ApplicationModel applicationMod
                             e);
                 }
             });
-            managerMap.put(applicationModel, clientManager);
-        }
-        return managerMap.get(applicationModel);
+            return clientManager;
+        });
     }
 
     public ZookeeperClient connect(URL url) {

dubbo-remoting/dubbo-remoting-zookeeper-curator5/src/test/java/org/apache/dubbo/remoting/zookeeper/curator5/support/ZookeeperClientManagerTest.java

@@ -20,8 +20,10 @@
 import org.apache.dubbo.remoting.zookeeper.curator5.Curator5ZookeeperClient;
 import org.apache.dubbo.remoting.zookeeper.curator5.ZookeeperClient;
 import org.apache.dubbo.remoting.zookeeper.curator5.ZookeeperClientManager;
+import org.apache.dubbo.rpc.model.ApplicationModel;
 
 import java.util.List;
+import java.util.Map;
 
 import org.junit.jupiter.api.AfterAll;
 import org.junit.jupiter.api.Assertions;
@@ -244,6 +246,98 @@ void testSameHostWithDifferentUser() {
         assertThat(client1, not(client2));
     }
 
+    @Test
+    void testGetInstanceConcurrentRaceCondition() throws Exception {
+        // Clear static managerMap via reflection to start fresh
+        java.lang.reflect.Field managerMapField = ZookeeperClientManager.class.getDeclaredField("managerMap");
+        managerMapField.setAccessible(true);
+        @SuppressWarnings("unchecked")
+        Map<?, ?> managerMap = (Map<?, ?>) managerMapField.get(null);
+        managerMap.clear();
+
+        ApplicationModel appModel = ApplicationModel.defaultModel();
+        int threadCount = 20;
+        java.util.concurrent.CountDownLatch startGate = new java.util.concurrent.CountDownLatch(1);
+        java.util.concurrent.CountDownLatch endGate = new java.util.concurrent.CountDownLatch(threadCount);
+        java.util.Set<ZookeeperClientManager> uniqueInstances =
+            java.util.Collections.synchronizedSet(new java.util.HashSet<>());
+
+        for (int i = 0; i < threadCount; i++) {
+            new Thread(() -> {
+                try {
+                    startGate.await();
+                    ZookeeperClientManager instance = ZookeeperClientManager.getInstance(appModel);
+                    uniqueInstances.add(instance);
+                } catch (Exception e) {
+                    e.printStackTrace();
+                } finally {
+                    endGate.countDown();
+                }
+            }).start();
+        }
+
+        startGate.countDown();
+        endGate.await();
+
+        // With the buggy code, multiple different instances may be created.
+        // With the fix (computeIfAbsent), exactly 1 instance should exist.
+        Assertions.assertEquals(1, uniqueInstances.size(),
+            "Expected exactly 1 ZookeeperClientManager instance, but got " + uniqueInstances.size()
+            + ". This indicates a TOCTOU race condition in getInstance().");
+
+        // Also verify only 1 entry in the static map
+        Assertions.assertEquals(1, managerMap.size(),
+            "Expected exactly 1 entry in managerMap");
+
+        managerMap.clear();
+    }
+
+    @Test
+    void testGetInstanceConcurrentRaceCondition() throws Exception {
+        // Clear static managerMap via reflection to start fresh
+        java.lang.reflect.Field managerMapField = ZookeeperClientManager.class.getDeclaredField("managerMap");
+        managerMapField.setAccessible(true);
+        @SuppressWarnings("unchecked")
+        Map<?, ?> managerMap = (Map<?, ?>) managerMapField.get(null);
+        managerMap.clear();
+
+        ApplicationModel appModel = ApplicationModel.defaultModel();
+        int threadCount = 20;
+        java.util.concurrent.CountDownLatch startGate = new java.util.concurrent.CountDownLatch(1);
+        java.util.concurrent.CountDownLatch endGate = new java.util.concurrent.CountDownLatch(threadCount);
+        java.util.Set<ZookeeperClientManager> uniqueInstances =
+            java.util.Collections.synchronizedSet(new java.util.HashSet<>());
+
+        for (int i = 0; i < threadCount; i++) {
+            new Thread(() -> {
+                try {
+                    startGate.await();
+                    ZookeeperClientManager instance = ZookeeperClientManager.getInstance(appModel);
+                    uniqueInstances.add(instance);
+                } catch (Exception e) {
+                    e.printStackTrace();
+                } finally {
+                    endGate.countDown();
+                }
+            }).start();
+        }
+
+        startGate.countDown();
+        endGate.await();
+
+        // With the buggy code, multiple different instances may be created.
+        // With the fix (computeIfAbsent), exactly 1 instance should exist.
+        Assertions.assertEquals(1, uniqueInstances.size(),
+            "Expected exactly 1 ZookeeperClientManager instance, but got " + uniqueInstances.size()
+            + ". This indicates a TOCTOU race condition in getInstance().");
+
+        // Also verify only 1 entry in the static map
+        Assertions.assertEquals(1, managerMap.size(),
+            "Expected exactly 1 entry in managerMap");
+
+        managerMap.clear();
+    }
+
     @AfterAll
     public static void afterAll() {
         mockedCurator5ZookeeperClientConstruction.close();