Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down Expand Up @@ -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) {
Comment thread
wborn marked this conversation as resolved.
// 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");
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -68,12 +70,22 @@ 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 {
// Regression-safe: run deactivate() on a DAEMON thread so a blocked

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This helper now has more commentary than the logic needs, and much of it narrates what setDaemon, join, interrupt, and the assertion already make clear. The non-obvious invariant worth preserving is that timedOut must be captured before interrupt() so cleanup cannot turn a timeout into a passing test. Could the surrounding comments be removed or condensed around that point? That would keep the regression rationale while making the test much easier to scan.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point — condensed in f2f2810. The helper is down to the one invariant that is genuinely not visible from the code:

// 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.

Everything else was narration of setDaemon, join, interrupt and the assertion, and is gone. Bundle tests still 40 run, 0 failures.

// deactivation can neither keep the test JVM alive nor leak a running
// non-daemon thread into subsequent tests; fail loudly via the assert.
Thread deactivateThread = new Thread(() -> webClientFactory.deactivate(), "webClientFactory-deactivate");
deactivateThread.setDaemon(true);
deactivateThread.start();
deactivateThread.join(2000);
deactivateThread.join(10_000);
Comment thread
wborn marked this conversation as resolved.
assertThat("deactivate() did not complete", deactivateThread.isAlive(), is(false));
}

@Test
Expand All @@ -85,6 +97,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 {
Expand Down