[
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, 6: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.
# The Reaper has to acquire the EvictionTimer class lock (like schedule and
cancel). I don't see contention for this lock as likely to cause problems
because schedule and cancel calls should in general only happen when starting /
stopping pools that run evictors, but in theory contention for this lock could
cause the Reaper to hold up the other tasks. This is another reason to
consider adding another thread to the executor pool or even running the Reaper
under a separate executor.
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: 50m
> 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)
