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-68417] Use Supplier overloads to avoid leaks from LogRecord.parameters #557

Merged
merged 1 commit into from
Jun 30, 2022

Conversation

jglick
Copy link
Member

@jglick jglick commented Jun 28, 2022

Seems the tricks explored in jenkinsci/jenkins#6643 & jenkinsci/support-core-plugin#376 are not necessary if code logging at fine levels takes care to use Supplier-based overloads (or otherwise convert record parameters to String, as in #41) where the parameters might refer to expensive object trees.

Sketch of iterative testing
mvnd -Pquick-build -f jenkins -pl core clean install
mvnd -Pquick-build -f support-core-plugin install
mvnd -Pquick-build -f workflow-cps-plugin install
JAVA_HOME=/usr/lib/jvm/java-17-openjdk-amd64 mvn -f pipeline-groovy-lib-plugin test -Dtest=LibraryMemoryTest

given

diff --git pom.xml pom.xml
index 9ee4e40..66b4259 100644
--- pom.xml
+++ pom.xml
@@ -63,15 +63,15 @@
     </pluginRepositories>
     <properties>
         <changelist>999999-SNAPSHOT</changelist>
-        <jenkins.version>2.289.1</jenkins.version>
+        <jenkins.version>2.358-SNAPSHOT</jenkins.version> <!-- TODO https://github.com/jenkinsci/jenkins/pull/6643 -->
         <gitHubRepo>jenkinsci/${project.artifactId}-plugin</gitHubRepo>
     </properties>
     <dependencyManagement>
         <dependencies>
             <dependency>
                 <groupId>io.jenkins.tools.bom</groupId>
-                <artifactId>bom-2.289.x</artifactId>
-                <version>1008.vb9e22885c9cf</version>
+                <artifactId>bom-2.332.x</artifactId>
+                <version>1451.v15f1fdb_772a_f</version>
                 <scope>import</scope>
                 <type>pom</type>
             </dependency>
@@ -101,6 +101,7 @@
         <dependency>
             <groupId>org.jenkins-ci.plugins.workflow</groupId>
             <artifactId>workflow-cps</artifactId>
+            <version>999999-SNAPSHOT</version> <!-- https://github.com/jenkinsci/workflow-cps-plugin/pull/557 -->
         </dependency>
         <dependency>
             <groupId>org.jenkins-ci.plugins.workflow</groupId>
@@ -203,5 +204,11 @@
             <version>1.10.1</version>
             <scope>test</scope>
         </dependency>
+        <dependency>
+            <groupId>org.jenkins-ci.plugins</groupId>
+            <artifactId>support-core</artifactId>
+            <version>999999-SNAPSHOT</version> <!-- TODO https://github.com/jenkinsci/support-core-plugin/pull/376 -->
+            <scope>test</scope>
+        </dependency>
     </dependencies>
 </project>
diff --git src/test/java/org/jenkinsci/plugins/workflow/libs/LibraryMemoryTest.java src/test/java/org/jenkinsci/plugins/workflow/libs/LibraryMemoryTest.java
index 59c9d05..a13feb6 100644
--- src/test/java/org/jenkinsci/plugins/workflow/libs/LibraryMemoryTest.java
+++ src/test/java/org/jenkinsci/plugins/workflow/libs/LibraryMemoryTest.java
@@ -26,6 +26,9 @@ package org.jenkinsci.plugins.workflow.libs;
 
 import groovy.lang.MetaClass;
 import hudson.PluginManager;
+import hudson.WebAppMain;
+import hudson.logging.LogRecorder;
+import hudson.logging.LogRecorderManager;
 import java.lang.ref.WeakReference;
 import java.lang.reflect.Method;
 import java.util.ArrayList;
@@ -53,7 +56,7 @@ public class LibraryMemoryTest {
     @ClassRule public static BuildWatcher buildWatcher = new BuildWatcher();
     @Rule public JenkinsRule r = new JenkinsRule();
     @Rule public GitSampleRepoRule sampleRepo = new GitSampleRepoRule();
-    @Rule public LoggerRule logging = new LoggerRule().record(CpsFlowExecution.class, Level.FINER);
+    @Rule public LoggerRule logging = new LoggerRule().record("org", Level.FINER);
 
     private static final List<WeakReference<ClassLoader>> LOADERS = new ArrayList<>();
     public static void register(Object o) {
@@ -65,6 +68,16 @@ public class LibraryMemoryTest {
     }
     @Issue("JENKINS-50223")
     @Test public void loaderReleased() throws Exception {
+        Method installLogger = WebAppMain.class.getDeclaredMethod("installLogger");
+        installLogger.setAccessible(true);
+        installLogger.invoke(new WebAppMain());
+        LogRecorderManager mgr = r.jenkins.getLog();
+        LogRecorder recorder = new LogRecorder("logging");
+        mgr.getRecorders().add(recorder);
+        LogRecorder.Target t = new LogRecorder.Target("org", Level.FINE);
+        recorder.getLoggers().add(t);
+        recorder.save();
+        t.enable();
         sampleRepo.init();
         sampleRepo.write("src/p/C.groovy", "package p; class C {}");
         sampleRepo.write("vars/leak.groovy", "def call() {def c = node {new p.C()}; [this, c].each {" + LibraryMemoryTest.class.getName() + ".register(it)}}");

@jglick jglick added the bug label Jun 28, 2022
@jglick jglick requested a review from dwnusbaum June 28, 2022 20:50
@jglick jglick merged commit aee50bd into jenkinsci:master Jun 30, 2022
@jglick jglick deleted the LogRecord-JENKINS-68417 branch June 30, 2022 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants