From bf94d2d441fec874d530d761a2d4ac5ad02db633 Mon Sep 17 00:00:00 2001
From: Luca Abbati <luca.abbati@datadoghq.com>
Date: Wed, 3 Jul 2019 14:49:19 +0200
Subject: [PATCH 1/4] Enable launch of jmxfetch app's executors as daemon,
 default to NO-daemon

When embedding the jmxfetch app in dd-trce-java we noticed that even if
we it in a thread marked as daemon the jmxfetch app was not terminating
when the main method exited.
After some investigation we noticed that threads in tasks executors were
keeping the thread alive.
This PR DOES NOT change the default behavior, were executors are run as
non-daemons. Adds the possibility, though, to be configured to be
executed as daemon.
---
 src/main/java/org/datadog/jmxfetch/App.java   | 31 +++++++++++++++----
 .../java/org/datadog/jmxfetch/AppConfig.java  | 11 +++++++
 .../datadog/jmxfetch/tasks/TaskProcessor.java |  2 +-
 3 files changed, 37 insertions(+), 7 deletions(-)

diff --git a/src/main/java/org/datadog/jmxfetch/App.java b/src/main/java/org/datadog/jmxfetch/App.java
index 37df8e394..7b92c7b3d 100644
--- a/src/main/java/org/datadog/jmxfetch/App.java
+++ b/src/main/java/org/datadog/jmxfetch/App.java
@@ -48,6 +48,7 @@
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 import java.util.concurrent.Future;
+import java.util.concurrent.ThreadFactory;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.regex.Matcher;
@@ -89,12 +90,12 @@ public App(AppConfig appConfig) {
         this.appConfig = appConfig;
 
         ExecutorService collectionThreadPool =
-                Executors.newFixedThreadPool(appConfig.getThreadPoolSize());
+                buildExecutorService(appConfig.getThreadPoolSize());
         collectionProcessor =
                 new TaskProcessor(collectionThreadPool, appConfig.getReporter());
 
         ExecutorService recoveryThreadPool =
-                Executors.newFixedThreadPool(appConfig.getReconnectionThreadPoolSize());
+                buildExecutorService(appConfig.getReconnectionThreadPoolSize());
         recoveryProcessor = new TaskProcessor(recoveryThreadPool, appConfig.getReporter());
 
         // setup client
@@ -263,7 +264,7 @@ private void clearInstances(Collection<Instance> instances) {
                         + "previous one hogging threads");
                 recoveryProcessor.stop();
                 recoveryProcessor.setThreadPoolExecutor(
-                        Executors.newFixedThreadPool(appConfig.getReconnectionThreadPoolSize()));
+                        buildExecutorService(appConfig.getReconnectionThreadPoolSize()));
             }
 
             List<TaskStatusHandler> statuses =
@@ -290,6 +291,24 @@ public TaskStatusHandler invoke(
         }
     }
 
+    /**
+     * Builds an {@link ExecutorService} of the specified fixed size. Threads will be created
+     * and executed as daemons the if {@link AppConfig#isDaemon()} is true. Defaults to false.
+     *
+     * @param size The thread pool size
+     * @return The create executor
+     */
+    private ExecutorService buildExecutorService(int size) {
+        return Executors.newFixedThreadPool(size, new ThreadFactory() {
+            @Override
+            public Thread newThread(Runnable runnable) {
+                Thread thread = Executors.defaultThreadFactory().newThread(runnable);
+                thread.setDaemon(appConfig.isDaemon());
+                return thread;
+            }
+        });
+    }
+
     private String getAutoDiscoveryName(String config) {
         String[] splitted = config.split(System.getProperty("line.separator"), 2);
 
@@ -470,7 +489,7 @@ public void doIteration() {
                         + "previous one hogging threads");
                 collectionProcessor.stop();
                 collectionProcessor.setThreadPoolExecutor(
-                        Executors.newFixedThreadPool(appConfig.getThreadPoolSize()));
+                        buildExecutorService(appConfig.getThreadPoolSize()));
             }
 
             List<TaskStatusHandler> statuses =
@@ -556,7 +575,7 @@ private void fixBrokenInstances(Reporter reporter) {
                         + "previous one hogging threads");
                 recoveryProcessor.stop();
                 recoveryProcessor.setThreadPoolExecutor(
-                        Executors.newFixedThreadPool(appConfig.getReconnectionThreadPoolSize()));
+                        buildExecutorService(appConfig.getReconnectionThreadPoolSize()));
             }
 
             Collections.shuffle(fixInstanceTasks);
@@ -885,7 +904,7 @@ public void init(boolean forceNewConnection) {
                         + "previous one hogging threads");
                 recoveryProcessor.stop();
                 recoveryProcessor.setThreadPoolExecutor(
-                        Executors.newFixedThreadPool(appConfig.getReconnectionThreadPoolSize()));
+                        buildExecutorService(appConfig.getReconnectionThreadPoolSize()));
             }
 
             List<TaskStatusHandler> statuses =
