[
https://issues.apache.org/jira/browse/POOL-386?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17162100#comment-17162100
]
Phil Steitz edited comment on POOL-386 at 7/21/20, 3:04 PM:
------------------------------------------------------------
A couple things to note / possibly improve in the patch:
# Usage tracking is accomplished by maintaining a hashmap whose keys are weak
references to eviction tasks belonging to client pools. A Reaper task has been
added to the executor that iterates the map and removes references (and tasks)
to tasks that have been gc-ed. When there are no live tasks left, it shuts
down the executor. The Reaper is created when a new executor is launched,
which is triggered by a schedule
call. The Reaper inherits the run frequency of the task being scheduled when
it is created. Not sure if this is the best, but seems reasonable.
# The executor thread pool size is not increased by the patch. It may make
sense to bump this to 2 or to make it configurable. AFAICT, concurrent
activation of evictor tasks should be OK, as they are guarded by the eviction
lock in BaseGenericObjectPool.
was (Author: psteitz):
A couple things to note / possibly improve in the patch:
# Usage tracking is accomplished by maintaining a hashmap whose keys are weak
references to eviction tasks belonging to client pools. A Reaper task has been
added to the executor that iterates the map and removes references (and tasks)
to tasks that have been gc-ed. When there are no live tasks left, it shuts
down the executor. The Reaper is created when a new executor is launched,
which is triggered by a ``schedule`` call. The Reaper inherits the run
frequency of the task being scheduled when it is created. Not sure if this is
the best, but seems reasonable.
# The executor thread pool size is not increased by the patch. It may make
sense to bump this to 2 or to make it configurable. AFAICT, concurrent
activation of evictor tasks should be OK, as they are guarded by the eviction
lock in BaseGenericObjectPool.
> Closing a pool can cause Evictor in another pool to be cancelled
> ----------------------------------------------------------------
>
> Key: POOL-386
> URL: https://issues.apache.org/jira/browse/POOL-386
> Project: Commons Pool
> Issue Type: Task
> Affects Versions: 2.6.1, 2.6.2, 2.7.0, 2.8.0
> Reporter: Phil Steitz
> Priority: Major
> Fix For: 2.8.1
>
> Time Spent: 40m
> Remaining Estimate: 0h
>
> The fix for POOL-337 introduced a race condition that can cause the shared
> EvictionTimer to be cancelled when a pool is closed but another pool still
> has an active Evictor. The EvictionTimer cancel method used to cancel an
> eviction task owned by a client pool uses this test to determine whether or
> not to shutdown its executor:
> {code:java}
> if (executor != null && executor.getQueue().isEmpty()){code}
> The executor may report an empty queue if it is executing a task. This will
> cause the executor to be shut down and scheduling of new tasks to stop.
> The unit test below illustrates the problem.
> {code:java}
>
> public void testEvictionTimerMultiplePools() throws InterruptedException {
> final AtomicIntegerFactory factory = new AtomicIntegerFactory();
> factory.setValidateLatency(50);
> final GenericObjectPool<AtomicInteger> evictingPool = new
> GenericObjectPool<>(factory);
> evictingPool.setTimeBetweenEvictionRunsMillis(100);
> evictingPool.setNumTestsPerEvictionRun(5);
> evictingPool.setTestWhileIdle(true);
> evictingPool.setMinEvictableIdleTimeMillis(50);
> for (int i = 0; i < 10; i++) {
> try {
> evictingPool.addObject();
> } catch (Exception e) {
> e.printStackTrace();
> }
> }
> for (int i = 0; i < 1000; i++) {
> GenericObjectPool<AtomicInteger> nonEvictingPool = new
> GenericObjectPool<>(factory);
> nonEvictingPool.close();
> }
> Thread.sleep(1000);
> Assert.assertEquals(0, evictingPool.getNumIdle());
> evictingPool.close();
> }
> {code}
> Proposed fix:
> Change the test guarding executor shutdown to
> {code:java}
> executor.getQueue().isEmpty() && executor.getActiveCount() == 0
> {code}
> This still exposes a low-probability race - a task completes after the
> isEmpty test and is requeued before the activecount test - but this is very
> unlikely.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
