Use `ClassValue` for caching #370

jglick · 2022-06-16T18:54:52Z

Generally a good idea for avoiding memory leaks, though that may not be a real issue in Jenkins since the ClassLoaders involved are those of plugins, which would never be collected during the JVM session anyway.

Motivated by a CloudBees CI thread dump showing a large number of threads blocked like this:

"Handling HEAD /jenkins/instance-identity/ from … : Jetty (winstone)-…" …
    - waiting to lock <…> (a java.util.HashMap)
      owned by "Handling GET /jenkins/job/someItem/configure from … : Jetty (winstone)-… SomeStuff/configure.jelly" …
    at org.kohsuke.stapler.WebApp.getMetaClass(WebApp.java:227)
    at org.kohsuke.stapler.WebApp.getMetaClass(WebApp.java:244)
    at org.kohsuke.stapler.Stapler.tryInvoke(Stapler.java:762)
    at org.kohsuke.stapler.Stapler.invoke(Stapler.java:898)
    at org.kohsuke.stapler.Stapler.invoke(Stapler.java:694)
    at org.kohsuke.stapler.Stapler.service(Stapler.java:240)
    at …

all of them blocked by one HTTP handler thread

"Handling GET /jenkins/job/someItem/configure from … : Jetty (winstone)-… SomeStuff/configure.jelly" …
    - waiting to lock <…> (a hudson.ClassicPluginStrategy$AntClassLoader2)
      owned by "Computer.threadPoolForRemoting [#…]" …
    at java.lang.Class.forName0(Native Method)
    at …
    at java.lang.reflect.Method.getParameterAnnotations(Method.java:639)
    at org.kohsuke.stapler.Function$InstanceFunction.getParameterAnnotations(Function.java:423)
    at jenkins.security.stapler.DoActionFilter.keep(DoActionFilter.java:97)
    at jenkins.security.stapler.TypedFilter.isRoutableMethod(TypedFilter.java:131)
    at jenkins.security.stapler.TypedFilter.isSpecificClassStaplerRelevant(TypedFilter.java:98)
    at jenkins.security.stapler.TypedFilter.isStaplerRelevant(TypedFilter.java:65)
    at jenkins.security.stapler.TypedFilter.isStaplerRelevantCached(TypedFilter.java:57)
    at jenkins.security.stapler.TypedFilter.isClassAcceptable(TypedFilter.java:50)
    at jenkins.security.stapler.TypedFilter.keep(TypedFilter.java:267)
    at org.kohsuke.stapler.FunctionList.filter(FunctionList.java:48)
    at org.kohsuke.stapler.MetaClass.buildDispatchers(MetaClass.java:192)
    at org.kohsuke.stapler.MetaClass.<init>(MetaClass.java:98)
    at org.kohsuke.stapler.WebApp.getMetaClass(WebApp.java:230)
    at org.kohsuke.stapler.jelly.groovy.GroovyFacet.createRequestDispatcher(GroovyFacet.java:71)
    at org.kohsuke.stapler.RequestImpl.getView(RequestImpl.java:269)
    at org.kohsuke.stapler.RequestImpl.getView(RequestImpl.java:264)
    at hudson.model.Descriptor.getHelpFile(Descriptor.java:756)
    at hudson.model.Descriptor.getHelpFile(Descriptor.java:738)
    at …
    at org.apache.commons.jexl.ExpressionImpl.evaluate(ExpressionImpl.java:80)
    at hudson.ExpressionFactory2$JexlExpression.evaluate(ExpressionFactory2.java:74)
    at org.apache.commons.jelly.tags.core.CoreTagLibrary$3.run(CoreTagLibrary.java:134)
    at org.apache.commons.jelly.tags.core.CoreTagLibrary$1.run(CoreTagLibrary.java:98)
    at org.apache.commons.jelly.impl.ScriptBlock.run(ScriptBlock.java:95)
    at org.apache.commons.jelly.tags.core.CoreTagLibrary$2.run(CoreTagLibrary.java:105)
    at org.kohsuke.stapler.jelly.CallTagLibScript.run(CallTagLibScript.java:120)
    at org.apache.commons.jelly.impl.ScriptBlock.run(ScriptBlock.java:95)
    at org.apache.commons.jelly.tags.core.CoreTagLibrary$2.run(CoreTagLibrary.java:105)
    at org.kohsuke.stapler.jelly.JellyViewScript.run(JellyViewScript.java:95)
    at org.kohsuke.stapler.jelly.IncludeTag.doTag(IncludeTag.java:171)
    at org.apache.commons.jelly.impl.TagScript.run(TagScript.java:269)
    at …
    at org.kohsuke.stapler.Facet$1.dispatch(Facet.java:240)
    at org.kohsuke.stapler.Stapler.tryInvoke(Stapler.java:766)
    at org.kohsuke.stapler.Stapler.invoke(Stapler.java:898)
    at org.kohsuke.stapler.MetaClass$4.doDispatch(MetaClass.java:281)
    at org.kohsuke.stapler.NameBasedDispatcher.dispatch(NameBasedDispatcher.java:58)
    at org.kohsuke.stapler.Stapler.tryInvoke(Stapler.java:766)
    at org.kohsuke.stapler.Stapler.invoke(Stapler.java:898)
    at org.kohsuke.stapler.MetaClass$4.doDispatch(MetaClass.java:281)
    at org.kohsuke.stapler.NameBasedDispatcher.dispatch(NameBasedDispatcher.java:58)
    at org.kohsuke.stapler.Stapler.tryInvoke(Stapler.java:766)
    at org.kohsuke.stapler.Stapler.invoke(Stapler.java:898)
    at org.kohsuke.stapler.Stapler.invoke(Stapler.java:694)
    at org.kohsuke.stapler.Stapler.service(Stapler.java:240)
    at …

