JENKINS-51123: Fix deadlock in CpsFlowExecution

A deadlock can arise if multiple threads tries to access storage simultaneously while one of them doing it from a synchronized method. The cause is that timing operation also tries to get the instance lock from inside the storage r/w lock. Solution is to change the order of locking by having the timing object wrapping the call to the r/w storage lock instead of the opposite.
jenkinsci · May 4, 2018 · 4bc2273 · 4bc2273
1 parent 2358bcd
commit 4bc2273
Showing 1 changed file with 50 additions and 34 deletions.
diff --git a/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java b/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java
@@ -477,7 +477,7 @@ public GroovyShell getTrustedShell() {
     public FlowNodeStorage getStorage() {
         return storage;
     }
-    
+
     public String getScript() {
         return script;
     }
@@ -1756,86 +1756,102 @@ private <T> T readChild(HierarchicalStreamReader r, UnmarshallingContext context
 
     class TimingFlowNodeStorage extends FlowNodeStorage {
         private final FlowNodeStorage delegate;
-        ReadWriteLock readWriteLock = new ReentrantReadWriteLock();
+        private final ReadWriteLock readWriteLock = new ReentrantReadWriteLock();
 
         TimingFlowNodeStorage(FlowNodeStorage delegate) {
             this.delegate = delegate;
         }
 
         @Override
         public FlowNode getNode(String string) throws IOException {
-            readWriteLock.readLock().lock();
             try (Timing t = time(TimingKind.flowNode)) {
-                return delegate.getNode(string);
-            } finally {
-                readWriteLock.readLock().unlock();
+                readWriteLock.readLock().lock();
+                try {
+                    return delegate.getNode(string);
+                } finally {
+                    readWriteLock.readLock().unlock();
+                }
             }
         }
 
         @Override
         public void storeNode(@Nonnull FlowNode n) throws IOException {
-            readWriteLock.writeLock().lock();
             try (Timing t = time(TimingKind.flowNode)) {
-                delegate.storeNode(n);
-            } finally {
-                readWriteLock.writeLock().unlock();
+                readWriteLock.writeLock().lock();
+                try {
+                    delegate.storeNode(n);
+                } finally {
+                    readWriteLock.writeLock().unlock();
+                }
             }
         }
 
         @Override
         public void storeNode(@Nonnull FlowNode n, boolean delayWritingActions) throws IOException {
-            readWriteLock.writeLock().lock();
             try (Timing t = time(TimingKind.flowNode)) {
-                delegate.storeNode(n, delayWritingActions);
-            } finally {
-                readWriteLock.writeLock().unlock();
+                readWriteLock.writeLock().lock();
+                try {
+                    delegate.storeNode(n, delayWritingActions);
+                } finally {
+                    readWriteLock.writeLock().unlock();
+                }
             }
         }
 
         @Override
         public void flush() throws IOException {
-            readWriteLock.writeLock().lock();
             try (Timing t = time(TimingKind.flowNode)) {
-                delegate.flush();
-            } finally {
-                readWriteLock.writeLock().unlock();
+                readWriteLock.writeLock().lock();
+                try {
+                    delegate.flush();
+                } finally {
+                    readWriteLock.writeLock().unlock();
+                }
             }
         }
 
         @Override
         public void flushNode(FlowNode node) throws IOException {
-            readWriteLock.writeLock().lock();
             try (Timing t = time(TimingKind.flowNode)) {
-                delegate.flushNode(node);
-            } finally {
-                readWriteLock.writeLock().unlock();
+                readWriteLock.writeLock().lock();
+                try {
+                    delegate.flushNode(node);
+                } finally {
+                    readWriteLock.writeLock().unlock();
+                }
             }
         }
 
         @Override
         public void autopersist(@Nonnull FlowNode n) throws IOException {
-            readWriteLock.writeLock().lock();
             try (Timing t = time(TimingKind.flowNode)) {
-                delegate.autopersist(n);
-            }  finally {
-                readWriteLock.writeLock().unlock();
+                readWriteLock.writeLock().lock();
+                try {
+                    delegate.autopersist(n);
+                }  finally {
+                    readWriteLock.writeLock().unlock();
+                }
             }
         }
 
         @Override public List<Action> loadActions(FlowNode node) throws IOException {
-            readWriteLock.readLock().lock();
             try (Timing t = time(TimingKind.flowNode)) {
-                return delegate.loadActions(node);
-            } finally {
-                readWriteLock.readLock().unlock();
+                readWriteLock.readLock().lock();
+                try {
+                    return delegate.loadActions(node);
+                } finally {
+                    readWriteLock.readLock().unlock();
+                }
             }
         }
         @Override public void saveActions(FlowNode node, List<Action> actions) throws IOException {
-            readWriteLock.writeLock().lock();
             try (Timing t = time(TimingKind.flowNode)) {
-                delegate.saveActions(node, actions);
-            } finally {
-                readWriteLock.writeLock().unlock();
+                readWriteLock.writeLock().lock();
+                try {
+                    delegate.saveActions(node, actions);
+                } finally {
+                    readWriteLock.writeLock().unlock();
+                }
             }
         }
     }