Skip to content

[JENKINS-63766] Work around JDK-8231454 #543

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
May 24, 2022
Merged

[JENKINS-63766] Work around JDK-8231454 #543

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
908999b
[JENKINS-63766] Work around JDK-8231454
basil May 24, 2022
8759418
JDK 16 contains the fix
basil May 24, 2022
2420785
Adapt to JDK-8277072
basil May 24, 2022
5934d66
Update src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecut…
basil May 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 50 additions & 7 deletions src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
import hudson.model.Action;
import hudson.model.Result;
import hudson.util.Iterators;
import io.jenkins.lib.versionnumber.JavaSpecificationVersion;
import jenkins.model.CauseOfInterruption;
import jenkins.model.Jenkins;
import org.jboss.marshalling.Unmarshaller;
Expand Down Expand Up @@ -1298,6 +1299,7 @@ private static void cleanUpLoader(ClassLoader loader, Set<ClassLoader> encounter
if (encounteredClasses.add(clazz)) {
LOGGER.log(Level.FINER, "found {0}", clazz.getName());
Introspector.flushFromCaches(clazz);
cleanUpClassInfoCache(clazz);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any idea if the same issue applies to sandboxed non-Pipeline Groovy scripts as well? This code duplicates https://github.com/jenkinsci/script-security-plugin/blob/35f6a0b8207ed3a32a85f27c1312da6cd738eeaa/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScript.java#L205-L214.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any idea if the same issue applies to sandboxed non-Pipeline Groovy scripts as well?

No idea.

This code duplicates https://github.com/jenkinsci/script-security-plugin/blob/35f6a0b8207ed3a32a85f27c1312da6cd738eeaa/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScript.java#L205-L214.

jenkinsci/script-security-plugin#420

cleanUpGlobalClassSet(clazz);
cleanUpClassHelperCache(clazz);
cleanUpObjectStreamClassCaches(clazz);
Expand Down Expand Up @@ -1358,6 +1360,44 @@ private static void cleanUpGlobalClassValue(@NonNull ClassLoader loader) throws
}
}

private static void cleanUpClassInfoCache(Class<?> clazz) {
JavaSpecificationVersion current = JavaSpecificationVersion.forCurrentJVM();
if (current.isNewerThan(new JavaSpecificationVersion("1.8"))
&& current.isOlderThan(new JavaSpecificationVersion("16"))) {
Comment on lines +1365 to +1366
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in JENKINS-63766, this bug was introduced in Java 9 and was fixed in Java 16. The only version of Java in this range that we expect users to be using realistically is Java 11, but cover the whole range just to be safe.

try {
// TODO Work around JDK-8231454.
Class<?> classInfoC = Class.forName("com.sun.beans.introspect.ClassInfo");
Field cacheF = classInfoC.getDeclaredField("CACHE");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As an aside, does anyone happen to know why this is not just using ClassValue, which is supposed to handle exactly this use case?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about this yesterday, too. Ask the OpenJDK people?

try {
cacheF.setAccessible(true);
} catch (RuntimeException e) { // TOOD Java 9+ InaccessibleObjectException
/*
* Not running with "--add-opens java.desktop/com.sun.beans.introspect=ALL-UNNAMED".
* Until core adds this to its --add-opens configuration, and until that core
* change is widely adopted, avoid unnecessary log spam and return early.
*/
if (LOGGER.isLoggable(Level.FINER)) {
LOGGER.log(Level.FINER, "Failed to clean up " + clazz.getName() + " from ClassInfo#CACHE. A metaspace leak may have occurred.", e);
}
return;
}
Object cache = cacheF.get(null);
Class<?> cacheC = Class.forName("com.sun.beans.util.Cache");
if (LOGGER.isLoggable(Level.FINER)) {
LOGGER.log(Level.FINER, "Cleaning up " + clazz.getName() + " from ClassInfo#CACHE.");
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was tempted to print whether the class was already in the cache or not, as is done elsewhere in this file, but Cache#get always adds an entry if none exists, and there seems to be no Cache#contains method, so logging the result of Cache#get would likely do more harm than good, so I avoided it.

}
Method removeM = cacheC.getMethod("remove", Object.class);
removeM.invoke(cache, clazz);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cache#get and Cache#remove synchronize internally on a queue object, so there is no synchronization needed here.

} catch (ReflectiveOperationException e) {
/*
* Should never happen, but if it does, ensure the failure is isolated to this
* method and does not prevent other cleanup logic from executing.
*/
LOGGER.log(Level.WARNING, "Failed to clean up " + clazz.getName() + " from ClassInfo#CACHE. A metaspace leak may have occurred.", e);
}
}
}

