Commons Logging
  1. Commons Logging
  2. LOGGING-51

[logging] Memory leaks in JBoss due to LogFactory cache

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.0.4
    • Fix Version/s: 1.1.0
    • Labels:
      None
    • Environment:

      Operating System: All
      Platform: All

      Description

      LogFactory.java maintains a static cache (Hashtable) of LogFactories, keyed by
      context ClassLoader. In JBoss, and may be in many other J2EE containers, each
      hot-redeployment of a J2EE application makes a new class loader for the
      application, destroying all references to the old one. However, commons-logging.
      jar is loaded by a parent classloader which is common for all applications, and
      still maintains hard references to would-be-dead ClassLoaders of undeployed
      applications. This leads to significant memory leaks, because all static members
      of all classes of the undeployed applications do not become a subject for GC. It
      would be excellent if this LogFactory caching could be disabled through a config
      or a WeakHashMap is used instead of Hashtable.

      1. ASF.LICENSE.NOT.GRANTED--WeakHashtable.java
        21 kB
        Brian Stansberry
      2. ASF.LICENSE.NOT.GRANTED--LogFactory.diff
        2 kB
        Brian Stansberry
      3. ASF.LICENSE.NOT.GRANTED--LogFactoryTest.diff
        15 kB
        Brian Stansberry
      4. ASF.LICENSE.NOT.GRANTED--WeakHashtable_javadoc.txt
        3 kB
        Brian Stansberry
      5. ASF.LICENSE.NOT.GRANTED--patch.txt
        9 kB
        Brian Stansberry
      6. ASF.LICENSE.NOT.GRANTED--JCL-guide.diff
        9 kB
        Brian Stansberry
      7. ASF.LICENSE.NOT.GRANTED--memoryleaktest.zip
        5 kB
        Brian Stansberry

        Activity

        Hide
        Simon Kitching added a comment -

        I don't think we're going to get any better than the current code on this issue.
        Brian's WeakHashtable fixes some cases transparently which is nice.

        A custom ServletContextListener class that calls release() can definitely solve
        the problem in all cases, but requires some additional work on the user's side.
        This class is currently in the release, but may end up just as documentation
        instead; issue still being debated.

        I'm therefore closing this.

        Show
        Simon Kitching added a comment - I don't think we're going to get any better than the current code on this issue. Brian's WeakHashtable fixes some cases transparently which is nice. A custom ServletContextListener class that calls release() can definitely solve the problem in all cases, but requires some additional work on the user's side. This class is currently in the release, but may end up just as documentation instead; issue still being debated. I'm therefore closing this.
        Hide
        David D. Kilzer added a comment -

        FYI, here's the corresponding JBoss bug. They've proposed a similar fix and
        (per their comments) may roll their own commons-logging-1.0.3 to fix the problem.

        http://jira.jboss.com/jira/browse/JBAS-1319

        Show
        David D. Kilzer added a comment - FYI, here's the corresponding JBoss bug. They've proposed a similar fix and (per their comments) may roll their own commons-logging-1.0.3 to fix the problem. http://jira.jboss.com/jira/browse/JBAS-1319
        Hide
        Brian Stansberry added a comment -

        Created an attachment (id=14501)
        Further tests for memory leak

        The patch includes a new class – a special classloader that may also be useful
        in tests of the JCL discovery process.

        Show
        Brian Stansberry added a comment - Created an attachment (id=14501) Further tests for memory leak The patch includes a new class – a special classloader that may also be useful in tests of the JCL discovery process.
        Hide
        Brian Stansberry added a comment -

        As discussed on the commons-dev list, even using WeakHashtable the classloader
        is not released when LogFactoryImpl is loaded by a parent loader and the
        Log wrapper is loaded by a child. This is because the LogFactoryImpl holds a
        reference to the Log in its instances map, and that reference prevents the Log's
        classloader being released.

        Will attach a patch to the JUnit tests that show the problem. Sadly, no fix is
        in the patch

        Show
        Brian Stansberry added a comment - As discussed on the commons-dev list, even using WeakHashtable the classloader is not released when LogFactoryImpl is loaded by a parent loader and the Log wrapper is loaded by a child. This is because the LogFactoryImpl holds a reference to the Log in its instances map, and that reference prevents the Log's classloader being released. Will attach a patch to the JUnit tests that show the problem. Sadly, no fix is in the patch
        Hide
        rdonkin@apache.org added a comment -

        Committed. Many thanks.

        I'm closing this one now (but please feel free to open new reports for any new
        improvements).

        Robert

        Show
        rdonkin@apache.org added a comment - Committed. Many thanks. I'm closing this one now (but please feel free to open new reports for any new improvements). Robert
        Hide
        Brian Stansberry added a comment -

        Created an attachment (id=14148)
        Update to the User Guide

        Attached is a patch to guide.xml that includes discussion of the issues in this
        bug and the fix. Also includes discussion of what the various jars in the
        standard distribution are, in order to give some context to the above.

        Show
        Brian Stansberry added a comment - Created an attachment (id=14148) Update to the User Guide Attached is a patch to guide.xml that includes discussion of the issues in this bug and the fix. Also includes discussion of what the various jars in the standard distribution are, in order to give some context to the above.
        Hide
        rdonkin@apache.org added a comment -

        Hi Brian

        Apologies for my tardiness (caught the traditional British Christmas Cold). Good patch, committed.

        I suspect that we're approaching the end of the line for this bug now. I still need to pull together some
        documentation and integrate it with the existing stuff. So I'll leave this open as a reminder.

        You might like to resubscribe to commons-dev (perhaps through www.gmane.org if the volume is
        offputting) since a lot of discussions have kicked off (involved IBM amongst others) about the future of
        commons logging and enterprise logging in general. I'm sure you're input would be welcomed.

        Robert

        Show
        rdonkin@apache.org added a comment - Hi Brian Apologies for my tardiness (caught the traditional British Christmas Cold). Good patch, committed. I suspect that we're approaching the end of the line for this bug now. I still need to pull together some documentation and integrate it with the existing stuff. So I'll leave this open as a reminder. You might like to resubscribe to commons-dev (perhaps through www.gmane.org if the volume is offputting) since a lot of discussions have kicked off (involved IBM amongst others) about the future of commons logging and enterprise logging in general. I'm sure you're input would be welcomed. Robert
        Hide
        Brian Stansberry added a comment -

        Created an attachment (id=13646)
        WeakHashtable uses strong refs to values

        OK, got a chance to change WeakHashtable to simple strong references to its
        values. Patch attached.

        The patch also follows up on a thought we'd mentioned on the dev list. Every
        10 puts it checks the ReferenceQueue and, if needed, purges one dead entry.
        Housekeeping in small doses I also added this behavior to remove().

        Also changed a couple local variable names Eclipse warned about for JDK 1.5

        Cheers,

        Brian

        Show
        Brian Stansberry added a comment - Created an attachment (id=13646) WeakHashtable uses strong refs to values OK, got a chance to change WeakHashtable to simple strong references to its values. Patch attached. The patch also follows up on a thought we'd mentioned on the dev list. Every 10 puts it checks the ReferenceQueue and, if needed, purges one dead entry. Housekeeping in small doses I also added this behavior to remove(). Also changed a couple local variable names Eclipse warned about for JDK 1.5 Cheers, Brian
        Hide
        rdonkin@apache.org added a comment -

        Another good patch. Committed. Many thanks.

        I think that it should be possible to simplify the code in the way you suggest (by using simple strong
        references) without effecting the function.

        My next priority will be to work on the documentation build and the user guide. Then push towards a
        1.0.5 release. I'll leave this bug open until that's done in case you find time.

        Robert

        Show
        rdonkin@apache.org added a comment - Another good patch. Committed. Many thanks. I think that it should be possible to simplify the code in the way you suggest (by using simple strong references) without effecting the function. My next priority will be to work on the documentation build and the user guide. Then push towards a 1.0.5 release. I'll leave this bug open until that's done in case you find time. Robert
        Hide
        Brian Stansberry added a comment -

        Created an attachment (id=13519)
        Javadoc for WeakHashtable

        Attached adds some class level Javadoc to WeakHashtable that tries to explain
        the classloading situation where a call to release() is still needed.

        There's some other stuff that I'd like to do on this (perhaps use simple strong
        references for values in WeakHashtable; refactor LogFactoryTest and LoadTest to
        avoid code duplication), but they don't affect the functionality, and it will
        probably be a while before a can get to them.

        Show
        Brian Stansberry added a comment - Created an attachment (id=13519) Javadoc for WeakHashtable Attached adds some class level Javadoc to WeakHashtable that tries to explain the classloading situation where a call to release() is still needed. There's some other stuff that I'd like to do on this (perhaps use simple strong references for values in WeakHashtable; refactor LogFactoryTest and LoadTest to avoid code duplication), but they don't affect the functionality, and it will probably be a while before a can get to them.
        Hide
        rdonkin@apache.org added a comment -

        Nice work. Committed. Many Thanks.

        I'm happy that this bug can be closed but I will leave it open for a while yet just in case Brian (or anyone
        else) wants to attach improvements.

        Robert

        Show
        rdonkin@apache.org added a comment - Nice work. Committed. Many Thanks. I'm happy that this bug can be closed but I will leave it open for a while yet just in case Brian (or anyone else) wants to attach improvements. Robert
        Hide
        Brian Stansberry added a comment -

        Created an attachment (id=13498)
        Patch to LogFactoryTest

        Show
        Brian Stansberry added a comment - Created an attachment (id=13498) Patch to LogFactoryTest
        Hide
        Brian Stansberry added a comment -

        Attaching some improvements to the LogFactoryTest. Designed to test different
        classloading configurations, particularly an isolated application classloader
        inside a container.

        Show
        Brian Stansberry added a comment - Attaching some improvements to the LogFactoryTest. Designed to test different classloading configurations, particularly an isolated application classloader inside a container.
        Hide
        Simon Kitching added a comment -

        Just to clarify: When Robert said "if you ever decide you've done enough and
        would like your name added, submit a patch" I believe he means provide a patch
        to the project.xml file which contains the author information

        Show
        Simon Kitching added a comment - Just to clarify: When Robert said "if you ever decide you've done enough and would like your name added, submit a patch" I believe he means provide a patch to the project.xml file which contains the author information
        Hide
        rdonkin@apache.org added a comment -

        Contributions take different forms. I commonly rewrite or reimplement patches before applying them.
        (One of the signs I look for when nominating a new committer is that they start producing patches that
        I can apply without modification.) Without your contribution, this code wouldn't have been written. (I'm
        quick to credit but slow to nominate.)

        Just FYI I never add credit unilaterally (except in commit messages where it provides a record) so if you
        ever decide that you've done enough and would like your name added, submit a patch.

        Show
        rdonkin@apache.org added a comment - Contributions take different forms. I commonly rewrite or reimplement patches before applying them. (One of the signs I look for when nominating a new committer is that they start producing patches that I can apply without modification.) Without your contribution, this code wouldn't have been written. (I'm quick to credit but slow to nominate.) Just FYI I never add credit unilaterally (except in commit messages where it provides a record) so if you ever decide that you've done enough and would like your name added, submit a patch.
        Hide
        Brian Stansberry added a comment -

        I'll look this over as carefully as I can and try to add a few tests. I've
        got a couple thoughts, but I'll post to the commons-dev list to keep from
        filling up Bugzilla.

        Re adding my name, at this point you've done all the work If I'm able to
        make any further significant contributions to this, please feel free to add my
        name.

        Show
        Brian Stansberry added a comment - I'll look this over as carefully as I can and try to add a few tests. I've got a couple thoughts, but I'll post to the commons-dev list to keep from filling up Bugzilla. Re adding my name, at this point you've done all the work If I'm able to make any further significant contributions to this, please feel free to add my name.
        Hide
        rdonkin@apache.org added a comment -

        I've checked in my changes. I'm going to leave the documentation for another day.

        The idea is that the Hashtable implementation can be specified by a system property and (when the
        class is found on the classpath) defaults to the optional WeakHashtable. The idea is that a user (on a
        1.3+ JVM) should be able to drop in the commons-logging-optional.jar and have the classloaders
        garbaged collectable without further changes.

        This is going to need some testing. I've added a few tests (if anyone wants to add more, please go
        ahead I'll probably post something on the lists tomorrow.

        BTW Brian, I'm happy to add your name to the contributors and/or author tags if that is your wish.

        Show
        rdonkin@apache.org added a comment - I've checked in my changes. I'm going to leave the documentation for another day. The idea is that the Hashtable implementation can be specified by a system property and (when the class is found on the classpath) defaults to the optional WeakHashtable. The idea is that a user (on a 1.3+ JVM) should be able to drop in the commons-logging-optional.jar and have the classloaders garbaged collectable without further changes. This is going to need some testing. I've added a few tests (if anyone wants to add more, please go ahead I'll probably post something on the lists tomorrow. BTW Brian, I'm happy to add your name to the contributors and/or author tags if that is your wish.
        Hide
        rdonkin@apache.org added a comment -

        I agree with the sentiments but hopefully this solution is so simple that no discovery is needed

        One of the aims of the bytecode stuff I looking at is allow replacement of LogFactory with something
        that implements a minimal API. This might allow a commons-discovery based discovery mechanism to
        be added (by doping) without having to introduce a core dependency.

        Show
        rdonkin@apache.org added a comment - I agree with the sentiments but hopefully this solution is so simple that no discovery is needed One of the aims of the bytecode stuff I looking at is allow replacement of LogFactory with something that implements a minimal API. This might allow a commons-discovery based discovery mechanism to be added (by doping) without having to introduce a core dependency.
        Hide
        Richard A. Sitze added a comment -

        The world is watching.. and waiting.

        Good to hear this is being address, in one way or another.

        Just FYI, commons-discovery attempts [impl. incomplete]
        to isolate JVM dependencies to a *.jdk package with
        a base interface JDKHooks and separate classes for
        JDK11Hooks, JDK12Hooks.

        We may not be able to support something in one version
        versus another, but at least this way we can document
        and isolate the dependencies. Even if it's as simple
        as a getWeakHashTable() function on one such.

        Show
        Richard A. Sitze added a comment - The world is watching.. and waiting. Good to hear this is being address, in one way or another. Just FYI, commons-discovery attempts [impl. incomplete] to isolate JVM dependencies to a *.jdk package with a base interface JDKHooks and separate classes for JDK11Hooks, JDK12Hooks. We may not be able to support something in one version versus another, but at least this way we can document and isolate the dependencies. Even if it's as simple as a getWeakHashTable() function on one such.
        Hide
        Brian Stansberry added a comment -

        Sure, no problem at all. I'll keep an eye out.

        Show
        Brian Stansberry added a comment - Sure, no problem at all. I'll keep an eye out.
        Hide
        rdonkin@apache.org added a comment -

        I'm on GMT so the offer's come a little later (it's almost done) but the offer's really appreciated

        This kind of stuff benefits from many eyes so what would be really useful is if you could take a look at
        the code (once I've committed it, probably tomorrow now) and run any test cases you have against it.

        Show
        rdonkin@apache.org added a comment - I'm on GMT so the offer's come a little later (it's almost done) but the offer's really appreciated This kind of stuff benefits from many eyes so what would be really useful is if you could take a look at the code (once I've committed it, probably tomorrow now) and run any test cases you have against it.
        Hide
        Brian Stansberry added a comment -

        Yes, I think I made a likely unnecessary assumption that commons-logging has to
        compile under JDK 1.1, not just run. So I used reflection throughout. If
        compiling is not an issue, I would be happy to tweak WeakHashtable to get rid of
        all the internal use of reflection (and add the system property you suggested as
        well).

        Show
        Brian Stansberry added a comment - Yes, I think I made a likely unnecessary assumption that commons-logging has to compile under JDK 1.1, not just run. So I used reflection throughout. If compiling is not an issue, I would be happy to tweak WeakHashtable to get rid of all the internal use of reflection (and add the system property you suggested as well).
        Hide
        rdonkin@apache.org added a comment -

        Maybe something alongs these lines might work.

        There are major performance issues with using a reflection to hook up the methods but it might be
        possible to create a Hashtable implementation that uses a WeakHashMap internally and then try to load
        the class by name when the LogFactory class is initialized. If the class can't be loaded then a normal
        Hashtable would be used.

        Probably be a good idea to allow the class name to be specified in a system property.

        Show
        rdonkin@apache.org added a comment - Maybe something alongs these lines might work. There are major performance issues with using a reflection to hook up the methods but it might be possible to create a Hashtable implementation that uses a WeakHashMap internally and then try to load the class by name when the LogFactory class is initialized. If the class can't be loaded then a normal Hashtable would be used. Probably be a good idea to allow the class name to be specified in a system property.
        Hide
        Brian Stansberry added a comment -

        Created an attachment (id=13361)
        LogFactory.diff

        Show
        Brian Stansberry added a comment - Created an attachment (id=13361) LogFactory.diff
        Hide
        Brian Stansberry added a comment -

        Created an attachment (id=13360)
        WeakHashtable.java

        Show
        Brian Stansberry added a comment - Created an attachment (id=13360) WeakHashtable.java
        Hide
        Brian Stansberry added a comment -

        I wrote something a couple weeks back that sounds like it might do something
        similar to what you were thinking about, but using reflection instead of
        bytecode manipulation. Basically, it detects if 1.2 is available and if so
        LogFactory uses a special Hashtable subclass that passes (almost) all calls
        through to a WeakHashMap. Almost, because to allow use in 1.1 JVMs the
        methods that include Collections framework classes in their signatures are not
        overridden. So, as you noted, this approach may cause issues with subclasses.

        I hesistated to submit this as it seems a bit of a hack, but, hey, why not

        Attached is a diff to LogFactory and a new class, WeakHashtable.

        Show
        Brian Stansberry added a comment - I wrote something a couple weeks back that sounds like it might do something similar to what you were thinking about, but using reflection instead of bytecode manipulation. Basically, it detects if 1.2 is available and if so LogFactory uses a special Hashtable subclass that passes (almost) all calls through to a WeakHashMap. Almost, because to allow use in 1.1 JVMs the methods that include Collections framework classes in their signatures are not overridden. So, as you noted, this approach may cause issues with subclasses. I hesistated to submit this as it seems a bit of a hack, but, hey, why not Attached is a diff to LogFactory and a new class, WeakHashtable.
        Hide
        rdonkin@apache.org added a comment -

        Had another look at this today and again it looks like the downsides of altering the code outweight the
        upside.

        I've been moving towards ideas about using bytecode manipulation to dope commons logging jars. The
        idea would be that though one-size-fits implementations are really not possible, it should be possible
        to use make small changes by altering the bytecode so that (for example) uses of the hashtable are
        replaced by calls to a WeakHashMap. The doped jar would no longer work correctly with certain
        subclasses or pre-Java2 JVMs but would release classloaders.

        Show
        rdonkin@apache.org added a comment - Had another look at this today and again it looks like the downsides of altering the code outweight the upside. I've been moving towards ideas about using bytecode manipulation to dope commons logging jars. The idea would be that though one-size-fits implementations are really not possible, it should be possible to use make small changes by altering the bytecode so that (for example) uses of the hashtable are replaced by calls to a WeakHashMap. The doped jar would no longer work correctly with certain subclasses or pre-Java2 JVMs but would release classloaders.
        Hide
        rdonkin@apache.org added a comment -

        Yep. I've been aware of this one for quite a while. It's necessary to call release when commons-logging
        is in the parent classloader of a container.

        There are a couple of issue which have prevented it being fixed. The first is that commons-logging
        needs to support JVMs which do not have weak-hash maps. The second is that for some reason the
        cache implementation (hashtable) was made protected rather than private so a direct change could
        mean breaking backward compatibility.

        I can see a few ways round these issues but they aren't not so simple and (to be honest) commons-
        logging really isn't an itch for me at the moment.

        The caching needs to be factored out and then some mechanism added to allow how these methods
        work to vary. A system property might do the trick but I wonder whether it might be possible to use
        aspect orientation. (I've been wondering whether releasing a vanilla implementation and enhancement
        scripts might be the long term solution to a host of thorny problems with logging.)

        Show
        rdonkin@apache.org added a comment - Yep. I've been aware of this one for quite a while. It's necessary to call release when commons-logging is in the parent classloader of a container. There are a couple of issue which have prevented it being fixed. The first is that commons-logging needs to support JVMs which do not have weak-hash maps. The second is that for some reason the cache implementation (hashtable) was made protected rather than private so a direct change could mean breaking backward compatibility. I can see a few ways round these issues but they aren't not so simple and (to be honest) commons- logging really isn't an itch for me at the moment. The caching needs to be factored out and then some mechanism added to allow how these methods work to vary. A system property might do the trick but I wonder whether it might be possible to use aspect orientation. (I've been wondering whether releasing a vanilla implementation and enhancement scripts might be the long term solution to a host of thorny problems with logging.)

          People

          • Assignee:
            Unassigned
            Reporter:
            Alexei Yudichev
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development