diff --git a/src/main/java/org/datadog/jmxfetch/AppConfig.java b/src/main/java/org/datadog/jmxfetch/AppConfig.java
index 9185fc547..ef3348e86 100644
--- a/src/main/java/org/datadog/jmxfetch/AppConfig.java
+++ b/src/main/java/org/datadog/jmxfetch/AppConfig.java
@@ -207,6 +207,13 @@ public class AppConfig {
     @Builder.Default
     private boolean targetDirectInstances = false;
 
+    /**
+     * Boolean setting to determine whether to ignore jvm_direct instances.
+     * If set to true, other instances will be ignored.
+     */
+    @Builder.Default
+    private boolean daemon = false;
+
     // This is used by things like APM agent to provide configuration from resources
     private List<String> instanceConfigResources;
     // This is used by things like APM agent to provide metric configuration from resources
@@ -365,4 +372,8 @@ public Integer getRefreshBeansPeriod() {
     public Map<String, String> getGlobalTags() {
         return globalTags;
     }
+
+    public boolean isDaemon() {
+        return daemon;
+    }
 }
diff --git a/src/main/java/org/datadog/jmxfetch/tasks/TaskProcessor.java b/src/main/java/org/datadog/jmxfetch/tasks/TaskProcessor.java
index ad57573ee..f3fc1fb48 100644
--- a/src/main/java/org/datadog/jmxfetch/tasks/TaskProcessor.java
+++ b/src/main/java/org/datadog/jmxfetch/tasks/TaskProcessor.java
@@ -37,7 +37,7 @@ public void setThreadPoolExecutor(ExecutorService executor) {
      * */
     public boolean ready() {
         ThreadPoolExecutor tpe = (ThreadPoolExecutor) threadPoolExecutor;
-        return !(tpe.getMaximumPoolSize() == tpe.getActiveCount());
+        return !tpe.isTerminated() && !(tpe.getMaximumPoolSize() == tpe.getActiveCount());
     }
 
     /**

From df44c6f1f3f20d9d960140316c8c00a1d56e0b33 Mon Sep 17 00:00:00 2001
From: Luca Abbati <luca.abbati@datadoghq.com>
Date: Wed, 3 Jul 2019 17:56:47 +0200
Subject: [PATCH 2/4] Improve api docs for new run as daemon functionality

---
 src/main/java/org/datadog/jmxfetch/App.java       | 2 +-
 src/main/java/org/datadog/jmxfetch/AppConfig.java | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/main/java/org/datadog/jmxfetch/App.java b/src/main/java/org/datadog/jmxfetch/App.java
index 7b92c7b3d..d386bb0b0 100644
--- a/src/main/java/org/datadog/jmxfetch/App.java
+++ b/src/main/java/org/datadog/jmxfetch/App.java
@@ -293,7 +293,7 @@ public TaskStatusHandler invoke(
 
     /**
      * Builds an {@link ExecutorService} of the specified fixed size. Threads will be created
-     * and executed as daemons the if {@link AppConfig#isDaemon()} is true. Defaults to false.
+     * and executed as daemons if {@link AppConfig#isDaemon()} is true. Defaults to false.
      *
      * @param size The thread pool size
      * @return The create executor
diff --git a/src/main/java/org/datadog/jmxfetch/AppConfig.java b/src/main/java/org/datadog/jmxfetch/AppConfig.java
index ef3348e86..39e8fa4cc 100644
--- a/src/main/java/org/datadog/jmxfetch/AppConfig.java
+++ b/src/main/java/org/datadog/jmxfetch/AppConfig.java
@@ -373,6 +373,9 @@ public Map<String, String> getGlobalTags() {
         return globalTags;
     }
 
+    /**
+     * @return Whether or not internal threads will be run as daemon.
+     */
     public boolean isDaemon() {
         return daemon;
     }

From d0b6bb7bd0a3a86208d7f05601083ad50b49e842 Mon Sep 17 00:00:00 2001
From: Luca Abbati <luca.abbati@datadoghq.com>
Date: Thu, 4 Jul 2019 15:24:03 +0200
Subject: [PATCH 3/4] Improve explanation in comment for the new daemon setting
 definition

---
 src/main/java/org/datadog/jmxfetch/AppConfig.java | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/main/java/org/datadog/jmxfetch/AppConfig.java b/src/main/java/org/datadog/jmxfetch/AppConfig.java
index 39e8fa4cc..5ad638a05 100644
--- a/src/main/java/org/datadog/jmxfetch/AppConfig.java
+++ b/src/main/java/org/datadog/jmxfetch/AppConfig.java
@@ -208,8 +208,9 @@ public class AppConfig {
     private boolean targetDirectInstances = false;
 
     /**
-     * Boolean setting to determine whether to ignore jvm_direct instances.
-     * If set to true, other instances will be ignored.
+     * Boolean setting to determine whether internal executors are launched as daemons or not.
+     * This is usefull when JMXFetch is embedded in a client app, e.g. for the java tracer,
+     * so that the client app's exit doesn't block on the termination of these internal threads.
      */
     @Builder.Default
     private boolean daemon = false;

From f5de3d78857c06bf3b1e33057fd5580e362f50c2 Mon Sep 17 00:00:00 2001
From: Luca Abbati <luca.abbati@datadoghq.com>
Date: Thu, 4 Jul 2019 15:24:35 +0200
Subject: [PATCH 4/4] Fix typo

---
 src/main/java/org/datadog/jmxfetch/AppConfig.java | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/main/java/org/datadog/jmxfetch/AppConfig.java b/src/main/java/org/datadog/jmxfetch/AppConfig.java
index 5ad638a05..7817d102a 100644
--- a/src/main/java/org/datadog/jmxfetch/AppConfig.java
+++ b/src/main/java/org/datadog/jmxfetch/AppConfig.java
@@ -209,7 +209,7 @@ public class AppConfig {
 
     /**
      * Boolean setting to determine whether internal executors are launched as daemons or not.
-     * This is usefull when JMXFetch is embedded in a client app, e.g. for the java tracer,
+     * This is useful when JMXFetch is embedded in a client app, e.g. for the java tracer,
      * so that the client app's exit doesn't block on the termination of these internal threads.
      */
     @Builder.Default