Fix POOL-337. Ensure cancelled eviction tasks are removed from scheduler. Includes a test case based on a patch by Reinald Verheij.
Project: http://git-wip-us.apache.org/repos/asf/commons-pool/repo Commit: http://git-wip-us.apache.org/repos/asf/commons-pool/commit/86761482 Tree: http://git-wip-us.apache.org/repos/asf/commons-pool/tree/86761482 Diff: http://git-wip-us.apache.org/repos/asf/commons-pool/diff/86761482 Branch: refs/heads/master Commit: 867614829ba6b041ca6becbad0f43cb180003404 Parents: 87683a5 Author: Mark Thomas <[email protected]> Authored: Mon May 21 12:45:18 2018 +0100 Committer: Mark Thomas <[email protected]> Committed: Mon May 21 12:51:17 2018 +0100 ---------------------------------------------------------------------- src/changes/changes.xml | 3 + .../pool2/impl/BaseGenericObjectPool.java | 16 ++- .../commons/pool2/impl/EvictionTimer.java | 19 ++-- .../commons/pool2/impl/TestEvictionTimer.java | 105 +++++++++++++++++++ 4 files changed, 133 insertions(+), 10 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/commons-pool/blob/86761482/src/changes/changes.xml ---------------------------------------------------------------------- diff --git a/src/changes/changes.xml b/src/changes/changes.xml index d0255c6..13227ee 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -50,6 +50,9 @@ The <action> type attribute can be add,update,fix,remove. <action dev="ggregory" issue="POOL-339" type="update"> Update optional library cglib from 3.2.5 to 3.2.6. </action> + <action dev="markt" issue="POOL-337" type="fix" due-to="Reinald Verheij"> + Ensure cancelled eviction tasks are removed from scheduler. + </action> </release> <release version="2.5.0" date="2017-12-16" description="This is a minor release, updating to Java 7."> <action dev="ggregory" issue="POOL-331" type="update"> http://git-wip-us.apache.org/repos/asf/commons-pool/blob/86761482/src/main/java/org/apache/commons/pool2/impl/BaseGenericObjectPool.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/commons/pool2/impl/BaseGenericObjectPool.java b/src/main/java/org/apache/commons/pool2/impl/BaseGenericObjectPool.java index d9a4df7..bd3e80b 100644 --- a/src/main/java/org/apache/commons/pool2/impl/BaseGenericObjectPool.java +++ b/src/main/java/org/apache/commons/pool2/impl/BaseGenericObjectPool.java @@ -26,6 +26,7 @@ import java.util.Arrays; import java.util.Deque; import java.util.Iterator; import java.util.TimerTask; +import java.util.concurrent.ScheduledFuture; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicLong; @@ -1026,7 +1027,10 @@ public abstract class BaseGenericObjectPool<T> extends BaseObject { * * @see GenericKeyedObjectPool#setTimeBetweenEvictionRunsMillis */ - class Evictor extends TimerTask { + class Evictor implements Runnable { + + private ScheduledFuture<?> scheduledFuture; + /** * Run pool maintenance. Evict objects qualifying for eviction and then * ensure that the minimum number of idle instances are available. @@ -1074,6 +1078,16 @@ public abstract class BaseGenericObjectPool<T> extends BaseObject { Thread.currentThread().setContextClassLoader(savedClassLoader); } } + + + void setScheduledFuture(ScheduledFuture<?> scheduledFuture) { + this.scheduledFuture = scheduledFuture; + } + + + void cancel() { + scheduledFuture.cancel(false); + } } /** http://git-wip-us.apache.org/repos/asf/commons-pool/blob/86761482/src/main/java/org/apache/commons/pool2/impl/EvictionTimer.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/commons/pool2/impl/EvictionTimer.java b/src/main/java/org/apache/commons/pool2/impl/EvictionTimer.java index 0b5e3ac..bc0a5ff 100644 --- a/src/main/java/org/apache/commons/pool2/impl/EvictionTimer.java +++ b/src/main/java/org/apache/commons/pool2/impl/EvictionTimer.java @@ -19,6 +19,7 @@ package org.apache.commons.pool2.impl; import java.security.AccessController; import java.security.PrivilegedAction; import java.util.TimerTask; +import java.util.concurrent.ScheduledFuture; import java.util.concurrent.ScheduledThreadPoolExecutor; import java.util.concurrent.ThreadFactory; import java.util.concurrent.TimeUnit; @@ -44,9 +45,6 @@ class EvictionTimer { /** Executor instance */ private static ScheduledThreadPoolExecutor executor; //@GuardedBy("EvictionTimer.class") - /** Static usage count tracker */ - private static int usageCount; //@GuardedBy("EvictionTimer.class") - /** Prevent instantiation */ private EvictionTimer() { // Hide the default constructor @@ -73,12 +71,15 @@ class EvictionTimer { * @param delay Delay in milliseconds before task is executed * @param period Time in milliseconds between executions */ - static synchronized void schedule(final Runnable task, final long delay, final long period) { + static synchronized void schedule( + final BaseGenericObjectPool<?>.Evictor task, final long delay, final long period) { if (null == executor) { executor = new ScheduledThreadPoolExecutor(1, new EvictorThreadFactory()); + executor.setRemoveOnCancelPolicy(true); } - usageCount++; - executor.scheduleWithFixedDelay(task, delay, period, TimeUnit.MILLISECONDS); + ScheduledFuture<?> scheduledFuture = + executor.scheduleWithFixedDelay(task, delay, period, TimeUnit.MILLISECONDS); + task.setScheduledFuture(scheduledFuture); } /** @@ -90,10 +91,10 @@ class EvictionTimer { * terminate? * @param unit The units for the specified timeout */ - static synchronized void cancel(final TimerTask task, final long timeout, final TimeUnit unit) { + static synchronized void cancel( + final BaseGenericObjectPool<?>.Evictor task, final long timeout, final TimeUnit unit) { task.cancel(); - usageCount--; - if (usageCount == 0) { + if (executor.getQueue().size() == 0) { executor.shutdown(); try { executor.awaitTermination(timeout, unit); http://git-wip-us.apache.org/repos/asf/commons-pool/blob/86761482/src/test/java/org/apache/commons/pool2/impl/TestEvictionTimer.java ---------------------------------------------------------------------- diff --git a/src/test/java/org/apache/commons/pool2/impl/TestEvictionTimer.java b/src/test/java/org/apache/commons/pool2/impl/TestEvictionTimer.java new file mode 100644 index 0000000..97d8bdd --- /dev/null +++ b/src/test/java/org/apache/commons/pool2/impl/TestEvictionTimer.java @@ -0,0 +1,105 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.commons.pool2.impl; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; +import java.lang.reflect.Field; +import java.util.concurrent.ScheduledFuture; +import java.util.concurrent.ThreadPoolExecutor; +import java.util.concurrent.TimeUnit; + +import org.apache.commons.pool2.BasePooledObjectFactory; +import org.apache.commons.pool2.PooledObject; +import org.junit.Test; + +/** + * Tests for {@link EvictionTimer}. + */ +public class TestEvictionTimer { + + @Test + public void testStartStopEvictionTimer() throws Exception { + + try (final GenericObjectPool<String> pool = new GenericObjectPool<>(new BasePooledObjectFactory<String>() { + + @Override + public String create() throws Exception { + return null; + } + + @Override + public PooledObject<String> wrap(final String obj) { + return new DefaultPooledObject<>(obj); + } + })) { + + // Start evictor #1 + final BaseGenericObjectPool<String>.Evictor evictor1 = pool.new Evictor(); + EvictionTimer.schedule(evictor1, 60000, 60000); + + // Assert that eviction objects are correctly allocated + // 1 - the evictor timer task is created + final Field evictorTaskFutureField = + evictor1.getClass().getDeclaredField("scheduledFuture"); + evictorTaskFutureField.setAccessible(true); + ScheduledFuture<?> sf = (ScheduledFuture<?>) evictorTaskFutureField.get(evictor1); + assertFalse(sf.isCancelled()); + // 2- and, the eviction action is added to executor thread pool + final Field evictorExecutorField = EvictionTimer.class.getDeclaredField("executor"); + evictorExecutorField.setAccessible(true); + final ThreadPoolExecutor evictionExecutor = (ThreadPoolExecutor) evictorExecutorField.get(null); + assertEquals(1, evictionExecutor.getQueue().size()); + + // Start evictor #2 + final BaseGenericObjectPool<String>.Evictor evictor2 = pool.new Evictor(); + EvictionTimer.schedule(evictor2, 60000, 60000); + + // Assert that eviction objects are correctly allocated + // 1 - the evictor timer task is created + sf = (ScheduledFuture<?>) evictorTaskFutureField.get(evictor2); + assertFalse(sf.isCancelled()); + // 2- and, the eviction action is added to executor thread pool + assertEquals(2, evictionExecutor.getQueue().size()); + + // Stop evictor #1 + EvictionTimer.cancel(evictor1, BaseObjectPoolConfig.DEFAULT_EVICTOR_SHUTDOWN_TIMEOUT_MILLIS, + TimeUnit.MILLISECONDS); + + // Assert that eviction objects are correctly cleaned + // 1 - the evictor timer task is cancelled + sf = (ScheduledFuture<?>) evictorTaskFutureField.get(evictor1); + assertTrue(sf.isCancelled()); + // 2- and, the eviction action is removed from executor thread pool + final ThreadPoolExecutor evictionExecutorOnStop = (ThreadPoolExecutor) evictorExecutorField.get(null); + assertEquals(1, evictionExecutorOnStop.getQueue().size()); + + // Stop evictor #2 + EvictionTimer.cancel(evictor2, BaseObjectPoolConfig.DEFAULT_EVICTOR_SHUTDOWN_TIMEOUT_MILLIS, + TimeUnit.MILLISECONDS); + + // Assert that eviction objects are correctly cleaned + // 1 - the evictor timer task is cancelled + sf = (ScheduledFuture<?>) evictorTaskFutureField.get(evictor2); + assertTrue(sf.isCancelled()); + // 2- and, the eviction thread pool executor is freed + assertNull(evictorExecutorField.get(null)); + } + } +} \ No newline at end of file
