[FIXED JENKINS-23098]

Reference: ZD-19531 Looking at [4], one notices that three threads are in an effective dead lock state around synchronizeOnMark. I extracted relevant part into [5]. Thread #1661 is trying to report a discovered mark, but blocking [1]. Thread #1665 is inside synchronizeOnMark, on markCountLock.wait() [2]. Thread #1667 is stuck on Future.get() and hasn't returned [3], which holds the lock that blocks [1] from unblocking [2]. The root problem is that synchronizeOnMark method is never meant to be concurrently executed. But given the way the lock is used, if one thread gets to wait(), it's possible that another thread would come along and go into this function. In this change, I'm preventing that by introducing another lock to serialize the execution of the entire synchronizeOnMark() call. I'm not using the "this" object for locking because it's already used for another purpose (see the lock() method) I'm not yet clear on why the synchronizeOnMark() method is called concurrently to begin with. The interaction with the -T option of Maven is suspected. [1] https://gist.github.com/kohsuke/374c22e737a77c9b0421#file-gistfile1-txt-L2 [2] https://gist.github.com/kohsuke/374c22e737a77c9b0421#file-gistfile1-txt-L34 [3] https://gist.github.com/kohsuke/374c22e737a77c9b0421#file-gistfile1-txt-L71 [4] https://gist.github.com/abayer/7ff4de807c6373eec40d [5] https://gist.github.com/kohsuke/374c22e737a77c9b0421
jenkinsci · Jul 9, 2014 · b145d59 · b145d59
1 parent 11cfd5e
commit b145d59
Showing 1 changed file with 22 additions and 15 deletions.
diff --git a/src/main/java/hudson/maven/SplittableBuildListener.java b/src/main/java/hudson/maven/SplittableBuildListener.java
@@ -79,6 +79,7 @@ final class SplittableBuildListener extends AbstractTaskListener implements Buil
 
     private int markCount = 0;
     private final Object markCountLock = new Object();
+    private final Object synchronizeLock = new Object();
 
     public SplittableBuildListener(BuildListener core) {
         this.core = core;
@@ -141,21 +142,27 @@ protected void onMarkFound() {
      * we will not receive any extra bytes after the marker string.
      */
     public void synchronizeOnMark(Channel ch) throws IOException, InterruptedException {
-        synchronized (markCountLock) {
-            int start = markCount;
-
-            // have the remote send us a mark
-            Future<Void> f = ch.callAsync(new SendMark());
-
-            // and block until we receive a mark
-            while (markCount==start && !f.isDone())
-                markCountLock.wait(10*1000);
-
-            // if SendMark fails, then we fail
-            try {
-                f.get();
-            } catch (ExecutionException e) {
-                throw new IOException2(e);
+        // this lock ensures that multiple concurrent executions of synchronizeOnMark get serialized
+        // and happens one at a time
+        synchronized (synchronizeLock) {
+
+            // this lock is for wait/notify idiom
+            synchronized (markCountLock) {
+                int start = markCount;
+
+                // have the remote send us a mark
+                Future<Void> f = ch.callAsync(new SendMark());
+
+                // and block until we receive a mark
+                while (markCount == start && !f.isDone())
+                    markCountLock.wait(10 * 1000);
+
+                // if SendMark fails, then we fail
+                try {
+                    f.get();
+                } catch (ExecutionException e) {
+                    throw new IOException2(e);
+                }
             }
         }
     }