Uploaded image for project: 'Groovy'
  1. Groovy
  2. GROOVY-7683

Memory leak when using Groovy as JSR-223 scripting language.

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.4.5
    • Fix Version/s: 2.4.8
    • Component/s: GroovyScriptEngine
    • Labels:
    • Environment:
      OS: tested on Mac OS X El Capitan and Windows 10.
      JVM: tested on 1.8.0_60 and 1.8.0_65.
    • Flags:
      Important

      Description

      We have a Java EE 7 web application in production that when handling single HTTP request can load and execute up to several Groovy scripts using the jsr-223 API. This application is deployed to GlassFish 4.1 server cluster with 4 instances, each having 8 GB of RAM available (Xmx=8g). We have to restart them every couple of days (3-4), because of leaking memory. After analyzing a couple of heap dumps, our main suspect is Groovy with its MetaMethodIndex$Entry class (the below table shows the top object from one of the heap dumps).

      Class Name Objects Shallow Heap Retained Heap
      MetaMethodIndex$Entry 3 360 001 188 160 056 >= 305 408 024

      To confirm our suspicions, I created simple Maven project with a single test case. The project is available on GitHub. The test case executes 10 different scripts (minimal differences) obtained from a single template 20000 times in 64 worker threads (the main thread is put to sleep for 10 seconds before starting worker threads, so that one can attach JVisualVM to the test process). After all threads are done, System.gc() is called to provoke full GC. Attaching to the process in which tests are run with JVisualVM reveals that the memory is not reclaimed.

      To run the test in your local environment, simply clone the GitHub project and run:

      mvn test
      

      The same test can be run with the -Dlanguage=javascript system option, which switches ScriptEngine from Groovy to Nashorn and uses slightly modified script template (only syntactical differences).

      mvn -Dlanguage=javascript test
      

      Running the test case using built-in Nashorn engine reveals no problems - all allocated memory is reclaimed after full GC.

      I know that the test case is run in Java SE environment, but I guess that it clearly reflects the issue. If it's not enough, I can create an arquillian test case.

      This may be a possible duplicate of GROOVY-7109.

      Any workarounds for this issue would be greatly appreciated.

        Issue Links

          Activity

          Hide
          blackdrag Jochen Theodorou added a comment - - edited

          It could also be related to GROOVY-7591. This can be verified by setting the system property groovy.use.classvalue and see if the memory leak still happens

          Show
          blackdrag Jochen Theodorou added a comment - - edited It could also be related to GROOVY-7591 . This can be verified by setting the system property groovy.use.classvalue and see if the memory leak still happens
          Hide
          jigga Arkadiusz Gasinski added a comment -

          Unfortunately, adding -Dgroovy.use.classvalue=false system property when running the test did not help - the memory was not reclaimed after the full GC. Any other options?

          Show
          jigga Arkadiusz Gasinski added a comment - Unfortunately, adding -Dgroovy.use.classvalue=false system property when running the test did not help - the memory was not reclaimed after the full GC. Any other options?
          Hide
          jigga Arkadiusz Gasinski added a comment -

          The other thing that bothers me is that each time the test is run, there are always 20000 different script classes loaded (Script1 to Script20000), even though there's only 10 variations of the script. Is this the expected behavior? The good thing tough is that there are no instances of the script classes, only the script classes themselves.

          Show
          jigga Arkadiusz Gasinski added a comment - The other thing that bothers me is that each time the test is run, there are always 20000 different script classes loaded (Script1 to Script20000), even though there's only 10 variations of the script. Is this the expected behavior? The good thing tough is that there are no instances of the script classes, only the script classes themselves.
          Hide
          jwagenleitner John Wagenleitner added a comment - - edited

          there are always 20000 different script classes loaded (Script1 to Script20000), even though there's only 10 variations of the script. Is this the expected behavior?

          Since the test project creates 20,000 engines and since the script class cache is per engine this will create 20k classes. This also creates lots of classloader instances and global cached methods. In this particular case it seems like creating a single engine with a logger binding and then using the context bean parameter to pass the taskId and receive the result would be the way to go. I realize the test project may have crafted to show the leak and not a real world example. A global class cache would help here, but I'm not sure if there are reasons (i.e., classloader) to keep it per instance. I'm not that familiar with JSR-223 so may be misunderstanding the use-case here.

          As for the large number of uncollected MetaMethodIndex$Entry instances I wonder if this might be similar to the issue reported in GROOVY-7083, since the script metaclass is modified and this might be why the classes are not unloaded.

          Show
          jwagenleitner John Wagenleitner added a comment - - edited there are always 20000 different script classes loaded (Script1 to Script20000), even though there's only 10 variations of the script. Is this the expected behavior? Since the test project creates 20,000 engines and since the script class cache is per engine this will create 20k classes. This also creates lots of classloader instances and global cached methods. In this particular case it seems like creating a single engine with a logger binding and then using the context bean parameter to pass the taskId and receive the result would be the way to go. I realize the test project may have crafted to show the leak and not a real world example. A global class cache would help here, but I'm not sure if there are reasons (i.e., classloader) to keep it per instance. I'm not that familiar with JSR-223 so may be misunderstanding the use-case here. As for the large number of uncollected MetaMethodIndex$Entry instances I wonder if this might be similar to the issue reported in GROOVY-7083 , since the script metaclass is modified and this might be why the classes are not unloaded.
          Hide
          blackdrag Jochen Theodorou added a comment -

          Thanks John. From the javadoc for ScriptEngineFactory:

          ScriptEngine getScriptEngine()
          
          Returns an instance of the ScriptEngine associated with this ScriptEngineFactory. A new ScriptEngine is generally returned, but implementations may pool, share or reuse engines.

          So it is not wrong to have 20k engines can happen... But of course this should still not cause a memory leak. As for the setting the meta class... The case in GROOVY-7083 sets a meta class in the registry, which forces the registry to keep a reference to this class at all times. But in the case here we set the meta class directly on the object, which only that object will have a reference to the meta class. So this should not have an effect and the object along with the meta class should be collectable (independent of the class and the classloader)

          Show
          blackdrag Jochen Theodorou added a comment - Thanks John. From the javadoc for ScriptEngineFactory: ScriptEngine getScriptEngine() Returns an instance of the ScriptEngine associated with this ScriptEngineFactory. A new ScriptEngine is generally returned, but implementations may pool, share or reuse engines. So it is not wrong to have 20k engines can happen... But of course this should still not cause a memory leak. As for the setting the meta class... The case in GROOVY-7083 sets a meta class in the registry, which forces the registry to keep a reference to this class at all times. But in the case here we set the meta class directly on the object, which only that object will have a reference to the meta class. So this should not have an effect and the object along with the meta class should be collectable (independent of the class and the classloader)
          Hide
          jwagenleitner John Wagenleitner added a comment -

          I tested the sample project with Groovy 2.3.11 and after the gc() call it unloaded all but about 2k classes. The MetaMethodIndex$Entry instance count dropped to 522. Tested with 2.4.0 and it behaves similar to 2.4.5 by not unloading the script classes.

          It looks like commit 97d78e9e52deb52c8e66db501ef208f30384d014 may have introduced a hard reference to the class (klazz) in combination of the ClassValue whereas I think it was a soft reference prior. In analyzing the heap dump it looks like

          java.lang.Thread
          - <Java Local>, contextClassLoader sun.misc.Launcher$AppClassLoader
              '- classes java.util.Vector
                '- elementData java.lang.Object[1280]
                   '- [822] class org.codehaus.groovy.reflection.ClassInfo
                      '- globalClassValue org.codehaus.groovy.reflection.GroovyClassValuePreJava7
                         '- map org.codehaus.groovy.reflection.GroovyClassValuePreJava7$GroovyClassValuePreJava7Map
                            '- segments org.codehaus.groovy.util.AbstractConcurrentMapBase$Segment[16]
                               '- [8] org.codehaus.groovy.reflection.GroovyClassValuePreJava7$GroovyClassValuePreJava7Segment
                                  '- table java.lang.Object[2048]  
                                     '- [1127] org.codehaus.groovy.reflection.GroovyClassValuePreJava7$EntryWithValue
                                        '- value org.codehaus.groovy.reflection.ClassInfo 
                                           '- klazz class Script15965                                        
                                              '- <classloader> groovy.lang.GroovyClassLoader$InnerLoader
          

          I'm not sure, but wanted to point it out since it seems to be a significant change between 2.3.x and 2.4.x that may have an affect on this issue.

          Show
          jwagenleitner John Wagenleitner added a comment - I tested the sample project with Groovy 2.3.11 and after the gc() call it unloaded all but about 2k classes. The MetaMethodIndex$Entry instance count dropped to 522. Tested with 2.4.0 and it behaves similar to 2.4.5 by not unloading the script classes. It looks like commit 97d78e9e52deb52c8e66db501ef208f30384d014 may have introduced a hard reference to the class (klazz) in combination of the ClassValue whereas I think it was a soft reference prior. In analyzing the heap dump it looks like java.lang. Thread - <Java Local>, contextClassLoader sun.misc.Launcher$AppClassLoader '- classes java.util.Vector '- elementData java.lang. Object [1280] '- [822] class org.codehaus.groovy.reflection.ClassInfo '- globalClassValue org.codehaus.groovy.reflection.GroovyClassValuePreJava7 '- map org.codehaus.groovy.reflection.GroovyClassValuePreJava7$GroovyClassValuePreJava7Map '- segments org.codehaus.groovy.util.AbstractConcurrentMapBase$Segment[16] '- [8] org.codehaus.groovy.reflection.GroovyClassValuePreJava7$GroovyClassValuePreJava7Segment '- table java.lang. Object [2048] '- [1127] org.codehaus.groovy.reflection.GroovyClassValuePreJava7$EntryWithValue '- value org.codehaus.groovy.reflection.ClassInfo '- klazz class Script15965 '- <classloader> groovy.lang.GroovyClassLoader$InnerLoader I'm not sure, but wanted to point it out since it seems to be a significant change between 2.3.x and 2.4.x that may have an affect on this issue.
          Hide
          blackdrag Jochen Theodorou added a comment -

          the change you mention was made to use ClassValue, yes. But because of GROOVY-7591 (and https://bugs.openjdk.java.net/browse/JDK-8136353) Groovy 2.4.5 does not use ClassValue by default anymore... and it does not show up in the dump you show here as well. But yes... if those are all hard references (and it looks like it), then it is not right.

          Show
          blackdrag Jochen Theodorou added a comment - the change you mention was made to use ClassValue, yes. But because of GROOVY-7591 (and https://bugs.openjdk.java.net/browse/JDK-8136353 ) Groovy 2.4.5 does not use ClassValue by default anymore... and it does not show up in the dump you show here as well. But yes... if those are all hard references (and it looks like it), then it is not right.
          Hide
          jkemnade Jochen Kemnade added a comment - - edited

          I'm also experiencing those leaks.

          this     - value: org.codehaus.groovy.reflection.ClassInfo #392
           <- value     - class: org.codehaus.groovy.reflection.GroovyClassValuePreJava7$EntryWithValue, value: org.codehaus.groovy.reflection.ClassInfo #392
            <- [65]     - class: java.lang.Object[], value: org.codehaus.groovy.reflection.GroovyClassValuePreJava7$EntryWithValue #317
             <- table     - class: org.codehaus.groovy.reflection.GroovyClassValuePreJava7$GroovyClassValuePreJava7Segment, value: java.lang.Object[] #424042
              <- [11]     - class: org.codehaus.groovy.util.AbstractConcurrentMapBase$Segment[], value: org.codehaus.groovy.reflection.GroovyClassValuePreJava7$GroovyClassValuePreJava7Segment #5
               <- segments     - class: org.codehaus.groovy.reflection.GroovyClassValuePreJava7$GroovyClassValuePreJava7Map, value: org.codehaus.groovy.util.AbstractConcurrentMapBase$Segment[] #2
                <- map     - class: org.codehaus.groovy.reflection.GroovyClassValuePreJava7, value: org.codehaus.groovy.reflection.GroovyClassValuePreJava7$GroovyClassValuePreJava7Map #1
                 <- globalClassValue     - class: org.codehaus.groovy.reflection.ClassInfo, value: org.codehaus.groovy.reflection.GroovyClassValuePreJava7 #1
                  <- [8080]     - class: java.lang.Object[], value: org.codehaus.groovy.reflection.ClassInfo class ClassInfo
                   <- elementData     - class: java.util.Vector, value: java.lang.Object[] #78509
                    <- classes     - class: org.apache.catalina.loader.WebappClassLoader, value: java.util.Vector #242
          

          Can we do something about them for 2.4.6? I'd try to help with a patch but I'm afraid that this part of the Groovy code is still over my head.

          Show
          jkemnade Jochen Kemnade added a comment - - edited I'm also experiencing those leaks. this - value: org.codehaus.groovy.reflection.ClassInfo #392 <- value - class: org.codehaus.groovy.reflection.GroovyClassValuePreJava7$EntryWithValue, value: org.codehaus.groovy.reflection.ClassInfo #392 <- [65] - class: java.lang.Object[], value: org.codehaus.groovy.reflection.GroovyClassValuePreJava7$EntryWithValue #317 <- table - class: org.codehaus.groovy.reflection.GroovyClassValuePreJava7$GroovyClassValuePreJava7Segment, value: java.lang.Object[] #424042 <- [11] - class: org.codehaus.groovy.util.AbstractConcurrentMapBase$Segment[], value: org.codehaus.groovy.reflection.GroovyClassValuePreJava7$GroovyClassValuePreJava7Segment #5 <- segments - class: org.codehaus.groovy.reflection.GroovyClassValuePreJava7$GroovyClassValuePreJava7Map, value: org.codehaus.groovy.util.AbstractConcurrentMapBase$Segment[] #2 <- map - class: org.codehaus.groovy.reflection.GroovyClassValuePreJava7, value: org.codehaus.groovy.reflection.GroovyClassValuePreJava7$GroovyClassValuePreJava7Map #1 <- globalClassValue - class: org.codehaus.groovy.reflection.ClassInfo, value: org.codehaus.groovy.reflection.GroovyClassValuePreJava7 #1 <- [8080] - class: java.lang.Object[], value: org.codehaus.groovy.reflection.ClassInfo class ClassInfo <- elementData - class: java.util.Vector, value: java.lang.Object[] #78509 <- classes - class: org.apache.catalina.loader.WebappClassLoader, value: java.util.Vector #242 Can we do something about them for 2.4.6? I'd try to help with a patch but I'm afraid that this part of the Groovy code is still over my head.
          Hide
          jkemnade Jochen Kemnade added a comment -

          If it's just a matter of replacing the hard reference by a soft one, here's a patch. If not, maybe someone can give a hint as to how this should be solved.

          Show
          jkemnade Jochen Kemnade added a comment - If it's just a matter of replacing the hard reference by a soft one, here's a patch. If not, maybe someone can give a hint as to how this should be solved.
          Hide
          blackdrag Jochen Theodorou added a comment -

          normally the ClassInfo object does not only refer to the class directly, but also indirectly using hard references, for example through method methods, which are based on the real methods of the class, which are based on reflection, which then have direct references to classes again, of which usually the class in question is one. If a patch like this fixes the problem so should enabling CMS. So frankly I would not expect this patch to work, in fact I would be quite surprised if it does. But if someone has this problem and can try out if the patch changes anything, I am curious to hear about the results. I would help even if the patch would not work out

          Show
          blackdrag Jochen Theodorou added a comment - normally the ClassInfo object does not only refer to the class directly, but also indirectly using hard references, for example through method methods, which are based on the real methods of the class, which are based on reflection, which then have direct references to classes again, of which usually the class in question is one. If a patch like this fixes the problem so should enabling CMS. So frankly I would not expect this patch to work, in fact I would be quite surprised if it does. But if someone has this problem and can try out if the patch changes anything, I am curious to hear about the results. I would help even if the patch would not work out
          Hide
          jkemnade Jochen Kemnade added a comment -

          This does not seem to fix GROOVY-7083 however.

          Show
          jkemnade Jochen Kemnade added a comment - This does not seem to fix GROOVY-7083 however.
          Hide
          jkemnade Jochen Kemnade added a comment -

          Oh, I read your comment too late. Yes, apparently it doesn't fix the problem. Maybe I misunderstood your comment about the hard references.

          Show
          jkemnade Jochen Kemnade added a comment - Oh, I read your comment too late. Yes, apparently it doesn't fix the problem. Maybe I misunderstood your comment about the hard references.
          Hide
          jwagenleitner John Wagenleitner added a comment -

          I tested the referenced Github LeakTest project again and this time created some memory pressure after the loop by calling the following method:

              // Create memory pressure to force soft reference collection
              private void createOOM() {
                  List<Long[]> buffer = new ArrayList<Long[]>(100);
                  int chunk = 128 * 1024 * 1024;
                  int limit = 50000;
                  try {
                      for (int i = 0; i < limit; i++) {
                          buffer.add(new Long[chunk]);
                      }
                  } catch (OutOfMemoryError oom) {
                      buffer.clear();
                      return;
                  }
                  throw new RuntimeException("OOM expected");
              }
          

          Using the default setting with ClassValue disabled things looked pretty much the same as before. However, enabling ClassValue by adding the following to the static initializer:

          System.setProperty("groovy.use.classvalue", "true");
          

          Seemed to dramatically reduce the heap size.

          Size: 60mb vs 1.2G
          Classes: 2.6k vs 22.6k
          Objects: 1.5m vs 18.2m
          Class Loader: 6 vs 40k

          So I think Jochen Theodorou original suggestion to try enabling ClassValue is worth a try. You might still see heap usage creep up over time but I would expect as memory pressure builds close to the 8G max you'll see GC work and should experience a lot less severe leak than without using it.

          Show
          jwagenleitner John Wagenleitner added a comment - I tested the referenced Github LeakTest project again and this time created some memory pressure after the loop by calling the following method: // Create memory pressure to force soft reference collection private void createOOM() { List< Long []> buffer = new ArrayList< Long []>(100); int chunk = 128 * 1024 * 1024; int limit = 50000; try { for ( int i = 0; i < limit; i++) { buffer.add( new Long [chunk]); } } catch (OutOfMemoryError oom) { buffer.clear(); return ; } throw new RuntimeException( "OOM expected" ); } Using the default setting with ClassValue disabled things looked pretty much the same as before. However, enabling ClassValue by adding the following to the static initializer: System .setProperty( "groovy.use.classvalue" , " true " ); Seemed to dramatically reduce the heap size. Size: 60mb vs 1.2G Classes: 2.6k vs 22.6k Objects: 1.5m vs 18.2m Class Loader: 6 vs 40k So I think Jochen Theodorou original suggestion to try enabling ClassValue is worth a try. You might still see heap usage creep up over time but I would expect as memory pressure builds close to the 8G max you'll see GC work and should experience a lot less severe leak than without using it.
          Hide
          jkemnade Jochen Kemnade added a comment -

          Yes, the classes are properly cleaned up with groovy.use.classvalue=true, so I guess the problem must be in GroovyClassValuePreJava7.
          Err, this may be a stupid question, but, is GroovyClassValue.remove(Class) ever called?

          Show
          jkemnade Jochen Kemnade added a comment - Yes, the classes are properly cleaned up with groovy.use.classvalue=true , so I guess the problem must be in GroovyClassValuePreJava7 . Err, this may be a stupid question, but, is GroovyClassValue.remove(Class) ever called?
          Show
          pascalschumacher Pascal Schumacher added a comment - If the problem is in GroovyClassValuePreJava7 one could compare https://github.com/apache/groovy/blob/master/src/main/org/codehaus/groovy/reflection/GroovyClassValuePreJava7.java and http://hg.openjdk.java.net/jdk8u/jdk8u60-dev/jdk/file/tip/src/share/classes/java/lang/ClassValue.java for differences.
          Hide
          pascalschumacher Pascal Schumacher added a comment - - edited

          Intellij can not find any usages of GroovyClassValue.remove(Class). I do not know anything about this part of Groovy so no idea were it's supposed to be used.

          Show
          pascalschumacher Pascal Schumacher added a comment - - edited Intellij can not find any usages of GroovyClassValue.remove(Class). I do not know anything about this part of Groovy so no idea were it's supposed to be used.
          Hide
          jwagenleitner John Wagenleitner added a comment -

          With GroovyClassValuePreJava7 I don't think GroovyClassValue.remove(Class) is designed to be called directly, I think it would be enough if the cached Entry in the ManagedConcurrentMap was collected due to its weakly held key (Class) being collected. I believe the issue is that the cached value in the map (ClassInfo) stores a strong reference to the cached key (Class). So I don't think the Entry ever gets cleared. I think changing this to a weak ref would allow the cache entry to be cleared, but hard to tell what problems that might cause or if it would ever be possible for the Class to be collected while the ClassInfo is used causing the weak ref to return null for the Class.

          I also think the ClassInfoCleanup is not doing what it's expected to do for cleanup. Since it's not stored it goes out of scope immediately and will be collected long before the ClassInfo it's meant to clean up after is eligible for collection.

          I'm not familiar with this part of the code, so might be way off.

          Show
          jwagenleitner John Wagenleitner added a comment - With GroovyClassValuePreJava7 I don't think GroovyClassValue.remove(Class) is designed to be called directly, I think it would be enough if the cached Entry in the ManagedConcurrentMap was collected due to its weakly held key (Class) being collected. I believe the issue is that the cached value in the map (ClassInfo) stores a strong reference to the cached key (Class) . So I don't think the Entry ever gets cleared. I think changing this to a weak ref would allow the cache entry to be cleared, but hard to tell what problems that might cause or if it would ever be possible for the Class to be collected while the ClassInfo is used causing the weak ref to return null for the Class. I also think the ClassInfoCleanup is not doing what it's expected to do for cleanup . Since it's not stored it goes out of scope immediately and will be collected long before the ClassInfo it's meant to clean up after is eligible for collection. I'm not familiar with this part of the code, so might be way off.
          Hide
          blackdrag Jochen Theodorou added a comment -

          How things on the JVM are supposed to be working is the following... imagine a class that is nowhere referenced anymore, but in an equally otherwise non-referenced class loader and another object. Imagine this other object is softly reachable, meaning no strong/weak/phantom reference is pointing to it.This is supposed to mean, that the class is softly reachable as well. So in theory, as long as the ClassInfo object is not strongly reachable, the ClassInfo object could be collected along with the class.
          In the practice though, things do not always work like that. I know for sure, that the IBM JVM for example needs special options given to it, to even check such a case. For the Oracle JVM enabling CMS used to help with this problem. For the Oracle JVM the problem is that normally the garbage collection for classes and the garbage collection for instances is not done together. So unless the ClassInfo object is collected it happens that the class will stay. Since ClassInfo objects are normally softly referenced, they will stay till the very last moment, increasing the probability of the problem to occur. The fact, that there is a multitude of objects referenced by ClassInfo, which hold strong references to the class (for example a reflective method object could be one, but the type information in the meta methods is already enough) does not help the garbage collector at all.
          As bad as it sounds, in the end it depends on the implementation of the garbage collector if that object can be collected. So it is supposed to work, but can for several reasons not sometimes.

          Of course that is all theory and past experience. It is very possible, that there is somewhere a strong reference, where it should not be. But as I tried to explain above, changing ClassInfo to use a weak key for the Class is not something I would expect to really help. The patch that is attached here, tries it with a SoftReference. WeakReference could be better, but frankly there are still many many strong references to the class through other objects hard referenced by the ClassInfo object. Could be, that this decreases the number of hard references just enough to not be over some threshold optimization in the garbage collector and then allows the garbage collector to collect. Or it might not have any effect. My expectation is that it won't change anything.

          As for the ClassInfoCleanup.. it is not right that it is not stored. When you create a Soft/WeakReference, then there will be a reference object and a ReferenceQueue. The queue will know the reference. And it is not that those references simply vanish. In fact, if you don't clean up the queue, you will get a memory leak from that side, even if the objects you reference would be collected. But for Weak/SoftReference this is not actually the case, you would need PhantomReference for that. On the other hand, you are right, that this code has almost zero relevance when it comes to the automatic removal of ClassInfo. If the ClassInfo has a strong reference set, the ClassInfo object itself will not be weak referenced. Same for the cleanups... there are more to support garbage collection a little, than that they are really needed. The real code managing the reference is in ManagedLinkedList as far as GlobalClassSet is concerned. Or the manged hashmap in GroovyClassValuePreJava7.

          Oh, one correction... ClassInfo is mostly weakly reachable in those structures, not softly. cachedClassRef and artifactClassLoader are should be soft reachable, same for the entries of LocalMap

          Show
          blackdrag Jochen Theodorou added a comment - How things on the JVM are supposed to be working is the following... imagine a class that is nowhere referenced anymore, but in an equally otherwise non-referenced class loader and another object. Imagine this other object is softly reachable, meaning no strong/weak/phantom reference is pointing to it.This is supposed to mean, that the class is softly reachable as well. So in theory, as long as the ClassInfo object is not strongly reachable, the ClassInfo object could be collected along with the class. In the practice though, things do not always work like that. I know for sure, that the IBM JVM for example needs special options given to it, to even check such a case. For the Oracle JVM enabling CMS used to help with this problem. For the Oracle JVM the problem is that normally the garbage collection for classes and the garbage collection for instances is not done together. So unless the ClassInfo object is collected it happens that the class will stay. Since ClassInfo objects are normally softly referenced, they will stay till the very last moment, increasing the probability of the problem to occur. The fact, that there is a multitude of objects referenced by ClassInfo, which hold strong references to the class (for example a reflective method object could be one, but the type information in the meta methods is already enough) does not help the garbage collector at all. As bad as it sounds, in the end it depends on the implementation of the garbage collector if that object can be collected. So it is supposed to work, but can for several reasons not sometimes. Of course that is all theory and past experience. It is very possible, that there is somewhere a strong reference, where it should not be. But as I tried to explain above, changing ClassInfo to use a weak key for the Class is not something I would expect to really help. The patch that is attached here, tries it with a SoftReference. WeakReference could be better, but frankly there are still many many strong references to the class through other objects hard referenced by the ClassInfo object. Could be, that this decreases the number of hard references just enough to not be over some threshold optimization in the garbage collector and then allows the garbage collector to collect. Or it might not have any effect. My expectation is that it won't change anything. As for the ClassInfoCleanup.. it is not right that it is not stored. When you create a Soft/WeakReference, then there will be a reference object and a ReferenceQueue. The queue will know the reference. And it is not that those references simply vanish. In fact, if you don't clean up the queue, you will get a memory leak from that side, even if the objects you reference would be collected. But for Weak/SoftReference this is not actually the case, you would need PhantomReference for that. On the other hand, you are right, that this code has almost zero relevance when it comes to the automatic removal of ClassInfo. If the ClassInfo has a strong reference set, the ClassInfo object itself will not be weak referenced. Same for the cleanups... there are more to support garbage collection a little, than that they are really needed. The real code managing the reference is in ManagedLinkedList as far as GlobalClassSet is concerned. Or the manged hashmap in GroovyClassValuePreJava7. Oh, one correction... ClassInfo is mostly weakly reachable in those structures, not softly. cachedClassRef and artifactClassLoader are should be soft reachable, same for the entries of LocalMap
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user jwagenleitner opened a pull request:

          https://github.com/apache/groovy/pull/219

          GROOVY-7683 - Memory leak when using Groovy as JSR-223 scripting language

          I am unsure what if any problems making the Class a `WeakReference` might have but thought I'd put this out there for review.

          I also tested against GROOVY-7646(https://issues.apache.org/jira/browse/GROOVY-7646) which was consistently throwing OOME after <2 mins prior to these changes and ran to completion with changes.

          Another thing I noticed in looking at this class is that [uses a `PhantomReference`](https://github.com/apache/groovy/blob/73f5979a468f1508134eba20ce503630b0fe0cc7/src/main/org/codehaus/groovy/reflection/ClassInfo.java#L405) and attempts [to use the value from `get()`](https://github.com/apache/groovy/blob/73f5979a468f1508134eba20ce503630b0fe0cc7/src/main/org/codehaus/groovy/reflection/ClassInfo.java#L435). However, this will always return `null` so the code path never executes.

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/jwagenleitner/groovy GROOVY-7683

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/groovy/pull/219.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #219


          commit 10689a29cbf167c40b05c38aa5c144724045eeb0
          Author: John Wagenleitner <jwagenleitner@apache.org>
          Date: 2015-12-17T00:44:29Z

          GROOVY-7683 - Memory leak when using Groovy as JSR-223 scripting language


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user jwagenleitner opened a pull request: https://github.com/apache/groovy/pull/219 GROOVY-7683 - Memory leak when using Groovy as JSR-223 scripting language I am unsure what if any problems making the Class a `WeakReference` might have but thought I'd put this out there for review. I also tested against GROOVY-7646 ( https://issues.apache.org/jira/browse/GROOVY-7646 ) which was consistently throwing OOME after <2 mins prior to these changes and ran to completion with changes. Another thing I noticed in looking at this class is that [uses a `PhantomReference`] ( https://github.com/apache/groovy/blob/73f5979a468f1508134eba20ce503630b0fe0cc7/src/main/org/codehaus/groovy/reflection/ClassInfo.java#L405 ) and attempts [to use the value from `get()`] ( https://github.com/apache/groovy/blob/73f5979a468f1508134eba20ce503630b0fe0cc7/src/main/org/codehaus/groovy/reflection/ClassInfo.java#L435 ). However, this will always return `null` so the code path never executes. You can merge this pull request into a Git repository by running: $ git pull https://github.com/jwagenleitner/groovy GROOVY-7683 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/groovy/pull/219.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #219 commit 10689a29cbf167c40b05c38aa5c144724045eeb0 Author: John Wagenleitner <jwagenleitner@apache.org> Date: 2015-12-17T00:44:29Z GROOVY-7683 - Memory leak when using Groovy as JSR-223 scripting language
          Hide
          jwagenleitner John Wagenleitner added a comment -

          Created a PR, this is scary and unfamiliar part of the code but thought I'd put it out there for review. Am really interested in learning more about this part of the code so appreciate the comments so far.

          As for the ClassInfoCleanup.. it is not right that it is not stored. When you create a Soft/WeakReference, then there will be a reference object and a ReferenceQueue. The queue will know the reference. And it is not that those references simply vanish.

          My understanding is that the ReferenceQueue will know of the reference (ClassInfoCleanup) when the referent (ClassInfo) is no longer reachable by strong or soft references. However, in order for the reference (ClassInfoCleanup) to be enqueued it must be alive when this happens. But I don't see any reason why the GC would hold on to this object (the ClassInfoCleanup) after the constructor completes. So that's why I believe that ClassInfoCleanup and DebugRef will never run the finalize method. Not to mention that both do not override the ManagedReference#finalizeReference but instead have it named finalizeRef.

          Show
          jwagenleitner John Wagenleitner added a comment - Created a PR, this is scary and unfamiliar part of the code but thought I'd put it out there for review. Am really interested in learning more about this part of the code so appreciate the comments so far. As for the ClassInfoCleanup.. it is not right that it is not stored. When you create a Soft/WeakReference, then there will be a reference object and a ReferenceQueue. The queue will know the reference. And it is not that those references simply vanish. My understanding is that the ReferenceQueue will know of the reference (ClassInfoCleanup) when the referent (ClassInfo) is no longer reachable by strong or soft references. However, in order for the reference (ClassInfoCleanup) to be enqueued it must be alive when this happens. But I don't see any reason why the GC would hold on to this object (the ClassInfoCleanup) after the constructor completes. So that's why I believe that ClassInfoCleanup and DebugRef will never run the finalize method. Not to mention that both do not override the ManagedReference#finalizeReference but instead have it named finalizeRef .
          Hide
          blackdrag Jochen Theodorou added a comment -

          John, you are right... I should not write comments on JIRA at 2:00 in the morning. The Reference has to be alive to be enqueued, you are fully right there. The conclusion that ClassInfoCleanup is then doing about nothing is most likely right as well, some for DebugRef. Those have been handled different before the ClassValue rewrite.

          Show
          blackdrag Jochen Theodorou added a comment - John, you are right... I should not write comments on JIRA at 2:00 in the morning. The Reference has to be alive to be enqueued, you are fully right there. The conclusion that ClassInfoCleanup is then doing about nothing is most likely right as well, some for DebugRef. Those have been handled different before the ClassValue rewrite.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user blackdrag commented on a diff in the pull request:

          https://github.com/apache/groovy/pull/219#discussion_r61761426

          — Diff: src/main/org/codehaus/groovy/util/ManagedReference.java —
          @@ -46,7 +46,6 @@ public final T get() {

          public final void clear() {
          ref.clear();

          • manager.removeStallEntries();
              • End diff –

          Why did you remove this call?

          Show
          githubbot ASF GitHub Bot added a comment - Github user blackdrag commented on a diff in the pull request: https://github.com/apache/groovy/pull/219#discussion_r61761426 — Diff: src/main/org/codehaus/groovy/util/ManagedReference.java — @@ -46,7 +46,6 @@ public final T get() { public final void clear() { ref.clear(); manager.removeStallEntries(); End diff – Why did you remove this call?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user blackdrag commented on a diff in the pull request:

          https://github.com/apache/groovy/pull/219#discussion_r61761881

          — Diff: src/main/org/codehaus/groovy/reflection/ClassInfo.java —
          @@ -35,13 +36,13 @@
          *

          • @author Alex.Tkachman
            */
            -public class ClassInfo {
            +public class ClassInfo implements Finalizable {

          private final LazyCachedClassRef cachedClassRef;
          private final LazyClassLoaderRef artifactClassLoader;
          private final LockableObject lock = new LockableObject();
          public final int hash = -1;

          • private final Class klazz;
            + private final WeakReference<Class<?>> klazz;
              • End diff –

          I wonder what we should do if the referenced class has been collected. At least in the non - ClassValue version, it might be, that the ClassInfo still exists. And if then somebody tries to get the meta class using that ClassInfo things will blow up

          Show
          githubbot ASF GitHub Bot added a comment - Github user blackdrag commented on a diff in the pull request: https://github.com/apache/groovy/pull/219#discussion_r61761881 — Diff: src/main/org/codehaus/groovy/reflection/ClassInfo.java — @@ -35,13 +36,13 @@ * @author Alex.Tkachman */ -public class ClassInfo { +public class ClassInfo implements Finalizable { private final LazyCachedClassRef cachedClassRef; private final LazyClassLoaderRef artifactClassLoader; private final LockableObject lock = new LockableObject(); public final int hash = -1; private final Class klazz; + private final WeakReference<Class<?>> klazz; End diff – I wonder what we should do if the referenced class has been collected. At least in the non - ClassValue version, it might be, that the ClassInfo still exists. And if then somebody tries to get the meta class using that ClassInfo things will blow up
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user jwagenleitner commented on a diff in the pull request:

          https://github.com/apache/groovy/pull/219#discussion_r61764022

          — Diff: src/main/org/codehaus/groovy/util/ManagedReference.java —
          @@ -46,7 +46,6 @@ public final T get() {

          public final void clear() {
          ref.clear();

          • manager.removeStallEntries();
              • End diff –

          `ClassInfo#finalizeReference` calls `setStrongMetaClass(null)` which in turn calls `replaceWeakMetaClassRef(null)` which calls `weakRef#clear` which is on ManagedReference. This causes a recursive call back into the queue can can lead to stackoverflow if there are lots of ClassInfo ManagedReferences in the Reference Queue.

          A related PR that attempts to prevent this reentrant processing is PR #298.

          Show
          githubbot ASF GitHub Bot added a comment - Github user jwagenleitner commented on a diff in the pull request: https://github.com/apache/groovy/pull/219#discussion_r61764022 — Diff: src/main/org/codehaus/groovy/util/ManagedReference.java — @@ -46,7 +46,6 @@ public final T get() { public final void clear() { ref.clear(); manager.removeStallEntries(); End diff – `ClassInfo#finalizeReference` calls `setStrongMetaClass(null)` which in turn calls `replaceWeakMetaClassRef(null)` which calls `weakRef#clear` which is on ManagedReference. This causes a recursive call back into the queue can can lead to stackoverflow if there are lots of ClassInfo ManagedReferences in the Reference Queue. A related PR that attempts to prevent this reentrant processing is PR #298.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user jwagenleitner commented on a diff in the pull request:

          https://github.com/apache/groovy/pull/219#discussion_r61764947

          — Diff: src/main/org/codehaus/groovy/reflection/ClassInfo.java —
          @@ -35,13 +36,13 @@
          *

          • @author Alex.Tkachman
            */
            -public class ClassInfo {
            +public class ClassInfo implements Finalizable {

          private final LazyCachedClassRef cachedClassRef;
          private final LazyClassLoaderRef artifactClassLoader;
          private final LockableObject lock = new LockableObject();
          public final int hash = -1;

          • private final Class klazz;
            + private final WeakReference<Class<?>> klazz;
              • End diff –

          That's the question I've struggled with too. My assumption here is that only the Groovy Runtime should ever ask for ClassInfo objects and in most places I can find they do so either with a Class or an instance of the Class which I think would mean it wouldn't have been collected. But it's been difficult for me to be certain that the case can never happen. Best I can think of is maybe check if the ref is null and throw a `GroovyBugError`.

          Show
          githubbot ASF GitHub Bot added a comment - Github user jwagenleitner commented on a diff in the pull request: https://github.com/apache/groovy/pull/219#discussion_r61764947 — Diff: src/main/org/codehaus/groovy/reflection/ClassInfo.java — @@ -35,13 +36,13 @@ * @author Alex.Tkachman */ -public class ClassInfo { +public class ClassInfo implements Finalizable { private final LazyCachedClassRef cachedClassRef; private final LazyClassLoaderRef artifactClassLoader; private final LockableObject lock = new LockableObject(); public final int hash = -1; private final Class klazz; + private final WeakReference<Class<?>> klazz; End diff – That's the question I've struggled with too. My assumption here is that only the Groovy Runtime should ever ask for ClassInfo objects and in most places I can find they do so either with a Class or an instance of the Class which I think would mean it wouldn't have been collected. But it's been difficult for me to be certain that the case can never happen. Best I can think of is maybe check if the ref is null and throw a `GroovyBugError`.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user blackdrag commented on a diff in the pull request:

          https://github.com/apache/groovy/pull/219#discussion_r61768963

          — Diff: src/main/org/codehaus/groovy/util/ManagedReference.java —
          @@ -46,7 +46,6 @@ public final T get() {

          public final void clear() {
          ref.clear();

          • manager.removeStallEntries();
              • End diff –

          ok, I +1ed PR #298

          Show
          githubbot ASF GitHub Bot added a comment - Github user blackdrag commented on a diff in the pull request: https://github.com/apache/groovy/pull/219#discussion_r61768963 — Diff: src/main/org/codehaus/groovy/util/ManagedReference.java — @@ -46,7 +46,6 @@ public final T get() { public final void clear() { ref.clear(); manager.removeStallEntries(); End diff – ok, I +1ed PR #298
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user blackdrag commented on a diff in the pull request:

          https://github.com/apache/groovy/pull/219#discussion_r61769520

          — Diff: src/main/org/codehaus/groovy/reflection/ClassInfo.java —
          @@ -35,13 +36,13 @@
          *

          • @author Alex.Tkachman
            */
            -public class ClassInfo {
            +public class ClassInfo implements Finalizable {

          private final LazyCachedClassRef cachedClassRef;
          private final LazyClassLoaderRef artifactClassLoader;
          private final LockableObject lock = new LockableObject();
          public final int hash = -1;

          • private final Class klazz;
            + private final WeakReference<Class<?>> klazz;
              • End diff –

          I think it would be sufficent to add to each method an according javadoc comment, as well as a class comment, that it is not safe to use without a Class associated with this ClassInfo. I would even consider a method to get the referenced Class, so people can check/ensure it will continue to exist. In that case we can probably avoid the checks and exceptions

          Show
          githubbot ASF GitHub Bot added a comment - Github user blackdrag commented on a diff in the pull request: https://github.com/apache/groovy/pull/219#discussion_r61769520 — Diff: src/main/org/codehaus/groovy/reflection/ClassInfo.java — @@ -35,13 +36,13 @@ * @author Alex.Tkachman */ -public class ClassInfo { +public class ClassInfo implements Finalizable { private final LazyCachedClassRef cachedClassRef; private final LazyClassLoaderRef artifactClassLoader; private final LockableObject lock = new LockableObject(); public final int hash = -1; private final Class klazz; + private final WeakReference<Class<?>> klazz; End diff – I think it would be sufficent to add to each method an according javadoc comment, as well as a class comment, that it is not safe to use without a Class associated with this ClassInfo. I would even consider a method to get the referenced Class, so people can check/ensure it will continue to exist. In that case we can probably avoid the checks and exceptions
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user jwagenleitner commented on a diff in the pull request:

          https://github.com/apache/groovy/pull/219#discussion_r61834705

          — Diff: src/main/org/codehaus/groovy/util/ManagedReference.java —
          @@ -46,7 +46,6 @@ public final T get() {

          public final void clear() {
          ref.clear();

          • manager.removeStallEntries();
              • End diff –

          Thanks for reviewing that other PR. I merged that PR and will rebase this and leave the call to manager.removeStallEntries().

          Show
          githubbot ASF GitHub Bot added a comment - Github user jwagenleitner commented on a diff in the pull request: https://github.com/apache/groovy/pull/219#discussion_r61834705 — Diff: src/main/org/codehaus/groovy/util/ManagedReference.java — @@ -46,7 +46,6 @@ public final T get() { public final void clear() { ref.clear(); manager.removeStallEntries(); End diff – Thanks for reviewing that other PR. I merged that PR and will rebase this and leave the call to manager.removeStallEntries().
          Hide
          vguna Veit Guna added a comment -

          Any news on this?

          I also encountered the mentioned problem, that MetaMethodIndex$Entry classes are growing quickly without being able to be GCed.
          My scenario is during loadtests on the client side. I use restassured 2.9.0 (uses groovy 2.4.6) to perform json parsing. I could track it down to this little snippet
          to reproduce the behavior:

          ...
          import com.jayway.restassured.path.json.JsonPath;
          ...
          	public static void main(String[] args) throws InterruptedException {
          		for (int ci = 0; ci < 100000; ci++) {
          			JsonPath.from("{\"id\": \"" + UUID.randomUUID().toString() + "\"}").getUUID("id");
          			Thread.sleep(1);
          		}
          	}
          

          Simply take a look using jvisualvm and you see the results.
          I also tried the latest 2.4.x groovy version and latest 2.3.x with the same results.

          Show
          vguna Veit Guna added a comment - Any news on this? I also encountered the mentioned problem, that MetaMethodIndex$Entry classes are growing quickly without being able to be GCed. My scenario is during loadtests on the client side. I use restassured 2.9.0 (uses groovy 2.4.6) to perform json parsing. I could track it down to this little snippet to reproduce the behavior: ... import com.jayway.restassured.path.json.JsonPath; ... public static void main( String [] args) throws InterruptedException { for ( int ci = 0; ci < 100000; ci++) { JsonPath.from( "{\" id\ ": \" " + UUID.randomUUID().toString() + " \ "}" ).getUUID( "id" ); Thread .sleep(1); } } Simply take a look using jvisualvm and you see the results. I also tried the latest 2.4.x groovy version and latest 2.3.x with the same results.
          Hide
          gsAndrew Andrew Garcia added a comment - - edited

          I've been tracking/trying to isolate this memory leak for a while now. I was running Grails 2.5.3 (using Groovy 2.4.4) on Heroku monitoring with New Relic. First I ran into New Relic's leaky agent with issues around async servlets, upgraded to their latest agent, and then I started running into this.

          I've since forced Grails 2.5.3 to load Groovy 2.4.5 when I was seeing a massive amount of ClassInfo's hanging around and the "Expando" class. Recently I've been seeing a ton of java.ref.lang.Finalizer's..

          Anyhow, I wanted to comment that I've played around with toggling groovy.use.classvalue and the image I attached is when I switched it to true. It's an interesting visualization to how the GC is behaving and how my non-heap memory is continuing to creep higher and higher although it appears to be at a slower pace than without it set.

          Show
          gsAndrew Andrew Garcia added a comment - - edited I've been tracking/trying to isolate this memory leak for a while now. I was running Grails 2.5.3 (using Groovy 2.4.4) on Heroku monitoring with New Relic. First I ran into New Relic's leaky agent with issues around async servlets, upgraded to their latest agent, and then I started running into this. I've since forced Grails 2.5.3 to load Groovy 2.4.5 when I was seeing a massive amount of ClassInfo's hanging around and the "Expando" class. Recently I've been seeing a ton of java.ref.lang.Finalizer's.. Anyhow, I wanted to comment that I've played around with toggling groovy.use.classvalue and the image I attached is when I switched it to true. It's an interesting visualization to how the GC is behaving and how my non-heap memory is continuing to creep higher and higher although it appears to be at a slower pace than without it set.
          Hide
          gsAndrew Andrew Garcia added a comment - - edited

          Linking to Groovy-7623 as it claims to be a possible solution path to this memory leak issue.

          Show
          gsAndrew Andrew Garcia added a comment - - edited Linking to Groovy-7623 as it claims to be a possible solution path to this memory leak issue.
          Hide
          gsAndrew Andrew Garcia added a comment -

          Also linking because I added a walkthrough on how I got that JDK to play nice with my grails project.

          Show
          gsAndrew Andrew Garcia added a comment - Also linking because I added a walkthrough on how I got that JDK to play nice with my grails project.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/groovy/pull/219

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/groovy/pull/219
          Hide
          jwagenleitner John Wagenleitner added a comment -

          Thanks for reporting the issue. I believe the referenced PR will fix the issue, but please re-open if you find that to not be the case.

          Show
          jwagenleitner John Wagenleitner added a comment - Thanks for reporting the issue. I believe the referenced PR will fix the issue, but please re-open if you find that to not be the case.
          Hide
          gsAndrew Andrew Garcia added a comment -

          Any idea when the expected 2.4.8 release date is?

          Show
          gsAndrew Andrew Garcia added a comment - Any idea when the expected 2.4.8 release date is?
          Hide
          jwagenleitner John Wagenleitner added a comment -

          Not sure, but there's been some talk about it on the dev mailing list which is usually a sign that it's not too far off.

          Show
          jwagenleitner John Wagenleitner added a comment - Not sure, but there's been some talk about it on the dev mailing list which is usually a sign that it's not too far off.

            People

            • Assignee:
              jwagenleitner John Wagenleitner
              Reporter:
              jigga Arkadiusz Gasinski
            • Votes:
              9 Vote for this issue
              Watchers:
              18 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development