SOLR-18142: Fix CloudSolrClient cache state refresh; regression. (#4176)

CloudSolrClient- fixed state refresh race; didn't refresh.  Regression from 9.10.1/10.0
Found by flaky test: CloudSolrClientCacheTest.testStaleStateRetryWaitsAfterSkipFailure

Author: dsmiley

changelog/unreleased/SOLR-18142.yml

@@ -0,0 +1,7 @@
+title: CloudSolrClient- fixed state refresh race; didn't refresh.  Regression from 9.10.1/10.0.
+type: fixed
+authors:
+  - name: David Smiley
+links:
+  - name: SOLR-18142
+    url: https://issues.apache.org/jira/browse/SOLR-18142

solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java

@@ -105,7 +105,7 @@ public abstract class CloudSolrClient extends SolrClient {
   private final boolean directUpdatesToLeadersOnly;
   private final RequestReplicaListTransformerGenerator requestRLTGenerator;
   private final boolean parallelUpdates;
-  private ExecutorService threadPool =
+  private final ExecutorService threadPool =
       ExecutorUtil.newMDCAwareCachedThreadPool(
           new SolrNamedThreadFactory("CloudSolrClient ThreadPool"));
 
@@ -642,9 +642,8 @@ protected boolean wasCommError(Throwable t) {
   public void close() {
     closed = true;
     collectionRefreshes.clear();
-    if (this.threadPool != null && !ExecutorUtil.isShutdown(this.threadPool)) {
+    if (!ExecutorUtil.isShutdown(this.threadPool)) {
       ExecutorUtil.shutdownAndAwaitTermination(this.threadPool);
-      this.threadPool = null;
     }
   }
 
@@ -1658,41 +1657,34 @@ protected DocCollection getDocCollection(String collection, Integer expectedVers
   }
 
   private CompletableFuture<DocCollection> triggerCollectionRefresh(String collection) {
-    if (closed) {
-      ExpiringCachedDocCollection cacheEntry = collectionStateCache.peek(collection);
-      DocCollection cached = cacheEntry == null ? null : cacheEntry.cached;
-      return CompletableFuture.completedFuture(cached);
-    }
-    return collectionRefreshes.computeIfAbsent(
+    return collectionRefreshes.compute(
         collection,
-        key -> {
-          ExecutorService executor = threadPool;
-          CompletableFuture<DocCollection> future;
-          if (executor == null || ExecutorUtil.isShutdown(executor)) {
-            future = new CompletableFuture<>();
-            try {
-              future.complete(loadDocCollection(key));
-            } catch (Throwable t) {
-              future.completeExceptionally(t);
-            }
+        (key, existingFuture) -> {
+          // A refresh is still in progress; return it.
+          if (existingFuture != null && !existingFuture.isDone()) {
+            return existingFuture;
+          }
+          // No refresh is in-progress, so trigger it.
+
+          if (ExecutorUtil.isShutdown(threadPool)) {
+            assert closed; // see close() for the sequence
+            ExpiringCachedDocCollection cacheEntry = collectionStateCache.peek(key);
+            DocCollection cached = cacheEntry == null ? null : cacheEntry.cached;
+            return CompletableFuture.completedFuture(cached);
           } else {
-            future =
-                CompletableFuture.supplyAsync(
-                    () -> {
-                      stateRefreshSemaphore.acquireUninterruptibly();
-                      try {
-                        return loadDocCollection(key);
-                      } finally {
-                        stateRefreshSemaphore.release();
-                      }
-                    },
-                    executor);
+            return CompletableFuture.supplyAsync(
+                () -> {
+                  stateRefreshSemaphore.acquireUninterruptibly();
+                  try {
+                    return loadDocCollection(key);
+                  } finally {
+                    stateRefreshSemaphore.release();
+                    // Remove the entry in case of many collections
+                    collectionRefreshes.remove(key);
+                  }
+                },
+                threadPool);
           }
-          future.whenCompleteAsync(
-              (result, error) -> {
-                collectionRefreshes.remove(key, future);
-              });
-          return future;
         });
   }