private static void cleanUpGlobalClassSet(@NonNull Class<?> clazz) throws Exception {
Class<?> classInfoC = Class.forName("org.codehaus.groovy.reflection.ClassInfo"); // or just ClassInfo.class, but unclear whether this will always be there
Field globalClassSetF = classInfoC.getDeclaredField("globalClassSet");
Expand Down Expand Up @@ -1405,13 +1445,16 @@ private static void cleanUpObjectStreamClassCaches(@NonNull Class<?> clazz) thro
for (String cacheFName : new String[] {"localDescs", "reflectors"}) {
Field cacheF = cachesC.getDeclaredField(cacheFName);
cacheF.setAccessible(true);
ConcurrentMap<Reference<Class<?>>, ?> cache = (ConcurrentMap) cacheF.get(null);
Iterator<? extends Entry<Reference<Class<?>>, ?>> iterator = cache.entrySet().iterator();
while (iterator.hasNext()) {
if (iterator.next().getKey().get() == clazz) {
iterator.remove();
LOGGER.log(Level.FINER, "cleaning up {0} from ObjectStreamClass.Caches.{1}", new Object[] {clazz.getName(), cacheFName});
break;
Object cache = cacheF.get(null);
if (cache instanceof ConcurrentMap) {
// Prior to JDK-8277072
Iterator<? extends Entry<Reference<Class<?>>, ?>> iterator = ((ConcurrentMap) cache).entrySet().iterator();
while (iterator.hasNext()) {
if (iterator.next().getKey().get() == clazz) {
iterator.remove();
LOGGER.log(Level.FINER, "cleaning up {0} from ObjectStreamClass.Caches.{1}", new Object[]{clazz.getName(), cacheFName});
break;
}
Comment on lines +1448 to +1457
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a separate fix for a related bug I noticed while testing under latest OpenJDK 11.0.16 (in particular, while testing with openjdk/jdk11u-dev#1103). I got the following new error:

java.lang.ClassCastException: class java.io.ObjectStreamClass$Caches$1 cannot be cast to class java.util.concurrent.ConcurrentMap (java.io.ObjectStreamClass$Caches$1 and java.util.concurrent.ConcurrentMap are in module java.base of loader 'bootstrap')
        at org.jenkinsci.plugins.workflow.cps.CpsFlowExecution.cleanUpObjectStreamClassCaches(CpsFlowExecution.java:1448)
        at org.jenkinsci.plugins.workflow.cps.CpsFlowExecution.cleanUpLoader(CpsFlowExecution.java:1305)
        at org.jenkinsci.plugins.workflow.cps.CpsFlowExecution.cleanUpHeap(CpsFlowExecution.java:1270)
        at org.jenkinsci.plugins.workflow.cps.CpsThreadGroup.run(CpsThreadGroup.java:464)
        at org.jenkinsci.plugins.workflow.cps.CpsThreadGroup.access$400(CpsThreadGroup.java:95)
        at org.jenkinsci.plugins.workflow.cps.CpsThreadGroup$2.call(CpsThreadGroup.java:330)
        at org.jenkinsci.plugins.workflow.cps.CpsThreadGroup$2.call(CpsThreadGroup.java:294)
        at org.jenkinsci.plugins.workflow.cps.CpsVmExecutorService$2.call(CpsVmExecutorService.java:67)
        at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
        at hudson.remoting.SingleLaneExecutorService$1.run(SingleLaneExecutorService.java:139)
        at jenkins.util.ContextResettingExecutorService$1.run(ContextResettingExecutorService.java:28)
        at jenkins.security.ImpersonatingExecutorService$1.run(ImpersonatingExecutorService.java:68)
        at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515)
        at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
        at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
        at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
        at java.base/java.lang.Thread.run(Thread.java:829)

Looking into this further, it appears the type of this field was changed in JDK-8277072, which was backported to OpenJDK 11.0.16, which I happened to be testing. Skipping over this logic got the error to go away, and I confirmed that the class was still unloaded when running with JDK-8277072 and without entering this if statement, so I conclude that JDK-8277072 made this if statement unnecessary.

}
}
}
Expand Down