[FIXED JENKINS-22560] Avoid deadlock by making AbstractBuild.runMixIn…

… final. (Forgot that Run’s are unmarshalled in place after a special constructor is called, so there is no need for readResolve or other tricks.) Also calling RunListener.onDeleted outside of the Run lock to avoid problems with things like ChangeLogHistoryRunListener.
jenkinsci · Apr 10, 2014 · a5d848b · a5d848b
1 parent 8a17462
commit a5d848b
Show file tree

Hide file tree

Showing 4 changed files with 18 additions and 16 deletions.
diff --git a/changelog.html b/changelog.html
@@ -58,6 +58,9 @@
   <li class=bug>
     Fixed NPE from view new job name autocompletion since 1.553.
     (<a href="https://issues.jenkins-ci.org/browse/JENKINS-22142">issue 22142</a>)
+  <li class='major bug'>
+    Deadlocks in concurrent builds under some conditions since 1.556.
+    (<a href="https://issues.jenkins-ci.org/browse/JENKINS-22560">issue 22560</a>)
   <li class=rfe>
     JNLP slaves are now handled through NIO-based remoting channels for better scalability.
 </ul>

diff --git a/core/src/main/java/hudson/model/AbstractBuild.java b/core/src/main/java/hudson/model/AbstractBuild.java
@@ -71,7 +71,6 @@
 import java.io.InterruptedIOException;
 import java.io.StringWriter;
 import java.lang.ref.WeakReference;
-import java.text.MessageFormat;
 import java.util.AbstractSet;
 import java.util.ArrayList;
 import java.util.Calendar;
@@ -156,7 +155,11 @@ public abstract class AbstractBuild<P extends AbstractProject<P,R>,R extends Abs
      */
     protected transient List<Environment> buildEnvironments;
 
-    private transient LazyBuildMixIn.RunMixIn<P,R> runMixIn;
+    private transient final LazyBuildMixIn.RunMixIn<P,R> runMixIn = new LazyBuildMixIn.RunMixIn<P,R>() {
+        @Override protected R asRun() {
+            return _this();
+        }
+    };
 
     protected AbstractBuild(P job) throws IOException {
         super(job);
@@ -174,14 +177,7 @@ public final P getProject() {
         return getParent();
     }
 
-    @Override public final synchronized LazyBuildMixIn.RunMixIn<P,R> getRunMixIn() {
-        if (runMixIn == null) {
-            runMixIn = new LazyBuildMixIn.RunMixIn<P,R>() {
-                @Override protected R asRun() {
-                    return _this();
-                }
-            };
-        }
+    @Override public final LazyBuildMixIn.RunMixIn<P,R> getRunMixIn() {
         return runMixIn;
     }
 

diff --git a/core/src/main/java/hudson/model/Run.java b/core/src/main/java/hudson/model/Run.java
@@ -1445,17 +1445,19 @@ public synchronized void deleteArtifacts() throws IOException {
      * @throws IOException
      *      if we fail to delete.
      */
-    public synchronized void delete() throws IOException {
+    public void delete() throws IOException {
+        File rootDir = getRootDir();
+        if (!rootDir.isDirectory()) {
+            throw new IOException(this + ": " + rootDir + " looks to have already been deleted");
+        }
+
         RunListener.fireDeleted(this);
 
+        synchronized (this) { // avoid holding a lock while calling plugin impls of onDeleted
         // if we have a symlink, delete it, too
         File link = new File(project.getBuildDir(), String.valueOf(getNumber()));
         link.delete();
 
-        File rootDir = getRootDir();
-        if (!rootDir.isDirectory()) {
-            throw new IOException(this + ": " + rootDir + " looks to have already been deleted");
-        }
         File tmp = new File(rootDir.getParentFile(),'.'+rootDir.getName());
 
         if (tmp.exists()) {
@@ -1474,6 +1476,7 @@ public synchronized void delete() throws IOException {
         LOGGER.log(FINE, "{0}: {1} successfully deleted", new Object[] {this, rootDir});
 
         removeRunFromParent();
+        }
     }
 
     @SuppressWarnings("unchecked") // seems this is too clever for Java's type system?

diff --git a/core/src/main/java/jenkins/model/lazy/LazyBuildMixIn.java b/core/src/main/java/jenkins/model/lazy/LazyBuildMixIn.java
@@ -280,7 +280,7 @@ public interface LazyLoadingRun<JobT extends Job<JobT,RunT> & Queue.Task & LazyB
 
     /**
      * Accompanying helper for the run type.
-     * Stateful but should be held in a {@code transient} field.
+     * Stateful but should be held in a {@code transient final} field.
      */
     public static abstract class RunMixIn<JobT extends Job<JobT,RunT> & Queue.Task & LazyBuildMixIn.LazyLoadingJob<JobT,RunT>, RunT extends Run<JobT,RunT> & LazyLoadingRun<JobT,RunT>> {