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

[ML19821]

Description

WebClientFactoryImpl shares one QueuedThreadPool between the common Jetty HttpClient and the common WebSocketClient, but hands it over unstarted. Jetty's ContainerLifeCycle.addBean(Object) heuristic therefore makes the pool a MANAGED bean of the first client started with it (the common HttpClient, which implicitly starts the pool during its own start), while the WebSocketClient — created after the pool is already running — merely borrows it as an UNMANAGED bean. deactivate() stops the owning HttpClient first, which tears the shared pool down while the WebSocketClient's ManagedSelector is still running on it. The selector thread dies with java.nio.channels.ClosedSelectorException without draining its update queue, and the subsequent commonWebSocketClient.stop() then waits forever on the latches in Jetty 9.4's timeout-less ManagedSelector.doStop() — freezing the entire framework shutdown until the service supervisor SIGKILLs the JVM.

Observed on openHAB 5.2.1 (openHABian/RPi4, Jetty 9.4.58): intermittent shutdown hangs (5 of 8 stops on one day, identical stack), TimeoutStopSec (600 s) exhausted, SIGKILL mid-deactivation (which once truncated log4j2.xml). The hang occurs with zero active WebSocket connections — it is the pool teardown, not connection activity. Full analysis with thread dumps and the timing chain available on request.

Fingerprint during a hang:

[WARN] [org.eclipse.jetty.io.ManagedSelector] - java.nio.channels.ClosedSelectorException

"Framework stop - Equinox Container" WAITING on CountDownLatch
    org.eclipse.jetty.io.ManagedSelector.doStop(ManagedSelector.java:140)
    org.eclipse.jetty.client.HttpClient.doStop(HttpClient.java:294)
    org.eclipse.jetty.websocket.client.WebSocketClient.doStop(WebSocketClient.java:429)
    org.openhab.core.io.net.http.internal.WebClientFactoryImpl.deactivate(WebClientFactoryImpl.java:131)

The fix

Jetty's own ContainerLifeCycle javadoc prescribes it:

"If adding a bean that is shared between multiple ContainerLifeCycle instances, then it should be started before being added, so it is unmanaged, or the API must be used to explicitly set it as unmanaged."

So: start the pool in initialize() before handing it to the clients (an already-started bean is added UNMANAGED, so neither client stops it anymore), and stop it explicitly as the last step of deactivate(). If threadPool.start() fails, the field is rolled back to null so a later lazy initialize() retry recreates the pool instead of reusing a dead one. The setStopTimeout(0) mitigation from eclipse-archived/smarthome#6632 keeps covering the pool's own stop, so deactivation cannot block there either.

No runtime behavior changes: the pool ran from the first client start before; it still does.

Testing

• mvn clean install -pl bundles/org.openhab.core.io.net: BUILD SUCCESS on this branch and on the 5.2.1 tag; 39 tests, 0 failures (4 skipped, pre-existing).
• A 5.2.1-based build of this fix is deployed on the affected installation. A verification stop shows the previously-never-reached second debug line 2 ms after the first (Jetty shared http client stopped → Jetty shared web socket client stopped), no ClosedSelectorException; in every hang the second line never appeared. Longer-term observation is running with a scoped DEBUG logger on org.openhab.core.io.net.http.internal recording the deactivation order of every future shutdown.

Related: eclipse-archived/smarthome#6632 (2018 predecessor of this hang, mitigated with setStopTimeout(0)), #3315 (Jetty 9.4 EOL umbrella — this is a minimal self-contained correction until the Jetty upgrade lands).

[wborn]

This PR is being reviewed with AI first.

The reported issue looks valid and the proposed lifecycle fix is sound. Jetty's ContainerLifeCycle explicitly documents that a lifecycle bean shared by multiple containers should be started before being added, or explicitly made unmanaged.

With the current code, the common HttpClient receives the stopped thread pool first and therefore ends up managing it. The WebSocket client's internal HttpClient receives the already-running pool and only borrows it. Stopping the common HTTP client first can consequently shut down the shared executor while the WebSocket selector is still using it, which explains the observed ClosedSelectorException and subsequent shutdown hang.

Explicitly owning the shared thread pool in WebClientFactoryImpl, starting it before handing it to either client, and stopping it after both clients is cleaner than merely reversing the shutdown order because it removes the accidental lifecycle ownership relationship altogether.

I found two follow-ups:

