EVICTION_TIMER is never cancelled.

• Key: POOL-97
• Type: Bug
• Status: Resolved
• Resolution: Fixed
• Priority: Major
• Assignee: Unassigned
• Reporter: Devendra Patil
• Affects Version/s: 1.3
• Fix Version/s: 1.4

Description

The static EVICTION_TIMER (java.util.Timer) used in GenericObjectPool is never cancelled (even after closing the pool). The GenericObjectPool.close() method just cancels the _evictor (TimerTask). I agree this behaviour is ideal if EVICTION_TIMER is to be used across multiple pools.

But, In my case, the resources (i.e. jars) are dynamically deployed and undeployed on remote grid-servers. If EVICTION_TIMER thread doesn't stop, the grid-servers fails to undeploy (i.e. delete) the jars. The grid-server doesn't restart during resource deployment/undeployment, so, setting EVICTION_TIMER to daemon doesn't help me.

Comments

[Henning Schmiedehausen]

Ah, that would explain some strange behaviour that I have seen in a project using commons-pool inside WebLogic 8.1.

Undeploy/deploy does not work unless I restart the server. Exactly the same phenomenon...

[Phil Steitz]

Attaching a patch (timer.patch) that makes the eviction timer a (lazy initialized) instance variable in GOP, GKOP and cancels it in close. Questions:

1. Will this resolve the issue fully?
2. Other than additional overhead in multi-pool settings, are there other negatives to this? How "bad" is the overhead issue?
3. Does the fix introduce any other problems?

[Robert Burrell Donkin]

This fix breaks binary compatibility but only at package private level.

One question which may need to investigated is whether any subtle concurrency issues are prevent by using a static instance. I've taken a look at the code and i can only see it being used to set up eviction timings. Not sure whether these are ever cleared out. So, the instance will ensure the pool remains in memory until the server is restarted. More eyes would be good since it's very easy to miss subtle concurrency issues.

I suppose that some benchmarks would be the right way to assess the performance impact of this patch.

A more complex solution would be to create a private subclass of timer that does knows when to unschedule eviction tasks. The evictor would need to implement a package interface (unschedulable?) and then test for the pool being closed (or something like that). This would be quite a bit more effort with more to go wrong.

[Phil Steitz]

A more conservative solution would be to revert to the pool 1.2 setup where the Evictor is a thread. This would also break backward compatibility at the package private level, but would revert to a well-tested solution with essentially the same scaling / perfornance challenges as the per instance Timer in the patch. See POOL-56 or 1.2 sources for changes to revert.

[Henri Yandell]

I don't see the package private as an issue.

[Mark Thomas]

I can reproduce this. I have been thinking about a solution that would fix this and POOL-56. I am not quite there yet. I'll post some patches here when I have something concrete. Being able to break binary compatibility at the package private level should make a solution easier.

[Phil Steitz]

A fix that would not re-open POOL-56 would be ideal here and I would not worry about the package private compat. issue.

[Mark Thomas]

This patch fixes POOL-97, keeps the fix for POOL-56 and passes all the unit tests. It does break the package private compatibility but I couldn't see an easy way around that. The patch also removes some unused code and imports in the two pool classes.

I have tested this with the latest Tomcat 5.5.x and the problem described in http://issues.apache.org/bugzilla/show_bug.cgi?id=43552 (which I believe is a symptom of this issue) is also fixed.

My aim was to stick to the commons-pool coding conventions but if this patch is viewed as not doing things in the commons-pool way then I won't be offended if you ask me to re-write it or if it is committed in a completely different form.

[Mark Thomas]

Updated patch. I missed a call to _evictor.cancel() in GKOP

[Phil Steitz]

Patch (v2) applied in r602765. Thanks!