groovy
  1. groovy
  2. GROOVY-1194

Memory leak in MetaClassRegistry

    Details

    • Type: Bug Bug
    • Status: Closed Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.0-JSR-4
    • Fix Version/s: 1.0-RC-1
    • Component/s: None
    • Labels:
      None
    • Environment:
      Sun JDK 1.4.2_08, Windows
    • Testcase included:
      yes
    • Number of attachments :
      2

      Description

      Hi,

      After having some time spent hunting a memory leak in our application, I've managed to narrow the problem to Groovy. Basically in our case we use a throw-away classloader that is then used to load a Groovy-generated class (to be more specific, we are using JasperReports with Groovy expressions).

      The issue as I see it is linked to GROOVY-375 which is where the use of WeakHashMap was introduced. The intention was correct as it was meant to allow garbage collector to release cached info that cannot longer be used (the info for throw-away classes in our case). However there is a restriction, described in the javadoc of WeakHashMap, that must be respected for this to work: the values stored in the map must not have direct or indirect strong references to their keys. This is not the case for the metaClasses map that maps Class instances to MetaClass instances because the MetaClass holds a direct reference (theClass) back to Class.

      Basically this is the same issue as with java.beans.Introspector class which also stores values with strong references to their keys (ie: Class->Method[] in declaredMethodCache field where Method has a strong reference to its class). Introspector class provides flushCaches methods as a fix for this problem.

      To demonstrate the problem I've created a junit test that can help to reproduce the problem and to test possible solutions.

      One possible fix for this problem is to use the same approach as the Introspector class and to provide a flush method that could be called explicitly by the user. Personally I don't like this solution as it delegates the responsibility to the user of the library which is bug-prone. Another solution would be to replace the strong references with WeakReferences where required. The attached test can help to test this solution.

      Peter.

      1. GroovyMemoryLeakTest.java
        5 kB
        Peter Severin
      2. MetaClassRegistry.patch
        5 kB
        Peter Severin

        Activity

        Hide
        Peter Severin added a comment -

        I've created a patch for the HEAD version of the MetaClassRegistry.java file. I've used a WeakReference to wrap the values that are stored in the WeakHashMap. There was a problem with default MetaClass instances that are initialized in the constructor:
        // lets register the default methods
        lookup(DefaultGroovyMethods.class);
        registerMethods(DefaultGroovyMethods.class, true);
        lookup(DefaultGroovyStaticMethods.class);
        registerMethods(DefaultGroovyStaticMethods.class, false);

        Apparently those should be always present in the HashMap overwise the unit tests do not pass (there is an error that says the String.plus() method cannot be found). To fix this I've mixed strong references with WeakReferences so that the default MetaClass instances are never garbage collected. There is a flag called "initializing" that is set to true during the MetaClassRegistry construction and that determines the references to be stored directly without wrapping them with WeakReference. Please see the patch for more details. With this fix all unit tests pass.

        Show
        Peter Severin added a comment - I've created a patch for the HEAD version of the MetaClassRegistry.java file. I've used a WeakReference to wrap the values that are stored in the WeakHashMap. There was a problem with default MetaClass instances that are initialized in the constructor: // lets register the default methods lookup(DefaultGroovyMethods.class); registerMethods(DefaultGroovyMethods.class, true); lookup(DefaultGroovyStaticMethods.class); registerMethods(DefaultGroovyStaticMethods.class, false); Apparently those should be always present in the HashMap overwise the unit tests do not pass (there is an error that says the String.plus() method cannot be found). To fix this I've mixed strong references with WeakReferences so that the default MetaClass instances are never garbage collected. There is a flag called "initializing" that is set to true during the MetaClassRegistry construction and that determines the references to be stored directly without wrapping them with WeakReference. Please see the patch for more details. With this fix all unit tests pass.
        Hide
        John Wilson added a comment -

        pacth did not fix the problem

        Show
        John Wilson added a comment - pacth did not fix the problem
        Hide
        Peter Severin added a comment -

        John, can you provide more details? It has been some time since the bug was created so perhaps the patch needs to be updated? We are still using groovy beta10 at our place with the original patch and it works for us.

        Show
        Peter Severin added a comment - John, can you provide more details? It has been some time since the bug was created so perhaps the patch needs to be updated? We are still using groovy beta10 at our place with the original patch and it works for us.
        Hide
        blackdrag blackdrag added a comment -

        fixed

        Show
        blackdrag blackdrag added a comment - fixed

          People

          • Assignee:
            blackdrag blackdrag
            Reporter:
            Peter Severin
          • Votes:
            5 Vote for this issue
            Watchers:
            8 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: