[CI] Netty ThreadDeathWatcher leak in security tests

[romseygeek]

https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+6.3+multijob-windows-compatibility/63/console

com.carrotsearch.randomizedtesting.ThreadLeakError: 1 thread leaked from SUITE scope at org.elasticsearch.integration.DocumentAndFieldLevelSecurityTests: 
   1) Thread[id=2012, name=threadDeathWatcher-2-11, state=TIMED_WAITING, group=TGRP-DocumentAndFieldLevelSecurityTests]
        at java.lang.Thread.sleep(Native Method)
        at io.netty.util.ThreadDeathWatcher$Watcher.run(ThreadDeathWatcher.java:152)
        at io.netty.util.concurrent.DefaultThreadFactory$DefaultRunnableDecorator.run(DefaultThreadFactory.java:138)
        at java.lang.Thread.run(Thread.java:748)
	at __randomizedtesting.SeedInfo.seed([5B65388C360678C0]:0)

Doesn't reproduce locally.

[elasticmachine]

Pinging @elastic/es-security

[jaymode]

@tbrooks8 @jasontedor do you have any thoughts on whether there is something more we can/should do after security tests to wait for Netty ThreadDeathWatcher to terminate?

Do we wait longer? Retry waiting (ie check the return value from awaitInactivity)? We currently have this:

elasticsearch/x-pack/plugin/security/src/test/java/org/elasticsearch/test/SecurityIntegTestCase.java

Lines 161 to 183 in 0b54455

	@AfterClass
	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();
	}
	}

[jasontedor]

@jaymode I think the problem here is that the @AfterClass on SecurityIntegTestCase run before the @AfterClass on ESIntegTestCase. This means that we are waiting on these threads to be inactive before the cluster has stopped. That means these threads might not be inactive and so they can not be stopped. Instead, I think we want to shutdown the cluster first and then stop these threads. Since JUnit always runs @AfterClass of a sub-class before @AfterClass of a super-class, we have to play a game to get this cleanup to run after the cluster has stopped. I propose something like this:

diff --git a/test/framework/src/main/java/org/elasticsearch/test/ESIntegTestCase.java b/test/framework/src/main/java/org/elasticsearch/test/ESIntegTestCase.java
index 7210fadd7ea..b543672c205 100644
--- a/test/framework/src/main/java/org/elasticsearch/test/ESIntegTestCase.java
+++ b/test/framework/src/main/java/org/elasticsearch/test/ESIntegTestCase.java
@@ -357,6 +357,12 @@ public abstract class ESIntegTestCase extends ESTestCase {
         initializeSuiteScope();
     }
 
+    private static final List<Runnable> afterClassRunnables = new ArrayList<>();
+
+    protected static void registerAfterClass(final Runnable afterClassRunnable) {
+        afterClassRunnables.add(afterClassRunnable);
+    }
+
     @Override
     protected final boolean enableWarningsCheck() {
         //In an integ test it doesn't make sense to keep track of warnings: if the cluster is external the warnings are in another jvm,
@@ -2101,6 +2107,9 @@ public abstract class ESIntegTestCase extends ESTestCase {
             }
         } else {
             clearClusters();
+            for (final Runnable afterClassRunnable : afterClassRunnables) {
+                afterClassRunnable.run();
+            }
         }
         SUITE_SEED = null;
         currentCluster = null;
* Unmerged path x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/CreateAndFollowIndexAction.java
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 00b46b332cb..cb338aecebe 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
@@ -154,6 +154,30 @@ public abstract class SecurityIntegTestCase extends ESIntegTestCase {
         }
     }
 
+    @BeforeClass
+    public static void addAfterClassRunnable() {
+        // Wait for the network threads to finish otherwise there is the possibility that one of
+        // the threads lingers and trips the thread leak detector
+        ESIntegTestCase.registerAfterClass(() -> {
+            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();
+            }
+        });
+    }
+
     /**
      * Set the static default settings to null to prevent a memory leak. The test framework also checks for memory leaks
      * and computes the size, this can cause issues when running with the security manager as it tries to do reflection
@@ -163,24 +187,6 @@ public abstract class SecurityIntegTestCase extends ESIntegTestCase {
     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

With this change, I notice that the threads are cleaned up much faster and I think this will address these spurious failures from the threads leaking.

What do you think of an approach like this?

[jaymode]

Good point @jasontedor. I wonder if we should add something like this to our test framework in lieu of using something like a JUnit ExternalResource class rule? I may be misunderstanding how the class rule works though, but I'll give it a shot today to see the execution order.

[jasontedor]

@jaymode I agree, I think that an external resource @Rule is worth pursuing. Thanks for picking this up.