[io.net] Address review: set stop timeout at pool start, add regression test

Move setStopTimeout(0) directly after the explicit pool start - the old
comment tied it to the HTTP client's start, which no longer starts the
pool - so it also applies when common-client initialization fails later.
Replace the 2-second continue-anyway workaround in the test's tearDown()
with an assertion that deactivate() completes, and add a test pinning
the ownership model: both clients share the pool unmanaged, and
deactivate() itself stops it.

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

Author: Martin Littkovsky

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

@@ -240,6 +240,10 @@ private synchronized void initialize() {
                 // 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
@@ -250,11 +254,6 @@ private synchronized void initialize() {
 
             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,14 @@ 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.
+        // 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.
         Thread deactivateThread = new Thread(() -> webClientFactory.deactivate());
         deactivateThread.start();
-        deactivateThread.join(2000);
+        deactivateThread.join(10_000);
+        assertThat("deactivate() did not complete", deactivateThread.isAlive(), is(false));
     }
 
     @Test
@@ -85,6 +89,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
+        webClientFactory.deactivate();
+        assertThat(sharedPool.isRunning(), is(false));
+    }
+
     @Disabled("connecting to the outside world makes this test flaky")
     @Test
     public void testCommonClientUsesExtensibleTrustManager() throws Exception {