From 6924a15176d3d26fd0fe19c0612b2b2185f0c6ab Mon Sep 17 00:00:00 2001 From: jaymode Date: Mon, 21 May 2018 12:50:07 -0600 Subject: [PATCH] Test: wait for netty threads in a JUnit ClassRule This commit changes the wait for a few netty threads to wait for these threads to complete after the cluster has stopped. Previously, we were waiting for these threads before the cluster was actually stopped; the cluster is stopped in an AfterClass method of ESIntegTestCase, while the wait was performed in the AfterClass of a class that extended ESIntegTestCase, which is always executed before the AfterClass of ESIntegTestCase. Now, the wait is contained in an ExternalResource ClassRule that implements the waiting for the threads to terminate in the after method. This rule is executed after the AfterClass method in ESIntegTestCase. The same fix has also been applied in SecuritySingleNodeTestCase. Closes #30301 --- .../test/SecurityIntegTestCase.java | 48 +++++++++++------- .../test/SecuritySingleNodeTestCase.java | 49 ++++++++++++------- 2 files changed, 60 insertions(+), 37 deletions(-) diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/test/SecurityIntegTestCase.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/test/SecurityIntegTestCase.java index 00b46b332cb7c..815f26942767a 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/test/SecurityIntegTestCase.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/test/SecurityIntegTestCase.java @@ -47,6 +47,7 @@ import org.junit.AfterClass; import org.junit.Before; import org.junit.BeforeClass; +import org.junit.ClassRule; import org.junit.Rule; import org.junit.rules.ExternalResource; @@ -163,24 +164,6 @@ public static void initDefaultSettings() { public static void destroyDefaultSettings() { SECURITY_DEFAULT_SETTINGS = null; customSecuritySettingsSource = null; - // Wait for the network threads to finish otherwise there is the possibility that one of - // the threads lingers and trips the thread leak detector - try { - GlobalEventExecutor.INSTANCE.awaitInactivity(5, TimeUnit.SECONDS); - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - } catch (IllegalStateException e) { - if (e.getMessage().equals("thread was not started") == false) { - throw e; - } - // ignore since the thread was never started - } - - try { - ThreadDeathWatcher.awaitInactivity(5, TimeUnit.SECONDS); - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - } } @Rule @@ -204,6 +187,35 @@ protected void before() throws Throwable { } }; + /** + * A JUnit class level rule that runs after the AfterClass method in {@link ESIntegTestCase}, + * which stops the cluster. After the cluster is stopped, there are a few netty threads that + * can linger, so we wait for them to finish otherwise these lingering threads can intermittently + * trigger the thread leak detector + */ + @ClassRule + public static final ExternalResource STOP_NETTY_RESOURCE = new ExternalResource() { + @Override + protected void after() { + try { + GlobalEventExecutor.INSTANCE.awaitInactivity(5, TimeUnit.SECONDS); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } catch (IllegalStateException e) { + if (e.getMessage().equals("thread was not started") == false) { + throw e; + } + // ignore since the thread was never started + } + + try { + ThreadDeathWatcher.awaitInactivity(5, TimeUnit.SECONDS); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + } + }; + @Before //before methods from the superclass are run before this, which means that the current cluster is ready to go public void assertXPackIsInstalled() { diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/test/SecuritySingleNodeTestCase.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/test/SecuritySingleNodeTestCase.java index 1ee654c0baffc..cda627806e7b5 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/test/SecuritySingleNodeTestCase.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/test/SecuritySingleNodeTestCase.java @@ -26,6 +26,7 @@ import org.junit.AfterClass; import org.junit.Before; import org.junit.BeforeClass; +import org.junit.ClassRule; import org.junit.Rule; import org.junit.rules.ExternalResource; @@ -97,25 +98,6 @@ private static void tearDownRestClient() { IOUtils.closeWhileHandlingException(restClient); restClient = null; } - - // Wait for the network threads to finish otherwise there is the possibility that one of - // the threads lingers and trips the thread leak detector - try { - GlobalEventExecutor.INSTANCE.awaitInactivity(5, TimeUnit.SECONDS); - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - } catch (IllegalStateException e) { - if (e.getMessage().equals("thread was not started") == false) { - throw e; - } - // ignore since the thread was never started - } - - try { - ThreadDeathWatcher.awaitInactivity(5, TimeUnit.SECONDS); - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - } } @Rule @@ -130,6 +112,35 @@ protected void before() { } }; + /** + * A JUnit class level rule that runs after the AfterClass method in {@link ESIntegTestCase}, + * which stops the cluster. After the cluster is stopped, there are a few netty threads that + * can linger, so we wait for them to finish otherwise these lingering threads can intermittently + * trigger the thread leak detector + */ + @ClassRule + public static final ExternalResource STOP_NETTY_RESOURCE = new ExternalResource() { + @Override + protected void after() { + try { + GlobalEventExecutor.INSTANCE.awaitInactivity(5, TimeUnit.SECONDS); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } catch (IllegalStateException e) { + if (e.getMessage().equals("thread was not started") == false) { + throw e; + } + // ignore since the thread was never started + } + + try { + ThreadDeathWatcher.awaitInactivity(5, TimeUnit.SECONDS); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + } + }; + @Before //before methods from the superclass are run before this, which means that the current cluster is ready to go public void assertXPackIsInstalled() {