[io.net] Fix shutdown hang: keep the shared thread pool alive until both clients are stopped (#5772)

Signed-off-by: Martin Littkovsky <2018turtle@proton.me>

Author: ML19821

bundles/org.openhab.core.io.net/src/main/java/org/openhab/core/io/net/http/internal/WebClientFactoryImpl.java

@@ -136,6 +136,13 @@ protected void deactivate() {
             commonWebSocketClient = null;
             logger.debug("Jetty shared web socket client stopped");
         }
+        if (threadPool != null) {
+            try {
+                threadPool.stop();
+            } catch (Exception e) {
+                logger.error("error while stopping shared Jetty thread pool", e);
+            }
+        }
         threadPool = null;
     }
 
@@ -227,15 +234,26 @@ private synchronized void initialize() {
         try {
             if (threadPool == null) {
                 threadPool = createThreadPool("common", minThreadsShared, maxThreadsShared, keepAliveTimeoutShared);
+                // The pool is shared between the http client and the web socket client. Start it
+                // ourselves so both Jetty containers treat it as an unmanaged bean and neither
+                // stops it while the other client is still using it (see the shared-bean rule in
+                // Jetty's ContainerLifeCycle javadoc). It is stopped explicitly in deactivate().
+                try {
+                    threadPool.start();
+                    // Set the stop timeout right after starting the pool we now own. We need the
+                    // stop timeout in order to prevent blocking the deactivation of this
+                    // component, see https://github.com/eclipse/smarthome/issues/6632
+                    threadPool.setStopTimeout(0);
+                } catch (Exception e) {
+                    // roll back so a later initialize() retry recreates the pool
+                    // instead of reusing a dead one
+                    threadPool = null;
+                    throw e;
+                }
             }
 
             if (commonHttpClient == null) {
                 commonHttpClient = createHttpClientInternal("common", null, true, threadPool);
-                // we need to set the stop timeout AFTER the client has been started, because
-                // otherwise the Jetty client sets it back to the default value.
-                // We need the stop timeout in order to prevent blocking the deactivation of this
-                // component, see https://github.com/eclipse/smarthome/issues/6632
-                threadPool.setStopTimeout(0);
                 logger.debug("Jetty shared http client created");
             }
 

bundles/org.openhab.core.io.net/src/test/java/org/openhab/core/io/net/http/internal/WebClientFactoryImplTest.java

@@ -26,6 +26,7 @@
 import java.util.Map;
 import java.util.concurrent.ArrayBlockingQueue;
 import java.util.concurrent.ExecutionException;
+import java.util.concurrent.Executor;
 import java.util.concurrent.ThreadPoolExecutor;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.TimeoutException;
@@ -36,6 +37,7 @@
 import org.eclipse.jdt.annotation.NonNullByDefault;
 import org.eclipse.jetty.client.HttpClient;
 import org.eclipse.jetty.client.api.ContentResponse;
+import org.eclipse.jetty.util.thread.QueuedThreadPool;
 import org.eclipse.jetty.websocket.client.WebSocketClient;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
@@ -68,12 +70,26 @@ public void setup() {
 
     @AfterEach
     public void tearDown() throws InterruptedException {
-        // Sometimes a java.nio.channels.ClosedSelectorException occurs when the commonWebSocketClient
-        // is stopped while its threads are still starting. This would cause webClientFactory.deactivate()
-        // to block forever so continue if it has not completed after 2 seconds.
-        Thread deactivateThread = new Thread(() -> webClientFactory.deactivate());
+        // Regression check for the former shutdown hang: stopping the common HTTP client used to
+        // tear down the shared thread pool underneath the common WebSocketClient, whose selector
+        // then died with a ClosedSelectorException and deactivate() blocked forever.
+        // Deactivation must always complete.
+        deactivateWithTimeout();
+    }
+
+    private void deactivateWithTimeout() throws InterruptedException {
+        Thread deactivateThread = new Thread(() -> webClientFactory.deactivate(), "webClientFactory-deactivate");
+        deactivateThread.setDaemon(true);
         deactivateThread.start();
-        deactivateThread.join(2000);
+        deactivateThread.join(10_000);
+
+        // Capture the verdict before interrupting: the interrupt is cleanup, so an interrupt that
+        // happens to unblock the deactivation must not turn an observed timeout into a pass.
+        boolean timedOut = deactivateThread.isAlive();
+        if (timedOut) {
+            deactivateThread.interrupt();
+        }
+        assertThat("deactivate() did not complete", timedOut, is(false));
     }
 
     @Test
@@ -85,6 +101,26 @@ public void testGetClients() throws Exception {
         assertThat(webSocketClient, is(notNullValue()));
     }
 
+    @Test
+    public void testSharedThreadPoolIsUnmanagedAndStoppedByDeactivate() throws Exception {
+        HttpClient httpClient = webClientFactory.getCommonHttpClient();
+        WebSocketClient webSocketClient = webClientFactory.getCommonWebSocketClient();
+
+        Executor executor = httpClient.getExecutor();
+        assertThat(executor, is(instanceOf(QueuedThreadPool.class)));
+        QueuedThreadPool sharedPool = (QueuedThreadPool) executor;
+
+        // both clients use the same pool, but neither container manages its lifecycle
+        assertThat(webSocketClient.getHttpClient().getExecutor(), is(sameInstance(executor)));
+        assertThat(httpClient.isManaged(sharedPool), is(false));
+        assertThat(webSocketClient.getHttpClient().isManaged(sharedPool), is(false));
+        assertThat(sharedPool.isRunning(), is(true));
+
+        // deactivation stops the pool itself, after both clients
+        deactivateWithTimeout();
+        assertThat(sharedPool.isRunning(), is(false));
+    }
+
     @Disabled("connecting to the outside world makes this test flaky")
     @Test
     public void testCommonClientUsesExtensibleTrustManager() throws Exception {