• WebClientFactoryImplTest.tearDown() already contains a workaround for essentially this exact ClosedSelectorException/shutdown hang, but it only waits two seconds and does not assert that deactivation actually completed. It would be good to turn this into a regression test and verify that deactivation completes successfully with both shared clients initialized.
• Now that the pool is started before being given to the HTTP client, the existing comment saying setStopTimeout(0) must happen after HttpClient.start() is no longer correct. Moving that setting directly after the explicit pool start would better match the new ownership model and also ensures it is applied if common-client initialization subsequently fails.

Apart from those points, the approach looks correct.

[wborn]

One additional test-related point that could not be attached as an inline review comment because WebClientFactoryImplTest.java is unchanged in this PR:

WebClientFactoryImplTest.tearDown() already documents essentially the same ClosedSelectorException/shutdown hang this PR fixes, but it currently starts deactivate() on another thread, waits two seconds, and then continues even if deactivation is still blocked.

Could we turn this into a regression test for the fix? At minimum, I think we should assert that deactivation completes within the timeout after both common clients have been initialized. Ideally we could also verify the lifecycle ownership directly, i.e. that neither client's HttpClient manages the shared executor and that the pool is stopped by WebClientFactoryImpl.deactivate().

[ML19821]

Thanks for the thorough review — both points are addressed in a256cff (kept as a separate commit so the delta stays reviewable):

• setStopTimeout(0) now sits directly after the explicit pool start, and the stale "after the client has been started" comment is gone. As you noted, this way it also applies when common-client initialization fails later.
• The tearDown() workaround is now a real regression check: it asserts that deactivate() completes (10 s budget) instead of silently continuing. A new testSharedThreadPoolIsUnmanagedAndStoppedByDeactivate pins the ownership model directly — both clients share the same QueuedThreadPool instance, neither client container manages it, and deactivate() itself stops it.

Bundle tests: 40 run, 0 failures.

[wborn]

The previous review findings have been addressed in a256cff. The shared thread pool now owns its stop timeout independently of the HTTP client lifecycle, and the new regression coverage verifies that both clients share the same unmanaged pool and that WebClientFactoryImpl stops it explicitly.

The production-code lifecycle fix looks correct. Starting the shared pool before handing it to either Jetty client ensures that neither client owns its lifecycle, and stopping it only after both clients have been stopped removes the shutdown ordering problem that caused the original hang.

One test robustness issue remains. The timeout-protected deactivation in tearDown() can still leave a blocked non-daemon thread behind when the regression occurs, and testSharedThreadPoolIsUnmanagedAndStoppedByDeactivate() calls deactivate() directly without any timeout at all. A future shutdown regression could therefore cause the test itself to hang before tearDown() is reached.

Apart from that test concern, no further issues were found with the Jetty lifecycle fix.

[ML19821]

Good catch on the remaining hang path — addressed in d7f75d1: both the tearDown() check and the explicit call in the ownership test now go through a shared deactivateWithTimeout() helper that runs deactivate() on a daemon thread with a bounded join. A future shutdown regression now fails the assertion instead of hanging the test itself or leaking a non-daemon thread into subsequent tests and the surefire fork. Bundle tests: 40 run, 0 failures.

[wborn]

The previous review concern has been mostly addressed in d7f75d1. Both the tearDown() path and the explicit deactivation in testSharedThreadPoolIsUnmanagedAndStoppedByDeactivate() now use the same timeout-protected helper, and making the deactivation thread a daemon ensures that a shutdown regression cannot keep the test JVM alive.

The production lifecycle fix looks correct. The shared thread pool is started before either Jetty client receives it, both clients therefore treat it as unmanaged, and WebClientFactoryImpl explicitly stops the pool only after both clients have been stopped.

One test robustness issue remains; see the inline comment.

Apart from that, no further issues were found.

[wborn]

The previous review concern has been addressed in 0c2b82d. The timeout result is captured before interruption, so the interrupt remains cleanup only and cannot turn a shutdown regression into a passing test.

The production lifecycle fix and regression coverage look correct, and CI is green on the current head.

No further issues were found. A human maintainer review is still needed.

[wborn]

Additional AI review on the current head:

The Jetty lifecycle fix remains sound, the previous findings are resolved, and the current CI/SAT run is green. The static-analysis report does not contain findings for either changed class.

One non-blocking maintainability comment remains inline regarding the amount of explanatory narration in the regression-test helper.

No blocking code issues were found. A human maintainer review is still needed before merge.

[wborn]

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.

[ML19821]

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.

[wborn]

Thanks for the fix!