While a contributing factor was the Computer.threadPoolForRemoting thread (not shown), blocked in class loading predating paralellism (introduced I think in jenkinsci/jenkins#5698), this seems silly because some of the rendered threads are trying to load unrelated classes: WebApp.classMap was a bottleneck.

timja

👍 should probably run core tests first?

core/src/main/java/org/kohsuke/stapler/WebApp.java

core/src/main/java/org/kohsuke/stapler/TearOffSupport.java

basil · 2022-06-16T21:57:09Z

core/src/main/java/org/kohsuke/stapler/ClassLoaderValue.java

+/**
+ * Like {@link ClassValue} but for a whole {@link ClassLoader}.
+ */


Very creative!

basil · 2022-06-16T21:57:38Z

core/src/main/java/org/kohsuke/stapler/AnnotationHandler.java

@@ -90,7 +75,22 @@ static Object handle(StaplerRequest request, Annotation[] annotations, String pa
        return null; // probably we should report an error
    }

-    private static final ConcurrentMap<Class<? extends Annotation>,AnnotationHandler> HANDLERS = new ConcurrentHashMap<>();
+    private static final ClassValue<AnnotationHandler> HANDLERS = new ClassValue<AnnotationHandler>() {
+        @SuppressFBWarnings(value = "THROWS_METHOD_THROWS_RUNTIMEEXCEPTION", justification = "shut up")


My sentiment exactly.

core/src/main/java/org/kohsuke/stapler/MetaClassLoader.java

jglick · 2022-06-16T22:40:44Z

run core tests

Probably. I ran jenkins.security.stapler.**.*Test successfully.

…assValue

jglick · 2022-06-24T15:27:07Z

run core tests first

Or just YOLO-release; at worst we amend some problem or revert altogether and cut another release, if a DB PR turns up issues.

IIUC this cannot be tested directly against current Jenkins trunk because as of #373 it would fail pending jenkinsci/jenkins#6083.

basil · 2022-06-24T15:45:14Z

Well for a few more hours anyway. I am planning to merge jenkinsci/jenkins#6083 today, just need to write a PR description. You know how it goes, writing the code is always the easy part, writing the docs always the hard part.

basil · 2022-06-24T16:57:22Z

I just merged it so we should be good to go for testing this change now.

jglick · 2022-06-27T12:56:27Z

jenkinsci/jenkins#6693 passed.

basil · 2022-08-23T18:46:05Z

Or just YOLO-release; at worst we amend some problem or revert altogether and cut another release, if a DB PR turns up issues.

Issues have turned up indeed: jenkinsci/ec2-plugin#771

Use ClassValue for caching

0bc3391

jglick added the bug label Jun 16, 2022

Gratuitous diff

c741ed9

timja approved these changes Jun 16, 2022

View reviewed changes

basil approved these changes Jun 16, 2022

View reviewed changes

core/src/main/java/org/kohsuke/stapler/MetaClassLoader.java Outdated Show resolved Hide resolved

jglick added 4 commits June 16, 2022 18:41

Obsolete comment jenkinsci#370 (comment)

a2703af

setTearOff should throw an error jenkinsci#370 (comment)

0c572f5

Merge branch 'master' of https://github.com/jenkinsci/stapler into Cl…

93eb8e7

…assValue

Synchronize access to WebApp.classMap jenkinsci#370 (comment)

d704de2

This was referenced Jun 24, 2022

Check compatibility of proposed Stapler ClassValue changes jenkinsci/jenkins#6693

Closed

-Xdoclint:all,-missing jenkinsci/pom#270

Merged

jglick merged commit 93a11d2 into jenkinsci:master Jun 27, 2022

jglick deleted the ClassValue branch June 27, 2022 12:56

jglick mentioned this pull request Aug 25, 2022

Make ClassLoaderValue compatible with PowerMock #402

Merged

This was referenced Aug 25, 2022

Upgrade Stapler from 1730.v43d2c7b_11a_27 to 1733.v981fdd98007e jenkinsci/jenkins#7033

Merged

Upgrade Stapler from 1711.v5b_1b_03f0fcf2 to 1711.1713.vc400cfb_5597a_ jenkinsci/jenkins#7034

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `ClassValue` for caching #370

Use `ClassValue` for caching #370

jglick commented Jun 16, 2022

timja left a comment

basil Jun 16, 2022

basil Jun 16, 2022

jglick commented Jun 16, 2022

jglick commented Jun 24, 2022

basil commented Jun 24, 2022

basil commented Jun 24, 2022

jglick commented Jun 27, 2022

basil commented Aug 23, 2022

Use ClassValue for caching #370

Use ClassValue for caching #370

Conversation

jglick commented Jun 16, 2022

timja left a comment

Choose a reason for hiding this comment

basil Jun 16, 2022

Choose a reason for hiding this comment

basil Jun 16, 2022

Choose a reason for hiding this comment

jglick commented Jun 16, 2022

jglick commented Jun 24, 2022

basil commented Jun 24, 2022

basil commented Jun 24, 2022

jglick commented Jun 27, 2022

basil commented Aug 23, 2022

Use `ClassValue` for caching #370

Use `ClassValue` for caching #370