-
-
Notifications
You must be signed in to change notification settings - Fork 470
[io.net] Fix shutdown hang: keep the shared thread pool alive until both clients are stopped #5772
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 4 commits
0e46566
a256cff
d7f75d1
0c2b82d
f2f2810
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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,33 @@ 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 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair point — condensed in // 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 |
||
| // 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); | ||
|
wborn marked this conversation as resolved.
|
||
|
|
||
| // Capture the verdict BEFORE interrupting: the interrupt is cleanup, not a second chance. | ||
| // If it unblocked the deactivation, this test still has to report the timeout it observed, | ||
| // otherwise a real shutdown regression would pass. | ||
| boolean timedOut = deactivateThread.isAlive(); | ||
| if (timedOut) { | ||
| // Best effort to release a blocked deactivation instead of leaving it running next to | ||
| // the following tests; the daemon flag above remains the final safeguard for the case | ||
| // where Jetty does not react to the interrupt at all. | ||
| deactivateThread.interrupt(); | ||
| } | ||
| assertThat("deactivate() did not complete", timedOut, is(false)); | ||
| } | ||
|
|
||
| @Test | ||
|
|
@@ -85,6 +108,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 { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.