Phil Steitz commented on POOL-337:

Sorry, I missed the unit test in the updated PR.  What I am still not getting 
is how the stack trace at the top can happen.  I can see how the memory leak is 
a problem and I think there is definitely a bug here, but I would like to be 
able to reproduce this directly from the GOP API, which I can't do.  The 
underlying Evictor task should get cancelled while the eviction lock is held, 
but somehow the executor is still firing it in the trace at the top.

> EvictionTimer does not remove cancelled tasks from the executor, leading to 
> an IllegalStateException when the evictor attempts to evict
> ---------------------------------------------------------------------------------------------------------------------------------------
>                 Key: POOL-337
>                 URL: https://issues.apache.org/jira/browse/POOL-337
>             Project: Commons Pool
>          Issue Type: Bug
>    Affects Versions: 2.5.0
>            Reporter: Reinald Verheij
>            Priority: Major
>         Attachments: EvictionTimer.java, 
> EvictionTimer.java.original-from-2.5.0.java
> EvictionTimer does not remove cancelled tasks from the executor, leading to 
> an IllegalStateException when the evictor attempts to evict.
> EvictionTimer::schedule() adds eviction tasks to the executor, but the cancel 
> does not remove it. This is asymmetric and leads to the following exception:
> {noformat}
> java.lang.IllegalStateException: Pool not open
>       at 
> org.apache.commons.pool2.impl.BaseGenericObjectPool.assertOpen(BaseGenericObjectPool.java:713)
>       at 
> org.apache.commons.pool2.impl.GenericObjectPool.evict(GenericObjectPool.java:721)
>       at 
> org.apache.commons.pool2.impl.BaseGenericObjectPool$Evictor.run(BaseGenericObjectPool.java:1077)
>       at 
> java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
>       at java.util.concurrent.FutureTask.runAndReset(FutureTask.java:308)
>       at 
> java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.access$301(ScheduledThreadPoolExecutor.java:180)
>       at 
> java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:294)
>       at 
> java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
>       at 
> java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
>       at java.lang.Thread.run(Thread.java:748){noformat}
> I think the cancel would need to remember the future which returned from 
> {{executor::scheduleWithFixedDelay()}} in {{schedule()}} and then do 
> something like this (see  [^EvictionTimer.java] compared to original  
> [^EvictionTimer.java.original-from-2.5.0.java] )
> {code:java}
> if (futures.containsKey(task)) {
>     futures.remove(task).cancel(false);
>     executor.purge();
> }{code}

This message was sent by Atlassian JIRA

Reply via email to