Merge pull request #634 from daspilker/JENKINS-30348

avoid class loader leak, really fixes JENKINS-30348
jenkinsci · Oct 5, 2015 · 785de7f · 785de7f
2 parents ea8254a + 76f8c6c
commit 785de7f
Show file tree

Hide file tree

Showing 4 changed files with 23 additions and 18 deletions.
diff --git a/docs/Home.md b/docs/Home.md
@@ -58,6 +58,7 @@ Have a look at the [Jenkins Job DSL Gradle example](https://github.com/sheehan/j
  * Added support for the [Rebuild Plugin](https://wiki.jenkins-ci.org/display/JENKINS/Rebuild+Plugin)
  * Added support for the [Global Variable String Parameter Plugin](https://wiki.jenkins-ci.org/display/JENKINS/Global+Variable+String+Parameter+Plugin)
  * Added support for the [Phabricator Differential Plugin](https://wiki.jenkins-ci.org/display/JENKINS/Phabricator+Differential+Plugin)
+ * The signature of `DslScriptLoader.runDslEngineForParent` has changed, see [Migration](Migration#migrating-to-139)
  * Removed implicit star import of `javaposse.jobdsl.dsl.ConfigFileType` in scripts, see
    [Migration](Migration#migrating-to-139)
  * Removed anything that has been deprecated in 1.31, see [Migration](Migration#migrating-to-131)

diff --git a/docs/Migration.md b/docs/Migration.md
@@ -71,6 +71,12 @@ javaposse.jobdsl.dsl.ConfigFileType.Custom
 javaposse.jobdsl.dsl.ConfigFileType.MavenSettings
 ```
 
+### DslScriptLoader
+
+The signature of `DslScriptLoader.runDslEngineForParent` has changed and the method is no longer public. The change was
+necessary to avoid a class loader leak and to fix ([JENKINS-30348](https://issues.jenkins-ci.org/browse/JENKINS-30348)).
+Use `DslScriptLoader.runDslEngine` instead.
+
 ## Migrating to 1.38
 
 ### Parameterized Trigger

diff --git a/job-dsl-core/src/main/groovy/javaposse/jobdsl/dsl/DslScriptLoader.java b/job-dsl-core/src/main/groovy/javaposse/jobdsl/dsl/DslScriptLoader.java
@@ -33,7 +33,7 @@ public class DslScriptLoader {
     private static final Logger LOGGER = Logger.getLogger(DslScriptLoader.class.getName());
     private static final Comparator<? super Item> ITEM_COMPARATOR = new ItemProcessingOrderComparator();
 
-    public static JobParent runDslEngineForParent(ScriptRequest scriptRequest, JobManagement jobManagement) throws IOException {
+    private static GeneratedItems runDslEngineForParent(ScriptRequest scriptRequest, JobManagement jobManagement) throws IOException {
         ClassLoader parentClassLoader = DslScriptLoader.class.getClassLoader();
         CompilerConfiguration config = createCompilerConfiguration(jobManagement);
 
@@ -85,7 +85,16 @@ public static JobParent runDslEngineForParent(ScriptRequest scriptRequest, JobMa
                 } catch (ScriptException e) {
                     throw new IOException("Unable to run script", e);
                 }
-                return jp;
+
+                GeneratedItems generatedItems = new GeneratedItems();
+                generatedItems.setConfigFiles(extractGeneratedConfigFiles(jp, scriptRequest.getIgnoreExisting()));
+                generatedItems.setJobs(extractGeneratedJobs(jp, scriptRequest.getIgnoreExisting()));
+                generatedItems.setViews(extractGeneratedViews(jp, scriptRequest.getIgnoreExisting()));
+                generatedItems.setUserContents(extractGeneratedUserContents(jp, scriptRequest.getIgnoreExisting()));
+
+                scheduleJobsToRun(jp.getQueueToBuild(), jobManagement);
+
+                return generatedItems;
             } finally {
                 if (engine.getGroovyClassLoader() instanceof Closeable) {
                     ((Closeable) engine.getGroovyClassLoader()).close();
@@ -123,18 +132,7 @@ static GeneratedItems runDslEngine(String scriptBody, JobManagement jobManagemen
     }
 
     public static GeneratedItems runDslEngine(ScriptRequest scriptRequest, JobManagement jobManagement) throws IOException {
-        JobParent jp = runDslEngineForParent(scriptRequest, jobManagement);
-        LOGGER.log(Level.FINE, String.format("Ran script and got back %s", jp));
-
-        GeneratedItems generatedItems = new GeneratedItems();
-        generatedItems.setConfigFiles(extractGeneratedConfigFiles(jp, scriptRequest.getIgnoreExisting()));
-        generatedItems.setJobs(extractGeneratedJobs(jp, scriptRequest.getIgnoreExisting()));
-        generatedItems.setViews(extractGeneratedViews(jp, scriptRequest.getIgnoreExisting()));
-        generatedItems.setUserContents(extractGeneratedUserContents(jp, scriptRequest.getIgnoreExisting()));
-
-        scheduleJobsToRun(jp.getQueueToBuild(), jobManagement);
-
-        return generatedItems;
+        return runDslEngineForParent(scriptRequest, jobManagement);
     }
 
     private static Set<GeneratedJob> extractGeneratedJobs(JobParent jp, boolean ignoreExisting) throws IOException {

diff --git a/job-dsl-core/src/test/groovy/javaposse/jobdsl/dsl/DslScriptLoaderSpec.groovy b/job-dsl-core/src/test/groovy/javaposse/jobdsl/dsl/DslScriptLoaderSpec.groovy
@@ -77,16 +77,16 @@ job('project-b') {
         ScriptRequest request = new ScriptRequest(null, scriptStr, resourcesDir, false)
 
         when:
-        JobParent jp = DslScriptLoader.runDslEngineForParent(request, jm)
+        GeneratedItems generatedItems = DslScriptLoader.runDslEngine(request, jm)
 
         then:
-        jp != null
-        def jobs = jp.referencedJobs
+        generatedItems != null
+        def jobs = generatedItems.jobs
         jobs.size() == 2
         def job = jobs.first()
         // If this one fails periodically, then it is because the referenced jobs are
         // Not in definition order, but rather in hash order. Hence, predictability.
-        job.name == 'project-a'
+        job.jobName == 'project-a'
 
         where:
         x << [1..25]