[FIXED JENKINS-28690] Deadlock in hudson.model.Executor

- Rather fun one here. The Lock code relies on assuming that Thread.interrupted() is clear on entry - If it then sees Thread.interrupted() set, it will interrupt the current thread in order to set the flag again. - Executor is a thread that does funky things with an overridden interrupt method - Executor.abortResult() is used to track a build be interrupted or aborted in some other way - As a result the abortResult can cause a deadlockif there is a genuine interruption - This fix clears the interrupt flag in abortResult() and uses the write lock in order to ensure: - The same lock as used in interrupt() is helf - The interrupt flag is clear - Clearing the interrupt flag should be safe as the only time it is called is immediately after an interruption and the resulting exception is caught and rethrown/logged anyway
jenkinsci · Jun 8, 2015 · ddb0a47 · ddb0a47
1 parent 998b2f2
commit ddb0a47
Showing 1 changed file with 9 additions and 2 deletions.
diff --git a/core/src/main/java/hudson/model/Executor.java b/core/src/main/java/hudson/model/Executor.java
@@ -205,15 +205,22 @@ private void interrupt(Result result, boolean forShutdown, CauseOfInterruption..
     }
 
     public Result abortResult() {
-        lock.readLock().lock();
+        // this method is almost always called as a result of the current thread being interrupted
+        // as a result we need to clean the interrupt flag so that the lock's lock method doesn't
+        // get confused and think it was interrupted while awaiting the lock
+        Thread.interrupted(); 
+        // we need to use a write lock as we may be repeatedly interrupted while processing and
+        // we need the same lock as used in void interrupt(Result,boolean,CauseOfInterruption...)
+        // JENKINS-28690
+        lock.writeLock().lock();
         try {
             Result r = interruptStatus;
             if (r == null) r =
                     Result.ABORTED; // this is when we programmatically throw InterruptedException instead of calling the interrupt method.
 
             return r;
         } finally {
-            lock.readLock().unlock();
+            lock.writeLock().unlock();
         }
     }