Memory leak introduced with the fix of JENKINS-36372 #41

jglick · 2016-08-12T13:20:39Z

Had disabled a test due to still-undiagnosed failures on Jenkins 2, which concealed the fact that a recent bug fix (#29) regressed the leak on Jenkins 1:

{groovy.lang.GroovyClassLoader$InnerLoader@571df0f9=private static final org.jboss.marshalling.reflect.SerializableClassRegistry org.jboss.marshalling.reflect.SerializableClassRegistry.INSTANCE->
org.jboss.marshalling.reflect.SerializableClassRegistry@67289b2f-registry->
org.jboss.marshalling.reflect.UnlockedHashMap@46269dbf-table->
org.jboss.marshalling.reflect.UnlockedHashMap$Table@4a13d1ef-array->
[Ljava.lang.Object;@2b2b3f3-[121]->
[Lorg.jboss.marshalling.reflect.UnlockedHashMap$Item;@182e8565-[0]->
org.jboss.marshalling.reflect.UnlockedHashMap$Item@13d66b69-key->
groovy.lang.GroovyClassLoader$InnerLoader@571df0f9}

@reviewbybees

…cting a memory leak regression introduced with the fix of JENKINS-36372 in 2.9.

jglick · 2016-08-12T13:21:21Z

pom.xml

@@ -63,6 +63,7 @@
    </pluginRepositories>
    <properties>
        <jenkins.version>1.642.3</jenkins.version>
+        <jenkins-test-harness.version>2.15-SNAPSHOT</jenkins-test-harness.version>


Downstream of jenkinsci/jenkins-test-harness#32. Actually passes without this dependency, but takes three times as long to collect the reference, due to lack of exponential escalation in the original version.

ghost · 2016-08-12T13:27:26Z

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

jglick · 2016-08-12T13:27:58Z

src/test/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecutionTest.java

    @Test public void loaderReleased() {
        story.addStep(new Statement() {
            @Override public void evaluate() throws Throwable {
+                Assume.assumeThat("TODO fails in Jenkins 2 for reasons TBD (no root references detected)", Jenkins.VERSION, Matchers.startsWith("1."));


Additional calls to fromRoots in the upstream PR were an attempt to diagnose this, so far without success.

jglick · 2016-08-12T13:56:17Z

Seem to be test failures, trying to figure out what they mean.

…tly be null in cleanUpHeap.

jglick · 2016-08-12T14:13:52Z

src/main/java/org/jenkinsci/plugins/workflow/cps/CpsThreadGroup.java

@@ -391,6 +393,10 @@ private void run() throws IOException {
        if (doneSomeWork) {
            saveProgram();
        }
+        if (ending) {
+            execution.cleanUpHeap();
+            scripts.clear();


Clearing scripts (introduced in #29) was also necessary, or even with the fix to the timing of SerializableClassRegistry.release, the test would still fail on Jenkins 1. The failure showed no root references, so I had to guess at the cause.

jglick · 2016-08-12T15:46:46Z

FindBugs failure, will deal with it.

In the meantime I found the cause of the Jenkins 2 failure at last:

groovy.lang.GroovyClassLoader$InnerLoader [Stack Local]
<loader> of WorkflowScript
klazz of org.codehaus.groovy.reflection.ClassInfo
value of org.codehaus.groovy.reflection.GroovyClassValuePreJava7$EntryWithValue
[0] of java.lang.Object[3]
[12] of java.lang.Object[32]
table of org.codehaus.groovy.reflection.GroovyClassValuePreJava7$GroovyClassValuePreJava7Segment
[14] of org.codehaus.groovy.util.AbstractConcurrentMapBase$Segment[16]
segments of org.codehaus.groovy.reflection.GroovyClassValuePreJava7$GroovyClassValuePreJava7Map
map of org.codehaus.groovy.reflection.GroovyClassValuePreJava7
globalClassValue of org.codehaus.groovy.reflection.ClassInfo
[6128] of java.lang.Object[20480]
elementData of java.util.Vector size = 13,066
classes of sun.misc.Launcher$AppClassLoader [Stack Local]
contextClassLoader of java.util.TimerThread [Thread, Stack Local]  "Timer-2" daemon tid=78 [TIMED_WAITING]

Unless you are running on IBM J9 VM, Groovy 2 switches to a broken GroovyClassValuePreJava7 which leaks ClassLoaders. (They claim to be affected by JDK-8136353 but I think the mistake is in Groovy, not Java.) The switch is controlled by a system property, but you would have to set it before Jenkins starts, so Pipeline cannot do so. So I am looking for a way to either clear all values from ClassInfo.globalClassValue (unfortunately Groovy’s nonstandard map implementations lack a clear method), or reset the whole collection to a version that works better (which is scary since it will affect every use of Groovy in Jenkins, pending JENKINS-29068), or somehow look up individual Class entries and clear them.

jglick · 2016-08-12T18:13:12Z

With a patched INSANE, I can now diagnose the problem properly in the test:

{groovy.lang.GroovyClassLoader$InnerLoader@368430db=private static final org.codehaus.groovy.reflection.GroovyClassValue org.codehaus.groovy.reflection.ClassInfo.globalClassValue->
org.codehaus.groovy.reflection.GroovyClassValuePreJava7@1a70d5df-map->
org.codehaus.groovy.reflection.GroovyClassValuePreJava7$GroovyClassValuePreJava7Map@54d79178-segments->
[Lorg.codehaus.groovy.util.AbstractConcurrentMapBase$Segment;@4a3f3c19-[12]->
org.codehaus.groovy.reflection.GroovyClassValuePreJava7$GroovyClassValuePreJava7Segment@3aa898c1-table->
[Ljava.lang.Object;@4bb9c2ea-[11]->
org.codehaus.groovy.reflection.GroovyClassValuePreJava7$EntryWithValue@274aa2f8-value->
org.codehaus.groovy.reflection.ClassInfo@3eacd88f-klazz->
java.lang.Class@4f65c3ca-<changed>->
groovy.lang.GroovyClassLoader$InnerLoader@368430db}

…mething else leaking in Jenkins 2.

jglick · 2016-08-12T21:01:02Z

Fixed that problem what I can tell, but now the test fails again on Jenkins 2 with no apparent GC root path. Heap analysis tools just tell me that the InnerLoader is being held as a “JNI global”, which does not say much. So I guess I leave it as is and hope this leak does not manifest itself in production.

jglick · 2016-08-12T21:08:17Z

src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java


-        // clean up heap
+    void cleanUpHeap() {


SerializableClassRegistry.release was getting called on the right loader, but too soon—before the final call to saveProgram, which would up adding the loader back to the registry. Not sure when this regressed—presumably at some point after the test was disabled.

abayer · 2016-08-12T21:48:44Z

🐝

POM updates

Added support for super.foo(...) calls

Restoring CpsFlowExecutionTest.loaderReleased on Jenkins 1, and corre…

1016fdf

…cting a memory leak regression introduced with the fix of JENKINS-36372 in 2.9.

jglick reviewed Aug 12, 2016
View reviewed changes

jglick added 2 commits August 12, 2016 10:03

Under certain circumstances, CpsFlowExecution.scriptClass can apparen…

558528c

…tly be null in cleanUpHeap.

Clearer code structure.

2579e06

jglick reviewed Aug 12, 2016
View reviewed changes

jglick added 3 commits August 12, 2016 15:28

FindBugs

b4bdaca

Noting what the diagnosed issue was.

d5d0da0

Dealt with the ClassInfo.globalClassValue leak, but there is still so…

52c3d47

…mething else leaking in Jenkins 2.

jglick reviewed Aug 12, 2016
View reviewed changes

jglick mentioned this pull request Aug 12, 2016

Set the Thread.contextClassLoader to the GroovyClassLoader #42

Merged

jglick merged commit 2f1580d into jenkinsci:master Aug 15, 2016

jglick deleted the CpsFlowExecutionTest.loaderReleased branch August 15, 2016 13:14

This was referenced Nov 2, 2016

More memory leaks #83

Merged

scripts might be null if loading old builds #89

Merged

svanoort pushed a commit that referenced this pull request Sep 28, 2017

Merge pull request #41 from jglick/pom

7f15c15

POM updates

dwnusbaum pushed a commit to dwnusbaum/workflow-cps-plugin that referenced this pull request Jun 1, 2020

Merge pull request jenkinsci#41 from cloudbees/super-call

836e220

Added support for super.foo(...) calls

jglick mentioned this pull request Jun 28, 2022

[JENKINS-68417] Revert LogRecord tricks jenkinsci/jenkins#6643

Merged

6 tasks

jglick mentioned this pull request Jun 28, 2022

[JENKINS-68417] Use Supplier overloads to avoid leaks from LogRecord.parameters #557

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Memory leak introduced with the fix of JENKINS-36372 #41

Memory leak introduced with the fix of JENKINS-36372 #41

jglick commented Aug 12, 2016

jglick Aug 12, 2016

ghost commented Aug 12, 2016

jglick Aug 12, 2016

jglick commented Aug 12, 2016

jglick Aug 12, 2016

jglick commented Aug 12, 2016

jglick commented Aug 12, 2016

jglick commented Aug 12, 2016

jglick Aug 12, 2016

abayer commented Aug 12, 2016

Memory leak introduced with the fix of JENKINS-36372 #41

Memory leak introduced with the fix of JENKINS-36372 #41

Conversation

jglick commented Aug 12, 2016

jglick Aug 12, 2016

Choose a reason for hiding this comment

ghost commented Aug 12, 2016

jglick Aug 12, 2016

Choose a reason for hiding this comment

jglick commented Aug 12, 2016

jglick Aug 12, 2016

Choose a reason for hiding this comment

jglick commented Aug 12, 2016

jglick commented Aug 12, 2016

jglick commented Aug 12, 2016

jglick Aug 12, 2016

Choose a reason for hiding this comment

abayer commented Aug 12, 2016