Executor service prevents JVM termination without explicit CacheManager.shutdown

Type: Bug
Status: Resolved
Priority: 2 Major
Resolution: Fixed
Component/s: ehcache-core
Labels:

Assignee: ljacomet
Reporter: sgriffin
Created: November 19, 2013
Votes: 1
Watchers: 7
Updated:
December 23, 2013
Resolved:
December 06, 2013

Description

The ExecutorService used for read-through cache loading in the net.sf.ehcache.Cache class has a couple problems with its use of ThreadPoolExecutor that cause unexpected cache loading behavior and an undesirable need to force CacheManager.shutdown to consume properly.

First, the comments in the code do not match what the actual code is doing. The comments on the ExecutorService field states:

/**
 * A ThreadPoolExecutor which uses a thread pool to schedule loads in the order in which they are requested.
 * <p/>
 * Each cache has its own one of these, if required. Because the Core Thread Pool is zero, no threads
 * are used until actually needed. Threads are added to the pool up to a maximum of 10. The keep alive
 * time is 60 seconds, after which, if they are not required they will be stopped and collected.
...
 */

This description matches what I expected to happen, but it’s not what the code is actually doing:

executorService = new ThreadPoolExecutor(EXECUTOR_CORE_POOL_SIZE, EXECUTOR_MAXIMUM_POOL_SIZE, EXECUTOR_KEEP_ALIVE_TIME,
                            TimeUnit.MILLISECONDS, new LinkedBlockingQueue(), new NamedThreadFactory("Cache Executor Service"));

These contants are defined as:

private static final int EXECUTOR_KEEP_ALIVE_TIME = 60000;
private static final int EXECUTOR_MAXIMUM_POOL_SIZE = Math.min(10, Runtime.getRuntime().availableProcessors());
private static final int EXECUTOR_CORE_POOL_SIZE = 1;

This code was copied from trunk. There are two key things to notice.

First, the Core Pool Size is set to *1* not *0*. This means that even if the keep alive is expired on this thread, the pool will never drop below this 1 thread once it’s allocated. Because this 1 thread is non-daemon, the JVM *will not terminate* normally unless Cache.dispose() is called.

Second, the queue implementation that’s used is LinkedBlockingQueue, which means that the 1 Core thread will be the *only* thread ever used and the EXECUTOR_MAXIMUM_POOL_SIZE has no actual impact on the number of threads used in the pool. See the JavaDoc on [ThreadPoolExecutor

http://docs.oracle.com/javase/6/docs/api/java/util/concurrent/ThreadPoolExecutor.html] for more information.

While I know that consumers _should_ call CacheManager.shutdown(), it would be nice if it wasn’t _required_ to keep from leaving orphaned JVMs. This is particularly impactful in a Hadoop map/reduce environment. At the very least the [documentation

http://ehcache.org/documentation/operations/shutdown] should be updated to clearly state that you must call shutdown() in all use cases and not just “if you are using persistent disk stores (Open Source), or distributed caching, care should be taken to shutdown Ehcache.” like it currently states.

Comments

Kavitha Tupelly 2013-11-25

Louis, Can you please take a initial review at this issue.

Alexander Snaps 2013-12-03

I’ll let Louis have the final word, but having had a quick look at this, I agree this isn’t optimal. The problem is changing this code now though, not quite sure we can change the loading to happen in parallel (and populate the cache in random order). So I’d stick to letting the ThreadPoolExecutor time out the core thread and, make the thread daemons, like so:

Index: src/main/java/net/sf/ehcache/Cache.java
===================================================================
--- src/main/java/net/sf/ehcache/Cache.java	(revision 8497)
+++ src/main/java/net/sf/ehcache/Cache.java	(working copy)
@@ -3433,7 +3433,8 @@
                 } else {
                     // we can create Threads
                     executorService = new ThreadPoolExecutor(EXECUTOR_CORE_POOL_SIZE, EXECUTOR_MAXIMUM_POOL_SIZE, EXECUTOR_KEEP_ALIVE_TIME,
-                            TimeUnit.MILLISECONDS, new LinkedBlockingQueue(), new NamedThreadFactory("Cache Executor Service"));
+                            TimeUnit.MILLISECONDS, new LinkedBlockingQueue(), new NamedThreadFactory("Cache Executor Service", true));
+                    ((ThreadPoolExecutor)executorService).allowCoreThreadTimeOut(true);
                 }
             }
         }
Index: src/main/java/net/sf/ehcache/util/NamedThreadFactory.java
===================================================================
--- src/main/java/net/sf/ehcache/util/NamedThreadFactory.java	(revision 8497)
+++ src/main/java/net/sf/ehcache/util/NamedThreadFactory.java	(working copy)
@@ -31,6 +31,7 @@
 
     private static AtomicInteger threadNumber = new AtomicInteger(1);
     private final String namePrefix;
+    private final boolean daemon;
 
     /**
      * Constructor accepting the prefix of the threads that will be created by this {@link ThreadFactory}
@@ -38,15 +39,28 @@
      * @param namePrefix
      *            Prefix for names of threads
      */
-    public NamedThreadFactory(String namePrefix) {
+    public NamedThreadFactory(String namePrefix, boolean daemon) {
         this.namePrefix = namePrefix;
+        this.daemon = daemon;
+
     }
+    /**
+     * Constructor accepting the prefix of the threads that will be created by this {@link ThreadFactory}
+     *
+     * @param namePrefix
+     *            Prefix for names of threads
+     */
+    public NamedThreadFactory(String namePrefix) {
+        this(namePrefix, false);
+    }
 
     /**
      * Returns a new thread using a name as specified by this factory {@inheritDoc}
      */
     public Thread newThread(Runnable runnable) {
-        return new Thread(runnable, namePrefix + " thread-" + threadNumber.getAndIncrement());
+        final Thread thread = new Thread(runnable, namePrefix + " thread-" + threadNumber.getAndIncrement());
+        thread.setDaemon(daemon);
+        return thread;
     }
 
 }

Sean Griffin 2013-12-03

+1 to the suggested changes. I’m fine keeping the max thread pool size at 1 if the requirement is that cache loading is done in FIFO order, though in that case I do think the internal code comments (and possibly the parameters given to the ThreadPoolExecutor constructor) should be updated to reflect the accurate behavior that up to 10 threads are _not_ allocated and that a max of 1 thread is used.

Alexander Snaps 2013-12-03

btw, it will grow, in the hypothetical case the LBQ reaches the size of Integer.MAX_VALUE (as that’s the current bound). So maybe the point about keeping it to the current behavior is moot… As there is not requirement for the FIFO behavior really, other than it being as so for ever since this feature made it in I think. So do we want to break this now? I don’t think we should. Also, exposing a config to bound the LBQ to some value isn’t really going to help users much I believe. Anyways, food for thoughts… but that’s what led me to this particular proposal. Updating the javadocs to reflect whatever this then does is certainly desirable yes.

Louis Jacomet Jacomet 2013-12-06

Fixed as indicated by Alex. javadoc for executorService updated to:

    /**
     * A ThreadPoolExecutor which uses a thread pool to schedule loads in the order in which they are requested.
     * <p/>
     * Each cache can have its own executor service, if required. The keep alive time is 60 seconds, after which,
     * if the thread is not required it will be stopped and collected, as core threads are allowed to time out.
     * <p/>
     * The executorService is only used for cache loading, and is created lazily on demand to avoid unnecessary resource
     * usage.
     * <p/>
     * Use {@link #getExecutorService()} to ensure that it is initialised.
     */
    private volatile ExecutorService executorService;