Enable launch of jmxfetch app's executors as daemon, default to NO-daemon #237
Conversation
…emon 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.
| @@ -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()); | |||
There was a problem hiding this comment.
just a question: is this change needed as well for the purposes of the PR, or is it an "unrelated" improvement?
There was a problem hiding this comment.
This is required because when running as daemon the underlying thread pull executor can be terminated before the shutdown hook. The problem is that starting from the shutdown hook we check if a thread executor is ready and if not ready we recreate.
I had a false positive (ready) when actually it was terminated.
I did not had a separate method as looking at the code around I thought that adding this check to the ready function made sense.
Totally open to that, though.
There was a problem hiding this comment.
thanks for the explanation!
Hmm, looking at where the ready function is used, and the case you described: does adding the condition here actually solve the issue you mentioned?
i.e.: if the underlying thread pull executor is terminated before the shutdown hook is run, the ready function here would return false, so the logic in the App would recreate a new thread pool executor. Is that the desired behavior?
There was a problem hiding this comment.
Thanks for looking into this. This is the expected behavior based on my investigation. The thread pool is recreated (to do some final work) and then once that work is done it will automatically exit.
I tested both in web apps (i.e. long running processes) and short lived CLI apps and this is how I could observe it behaves.
Anyway this ring a bell in you I can look better into this. Do you have a specific concern I can specifically look for?
There was a problem hiding this comment.
Hi @olivielpeau, does my answer make sense to you? Please let me know
There was a problem hiding this comment.
Ok, thanks for the explanation, makes sense!
There was a problem hiding this comment.
LGTM, would appreciate a quick review from @truthbk who originally wrote this logic, to double-check I haven't missed anything :)
|
Thanks a lot everyone! Should I merge to master myself? Also, can we plan a release to include this fix? |
| return Executors.newFixedThreadPool(size, new ThreadFactory() { | ||
| @Override | ||
| public Thread newThread(Runnable runnable) { | ||
| Thread thread = Executors.defaultThreadFactory().newThread(runnable); |
There was a problem hiding this comment.
I would suggest assigning a proper name to the thread... this would have greatly helped our troubleshooting. Maybe next time someone does some work here?
When embedding the jmxfetch app in dd-trce-java we noticed that even if
we run 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.