[FIXED JENKINS-10615]

Hold on to the lease until the very end. Previously, the lease was only held until the main build section is over, before publishers start running.
jenkinsci · Nov 8, 2013 · d183345 · jglick · Nov 11, 2013 · d183345
1 parent 61cf2df
commit d183345
Show file tree

Hide file tree

Showing 2 changed files with 51 additions and 44 deletions.
diff --git a/changelog.html b/changelog.html
@@ -72,6 +72,9 @@
   <li class=rfe>
     Add log handling line beginning with 'file://' as URL.
     (<a href="https://issues.jenkins-ci.org/browse/JENKINS-19866">issue 19866</a>)
+  <li class=bug>
+    Builds of a concurrently executable job might end up colliding on the same workspace.
+    (<a href="https://issues.jenkins-ci.org/browse/JENKINS-10615">issue 10615</a>)
 </ul>
 </div><!--=TRUNK-END=-->
 

diff --git a/core/src/main/java/hudson/model/AbstractBuild.java b/core/src/main/java/hudson/model/AbstractBuild.java
@@ -416,8 +416,8 @@ public Set<User> getCulprits() {
             if (upstreamCulprits) {
                 // If we have dependencies since the last successful build, add their authors to our list
                 if (getPreviousNotFailedBuild() != null) {
-                    Map <AbstractProject,AbstractBuild.DependencyChange> depmap = getDependencyChanges(getPreviousSuccessfulBuild());
-                    for (AbstractBuild.DependencyChange dep : depmap.values()) {
+                    Map <AbstractProject,DependencyChange> depmap = getDependencyChanges(getPreviousSuccessfulBuild());
+                    for (DependencyChange dep : depmap.values()) {
                         for (AbstractBuild<?,?> b : dep.getBuilds()) {
                             for (Entry entry : b.getChangeSet()) {
                                 r.add(entry.getAuthor());
@@ -495,6 +495,11 @@ deprecated class here.
          */
         protected BuildListener listener;
 
+        /**
+         * Lease of the workspace.
+         */
+        private Lease lease;
+
         /**
          * Returns the current {@link Node} on which we are building.
          */
@@ -542,58 +547,53 @@ public Result run(BuildListener listener) throws Exception {
             else
             	listener.getLogger().print(Messages.AbstractBuild_Building());
 
-            final Lease lease = decideWorkspace(node,Computer.currentComputer().getWorkspaceList());
+            lease = decideWorkspace(node, Computer.currentComputer().getWorkspaceList());
 
-            try {
-                workspace = lease.path.getRemote();
-                listener.getLogger().println(Messages.AbstractBuild_BuildingInWorkspace(workspace));
-                node.getFileSystemProvisioner().prepareWorkspace(AbstractBuild.this,lease.path,listener);
-
-                for (WorkspaceListener wl : WorkspaceListener.all()) {
-                    wl.beforeUse(AbstractBuild.this, lease.path, listener);
-                }
+            workspace = lease.path.getRemote();
+            listener.getLogger().println(Messages.AbstractBuild_BuildingInWorkspace(workspace));
+            node.getFileSystemProvisioner().prepareWorkspace(AbstractBuild.this,lease.path,listener);
 
-                getProject().getScmCheckoutStrategy().preCheckout(AbstractBuild.this, launcher, this.listener);
-                getProject().getScmCheckoutStrategy().checkout(this);
+            for (WorkspaceListener wl : WorkspaceListener.all()) {
+                wl.beforeUse(AbstractBuild.this, lease.path, listener);
+            }
 
-                if (!preBuild(listener,project.getProperties()))
-                    return Result.FAILURE;
+            getProject().getScmCheckoutStrategy().preCheckout(AbstractBuild.this, launcher, this.listener);
+            getProject().getScmCheckoutStrategy().checkout(this);
 
-                Result result = doRun(listener);
+            if (!preBuild(listener,project.getProperties()))
+                return Result.FAILURE;
 
-                Computer c = node.toComputer();
-                if (c==null || c.isOffline()) {
-                    // As can be seen in HUDSON-5073, when a build fails because of the slave connectivity problem,
-                    // error message doesn't point users to the slave. So let's do it here.
-                    listener.hyperlink("/computer/"+builtOn+"/log","Looks like the node went offline during the build. Check the slave log for the details.");
+            Result result = doRun(listener);
 
-                    if (c != null) {
-                        // grab the end of the log file. This might not work very well if the slave already
-                        // starts reconnecting. Fixing this requires a ring buffer in slave logs.
-                        AnnotatedLargeText<Computer> log = c.getLogText();
-                        StringWriter w = new StringWriter();
-                        log.writeHtmlTo(Math.max(0,c.getLogFile().length()-10240),w);
+            Computer c = node.toComputer();
+            if (c==null || c.isOffline()) {
+                // As can be seen in HUDSON-5073, when a build fails because of the slave connectivity problem,
+                // error message doesn't point users to the slave. So let's do it here.
+                listener.hyperlink("/computer/"+builtOn+"/log","Looks like the node went offline during the build. Check the slave log for the details.");
 
-                        listener.getLogger().print(ExpandableDetailsNote.encodeTo("details",w.toString()));
-                        listener.getLogger().println();
-                    }
+                if (c != null) {
+                    // grab the end of the log file. This might not work very well if the slave already
+                    // starts reconnecting. Fixing this requires a ring buffer in slave logs.
+                    AnnotatedLargeText<Computer> log = c.getLogText();
+                    StringWriter w = new StringWriter();
+                    log.writeHtmlTo(Math.max(0,c.getLogFile().length()-10240),w);
+
+                    listener.getLogger().print(ExpandableDetailsNote.encodeTo("details",w.toString()));
+                    listener.getLogger().println();
                 }
+            }
 
-                // kill run-away processes that are left
-                // use multiple environment variables so that people can escape this massacre by overriding an environment
-                // variable for some processes
-                launcher.kill(getCharacteristicEnvVars());
+            // kill run-away processes that are left
+            // use multiple environment variables so that people can escape this massacre by overriding an environment
+            // variable for some processes
+            launcher.kill(getCharacteristicEnvVars());
 
-                // this is ugly, but for historical reason, if non-null value is returned
-                // it should become the final result.
-                if (result==null)    result = getResult();
-                if (result==null)    result = Result.SUCCESS;
+            // this is ugly, but for historical reason, if non-null value is returned
+            // it should become the final result.
+            if (result==null)    result = getResult();
+            if (result==null)    result = Result.SUCCESS;
 
-                return result;
-            } finally {
-                lease.release();
-                this.listener = null;
-            }
+            return result;
         }
 
         /**
@@ -649,7 +649,7 @@ public void defaultCheckout() throws IOException, InterruptedException {
                 build.scm = NullChangeLogParser.INSTANCE;
 
                 try {
-                    if (project.checkout(build,launcher,listener,new File(build.getRootDir(),"changelog.xml"))) {
+                    if (project.checkout(build, launcher,listener,new File(build.getRootDir(),"changelog.xml"))) {
                         // check out succeeded
                         SCM scm = project.getScm();
 
@@ -715,6 +715,10 @@ public final void post(BuildListener listener) throws Exception {
         }
 
         public void cleanUp(BuildListener listener) throws Exception {
+            if (lease!=null) {
+                lease.release();
+                lease = null;
+            }
             BuildTrigger.execute(AbstractBuild.this, listener);
             buildEnvironments = null;
         }