KMeansClustering cause thread leak

[gogo111007]

Issue Description

i use KMeansClustering like KMeansTest.java
every client request, i get new kmeans object by KMeansClustering.setup()
but there is no way to destroy threads allocate by KMeansClustering
after thousands or more request, too many threads cause crash

Version Information

• deeplearning4j ver newest
• windows 7 and centos 6.x

[jentfoo]

I recently stumbled across this too. The issue is in BaseClusteringAlgorithm. It constructs an ExecutorService but provides no way to free it.

https://github.com/deeplearning4j/deeplearning4j/blob/master/deeplearning4j-nearestneighbors-parent/nearestneighbor-core/src/main/java/org/deeplearning4j/clustering/algorithm/BaseClusteringAlgorithm.java#L70

I would be happy to help provide a PR to solve this if the maintainers give me some direction as to how this should be resolved.

the jdk attempts to implement a finalizer in ThreadPoolExecutor to shutdown the pool if it is garbage collected, however this does not work correctly if threads are started since they also hold a reference to their parent pool (and thus the finalizer is never invoked even if BaseClusteringAlgorithm is garbage collected).

If you wanted to rely on garbage collection I see two possible options:

1. Selfish plug of using a better thread pool. Thread pools (PriorityScheduler for example) from my threadly project do not suffer from this issue. They can be collected normally, or you can use CentralThreadlyPool as a way to avoid needing to manage the lifecycle at all.
2. You can implement a finalizer in BaseClusteringAlgorithm which invokes a shutdown to it's retained pool.

I personally don't like depending on finalizers. My personal preference (though potentially unpopular) would be to have people supply an Executor (Executor is way better than ExecutorService if you can), and then put the thread pool management on the user.

If you are willing to depend on threadly, if an executor is not provided using a threadly executor can provide the ability to fall back to garbage collection for releasing the created threads.

[jentfoo]

I guess after looking at ExecutorServiceProvider maybe the original intention was to set instance (which currently goes unused) and reuse that instance.

[saudet]

@jentfoo Thanks for the input! Using threadly looks like a good technical solution, but I think the Mozilla license is a problem, @agibsonccc ?

[agibsonccc]

We follow apache legal guidelines: https://www.apache.org/legal/resolved.html It doesn't appear to conflict with the EPL, I'd be ok with this in that case.

[jentfoo]

@agibsonccc and @saudet Thanks for taking a look. One of the primary reasons for choosing MPL 2.0 was its compatibility with other licenses. It seemed like a license that would be a nice fit for a library like threadly. I want anyone who wants to be able to use threadly, my only reason for not using apache is I want development on the project to remain open source and encourage modifications needed to be merged to the upstream project if possible.

So in short, I don't see any reason it should be a problem either. I was going to provide a PR for #4781 as well, so if you wanted I could include adding threadly and any other concurrency cleanup things I might notice (all open for discussion of course, but to give us a starting point for that discussion).

[saudet]

Yes sounds good, go ahead with the PR, thanks!

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.