Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JENKINS-42189] Fix ClassInfo cleanup in Groovy 2.4.8+ #109

Merged
merged 3 commits into from Feb 23, 2017

Conversation

Projects
None yet
4 participants
@jglick
Copy link
Member

jglick commented Feb 23, 2017

JENKINS-42189

Directly solves the soft leak, and probably also solves the deadlock (without waiting for apache/groovy#489 in a 2.4.9 update) by simply skipping the affected code path.

Amends #83. Required by apache/groovy#219 and apache/groovy#427.

@reviewbybees

[FIXED JENKINS-42189] In Groovy 2.4.8+, ClassInfo.klazz is replaced b…
…y .classRef.

cleanUpGlobalClassValue needs to walk through those (not just Sentinel.class over and over);
and cleanUpGlobalClassSet is no longer relevant since classRef is weak.
@abayer

abayer approved these changes Feb 23, 2017

try {
Field classRefF = classInfoC.getDeclaredField("classRef");
return; // 2.4.8+, nothing to do here (classRef is weak anyway)
} catch (NoSuchFieldException x2) {} // 2.4.7-

This comment has been minimized.

@oleg-nenashev

This comment has been minimized.

@oleg-nenashev

This comment has been minimized.

@jglick

jglick Feb 23, 2017

Author Member

No, Groovy 2.4.7.

@oleg-nenashev
Copy link
Member

oleg-nenashev left a comment

🐝 if it got enough manual testing (I'd guess so)

@jglick

This comment has been minimized.

Copy link
Member Author

jglick commented Feb 23, 2017

if it got enough manual testing

It got automated testing, in CpsFlowExecutionTest.loaderReleased. Prior to the fix, this test does pass against jenkins.version=2.47, but only after forcing collection of soft references. (Currently assertGC tolerates soft leaks. I could perhaps amend it to fail if regular GC cycles do not suffice to clear everything.)

@@ -996,19 +997,31 @@ private static void cleanUpGlobalClassValue(@Nonnull ClassLoader loader) throws
Object map = mapF.get(globalClassValue);
Class<?> groovyClassValuePreJava7Map = Class.forName("org.codehaus.groovy.reflection.GroovyClassValuePreJava7$GroovyClassValuePreJava7Map");
Collection entries = (Collection) groovyClassValuePreJava7Map.getMethod("values").invoke(map);
Field klazzF = classInfoC.getDeclaredField("klazz");
klazzF.setAccessible(true);
Method removeM = groovyClassValuePreJava7Map.getMethod("remove", Object.class);

This comment has been minimized.

@svanoort

svanoort Feb 23, 2017

Member

Suggest using MethodHandle over raw Method -- as fast as Method in the worst case, an order of magnitude faster in the best: http://andrewtill.blogspot.com/2011/08/using-method-handles.html

This comment has been minimized.

@jglick

jglick Feb 23, 2017

Author Member

Could be a nice refinement to cleanup code later. In this case the number of accesses is fairly small so the cost is probably insignificant compared to other aspects of the build. Anyway out of scope for this PR.

This comment has been minimized.

@svanoort

svanoort Feb 23, 2017

Member

Unless I misunderstand, it's potentially running for every class with a reference held to it -- I do not think that qualifies as a "small" number

@@ -996,19 +997,31 @@ private static void cleanUpGlobalClassValue(@Nonnull ClassLoader loader) throws
Object map = mapF.get(globalClassValue);
Class<?> groovyClassValuePreJava7Map = Class.forName("org.codehaus.groovy.reflection.GroovyClassValuePreJava7$GroovyClassValuePreJava7Map");
Collection entries = (Collection) groovyClassValuePreJava7Map.getMethod("values").invoke(map);
Field klazzF = classInfoC.getDeclaredField("klazz");
klazzF.setAccessible(true);
Method removeM = groovyClassValuePreJava7Map.getMethod("remove", Object.class);
Class<?> entryC = Class.forName("org.codehaus.groovy.util.AbstractConcurrentMapBase$Entry");
Method getValueM = entryC.getMethod("getValue");

This comment has been minimized.

@svanoort

svanoort Feb 23, 2017

Member

Suggest using MethodHandle over Method here.

@svanoort
Copy link
Member

svanoort left a comment

🐝

@jglick jglick merged commit dc00a0e into jenkinsci:master Feb 23, 2017

1 check passed

Jenkins This pull request looks good
Details

@jglick jglick deleted the jglick:groovy-2.4.8-JENKINS-42189 branch Feb 23, 2017

jglick added a commit that referenced this pull request Feb 23, 2017

jglick added a commit to jglick/workflow-cps-plugin that referenced this pull request Feb 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.