Do not close threadpool if termination fails

server/src/main/java/org/elasticsearch/client/transport/TransportClient.java

@@ -374,7 +374,14 @@ public void close() {
         for (LifecycleComponent plugin : pluginLifecycleComponents) {
             closeables.add(plugin);
         }
-        closeables.add(() -> ThreadPool.terminate(injector.getInstance(ThreadPool.class), 10, TimeUnit.SECONDS));
+        closeables.add(() -> {
+            final ThreadPool pool = injector.getInstance(ThreadPool.class);
+            final boolean terminated = ThreadPool.terminate(pool, 10, TimeUnit.SECONDS);
+            if (terminated == false) {
+                // the pool is only closed if termination succeeds, just close even if termination failed
+                pool.close();
+            }
+        });
         IOUtils.closeWhileHandlingException(closeables);
     }
 

server/src/main/java/org/elasticsearch/threadpool/ThreadPool.java

@@ -412,6 +412,7 @@ public boolean awaitTermination(long timeout, TimeUnit unit) throws InterruptedE
             }
         }
         cachedTimeThread.join(unit.toMillis(timeout));
+        result &= cachedTimeThread.isAlive() == false;
         return result;
     }
 
@@ -699,22 +700,28 @@ private static boolean awaitTermination(
 
     /**
      * Returns <code>true</code> if the given pool was terminated successfully. If the termination timed out,
-     * the service is <code>null</code> this method will return <code>false</code>.
+     * the service is <code>null</code> this method will return <code>false</code>. The pool is only closed if
+     * the termination was successful.
      */
     public static boolean terminate(ThreadPool pool, long timeout, TimeUnit timeUnit) {
+        boolean terminated = false;
         if (pool != null) {
-            // Leverage try-with-resources to close the threadpool
-            try (ThreadPool c = pool) {
+            try {
                 pool.shutdown();
                 if (awaitTermination(pool, timeout, timeUnit)) {
-                    return true;
+                    terminated = true;
+                } else {
+                    // last resort
+                    pool.shutdownNow();
+                    terminated = awaitTermination(pool, timeout, timeUnit);
+                }
+            } finally {
+                if (terminated) {
+                    pool.close();
                 }
-                // last resort
-                pool.shutdownNow();
-                return awaitTermination(pool, timeout, timeUnit);
             }
         }
-        return false;
+        return terminated;
     }
 
     private static boolean awaitTermination(

server/src/test/java/org/elasticsearch/node/NodeTests.java

@@ -50,7 +50,6 @@
 import static org.elasticsearch.cluster.metadata.IndexMetaData.SETTING_NUMBER_OF_SHARDS;
 import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
 
-@LuceneTestCase.AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/42577")
 @LuceneTestCase.SuppressFileSystems(value = "ExtrasFS")
 public class NodeTests extends ESTestCase {
 
@@ -154,9 +153,12 @@ public void testCloseOnOutstandingTask() throws Exception {
         node.start();
         ThreadPool threadpool = node.injector().getInstance(ThreadPool.class);
         AtomicBoolean shouldRun = new AtomicBoolean(true);
+        CountDownLatch threadRunning = new CountDownLatch(1);
         threadpool.executor(ThreadPool.Names.SEARCH).execute(() -> {
+            threadRunning.countDown();
             while (shouldRun.get());
         });
+        threadRunning.await();
         node.close();
         shouldRun.set(false);
         assertTrue(node.awaitClose(1, TimeUnit.DAYS));
@@ -167,12 +169,17 @@ public void testAwaitCloseTimeoutsOnNonInterruptibleTask() throws Exception {
         node.start();
         ThreadPool threadpool = node.injector().getInstance(ThreadPool.class);
         AtomicBoolean shouldRun = new AtomicBoolean(true);
+        CountDownLatch threadRunning = new CountDownLatch(1);
         threadpool.executor(ThreadPool.Names.SEARCH).execute(() -> {
+            threadRunning.countDown();
             while (shouldRun.get());
         });
+        threadRunning.await();
         node.close();
         assertFalse(node.awaitClose(0, TimeUnit.MILLISECONDS));
         shouldRun.set(false);
+        // call this again to ensure we terminate and close the threadpool
+        assertTrue(node.awaitClose(1, TimeUnit.DAYS));
     }
 
     public void testCloseOnInterruptibleTask() throws Exception {

test/framework/src/main/java/org/elasticsearch/test/client/NoOpClient.java

@@ -57,10 +57,15 @@ void doExecute(Action<Response> action, Request request, ActionListener<Response
 
     @Override
     public void close() {
+        boolean terminated = false;
         try {
-            ThreadPool.terminate(threadPool(), 10, TimeUnit.SECONDS);
+            terminated = ThreadPool.terminate(threadPool(), 10, TimeUnit.SECONDS);
         } catch (Exception e) {
             throw new ElasticsearchException(e.getMessage(), e);
         }
+
+        if (terminated == false) {
+            throw new IllegalStateException("threadpool was not terminated after waiting for 10 seconds");
+        }
     }
 }