From 07fb00e05ab37a7a09bc99b6316ace16916f1999 Mon Sep 17 00:00:00 2001
From: Stephen Connolly <stephen.alan.connolly@gmail.com>
Date: Tue, 28 Oct 2014 13:48:00 +0000
Subject: [PATCH] If you are synchronizing on something to access a volatile,
 then you are doing something wrong

- Yet more interrupting bad patterns, especially where we expect the node monitor to take up to 30 seconds to complete if an async variety
---
 .../AbstractAsyncNodeMonitorDescriptor.java   |  7 ---
 .../AbstractNodeMonitorDescriptor.java        | 52 +++++++++++++------
 2 files changed, 37 insertions(+), 22 deletions(-)

diff --git a/core/src/main/java/hudson/node_monitors/AbstractAsyncNodeMonitorDescriptor.java b/core/src/main/java/hudson/node_monitors/AbstractAsyncNodeMonitorDescriptor.java
index bfb88d264e..7a0a0237fd 100644
--- a/core/src/main/java/hudson/node_monitors/AbstractAsyncNodeMonitorDescriptor.java
+++ b/core/src/main/java/hudson/node_monitors/AbstractAsyncNodeMonitorDescriptor.java
@@ -108,12 +108,5 @@ public abstract class AbstractAsyncNodeMonitorDescriptor<T> extends AbstractNode
         return data;
     }
 
-    /**
-     * Controls the time out of monitoring.
-     */
-    protected long getMonitoringTimeOut() {
-        return TimeUnit.SECONDS.toMillis(30);
-    }
-
     private static final Logger LOGGER = Logger.getLogger(AbstractAsyncNodeMonitorDescriptor.class.getName());
 }
diff --git a/core/src/main/java/hudson/node_monitors/AbstractNodeMonitorDescriptor.java b/core/src/main/java/hudson/node_monitors/AbstractNodeMonitorDescriptor.java
index 5e45bd224e..21c0f95dd3 100644
--- a/core/src/main/java/hudson/node_monitors/AbstractNodeMonitorDescriptor.java
+++ b/core/src/main/java/hudson/node_monitors/AbstractNodeMonitorDescriptor.java
@@ -26,6 +26,7 @@ package hudson.node_monitors;
 import hudson.Util;
 import hudson.model.Computer;
 import hudson.model.Descriptor;
+import hudson.model.Run;
 import jenkins.model.Jenkins;
 import hudson.model.ComputerSet;
 import hudson.model.AdministrativeMonitor;
@@ -33,6 +34,7 @@ import hudson.triggers.SafeTimerTask;
 import hudson.slaves.OfflineCause;
 import jenkins.util.Timer;
 
+import javax.annotation.concurrent.GuardedBy;
 import java.io.IOException;
 import java.util.Collections;
 import java.util.Date;
@@ -105,7 +107,14 @@ public abstract class AbstractNodeMonitorDescriptor<T> extends Descriptor<NodeMo
     /**
      * Represents the update activity in progress.
      */
-    private transient volatile Record inProgress = null;
+    @GuardedBy("this")
+    private transient Record inProgress = null;
+
+    /**
+     * Represents when an update activity was last started.
+     */
+    @GuardedBy("this")
+    private transient long inProgressStarted = Long.MIN_VALUE;
 
     /**
      * Performs monitoring of the given computer object.
@@ -171,9 +180,8 @@ public abstract class AbstractNodeMonitorDescriptor<T> extends Descriptor<NodeMo
     /**
      * Is the monitoring activity currently in progress?
      */
-    private boolean isInProgress() {
-        Record r = inProgress;  // capture for atomicity
-        return r!=null && r.isAlive();
+    private synchronized boolean isInProgress() {
+        return inProgress !=null && inProgress.isAlive();
     }
 
     /**
@@ -242,10 +250,32 @@ public abstract class AbstractNodeMonitorDescriptor<T> extends Descriptor<NodeMo
     /**
      * @see NodeMonitor#triggerUpdate()
      */
-    /*package*/ Thread triggerUpdate() {
-        Record t = new Record();
+    /*package*/ synchronized Thread triggerUpdate() {
+        if (inProgress != null) {
+            if (System.currentTimeMillis() > inProgressStarted + getMonitoringTimeOut() + 1000) {
+                // maybe it got stuck?
+                LOGGER.log(Level.WARNING, "Previous {0} monitoring activity still in progress. Interrupting",
+                        getDisplayName());
+                inProgress.interrupt();
+                inProgress = null; // we interrupted the old one so it's now dead to us.
+            } else {
+                // return the in progress
+                return inProgress;
+            }
+        }
+        final Record t = new Record();
         t.start();
-        return t;
+        // only store the new thread if we started it
+        inProgress = t;
+        inProgressStarted = System.currentTimeMillis();
+        return inProgress;
+    }
+
+    /**
+     * Controls the time out of monitoring.
+     */
+    protected long getMonitoringTimeOut() {
+        return TimeUnit.SECONDS.toMillis(30);
     }
 
     /**
@@ -262,14 +292,6 @@ public abstract class AbstractNodeMonitorDescriptor<T> extends Descriptor<NodeMo
 
         public Record() {
             super("Monitoring thread for "+getDisplayName()+" started on "+new Date());
-            synchronized(AbstractNodeMonitorDescriptor.this) {
-                if(inProgress!=null) {
-                    // maybe it got stuck?
-                    LOGGER.log(Level.WARNING, "Previous {0} monitoring activity still in progress. Interrupting", getDisplayName());
-                    inProgress.interrupt();
-                }
-                inProgress = this;
-            }
         }
 
         @Override