From ddb0a472ad44fcbce31fb565d477901575e3c581 Mon Sep 17 00:00:00 2001 From: Stephen Connolly Date: Mon, 8 Jun 2015 14:38:49 +0100 Subject: [PATCH] [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 --- core/src/main/java/hudson/model/Executor.java | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/hudson/model/Executor.java b/core/src/main/java/hudson/model/Executor.java index 08864961eb..df9fb9714e 100644 --- a/core/src/main/java/hudson/model/Executor.java +++ b/core/src/main/java/hudson/model/Executor.java @@ -205,7 +205,14 @@ public class Executor extends Thread implements ModelObject { } 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 = @@ -213,7 +220,7 @@ public class Executor extends Thread implements ModelObject { return r; } finally { - lock.readLock().unlock(); + lock.writeLock().unlock(); } } -- GitLab