Details

    • Bug
    • Status: Closed
    • Major
    • Resolution: Fixed
    • 1.3
    • 1.4
    • None

    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.

      Attachments

        1. timer.patch
          3 kB
          Phil Steitz
        2. pool-97-markt-v2.patch
          6 kB
          Mark Thomas

        Activity

          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...

          henning Henning Schmiedehausen added a comment - 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...
          psteitz Phil Steitz added a comment -

          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?

          psteitz Phil Steitz added a comment - 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?

          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.

          robertburrelldonkin Robert Burrell Donkin added a comment - 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.
          psteitz Phil Steitz added a comment -

          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.

          psteitz Phil Steitz added a comment - 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.
          bayard Henri Yandell added a comment -

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

          bayard Henri Yandell added a comment - I don't see the package private as an issue.
          markt Mark Thomas added a comment -

          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.

          markt Mark Thomas added a comment - 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.
          psteitz Phil Steitz added a comment -

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

          psteitz Phil Steitz added a comment - A fix that would not re-open POOL-56 would be ideal here and I would not worry about the package private compat. issue.
          markt Mark Thomas added a comment -

          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.

          markt Mark Thomas added a comment - 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.
          markt Mark Thomas added a comment -

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

          markt Mark Thomas added a comment - Updated patch. I missed a call to _evictor.cancel() in GKOP
          psteitz Phil Steitz added a comment -

          Patch (v2) applied in r602765. Thanks!

          psteitz Phil Steitz added a comment - Patch (v2) applied in r602765. Thanks!

          People

            Unassigned Unassigned
            devendra_patil Devendra Patil
            Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved: