Ensure that listeners do not get notified when a computer is killed from a...

Ensure that listeners do not get notified when a computer is killed from a thread with the Queue lock

- We don't know what those listeners will be doing
- I'm not completely happy with having to do it this way, there are some theoretical paths where we end
  up re-acquiring the Queue lock in Computer.kill() but they should not be the normal path

core/src/main/java/hudson/model/AbstractCIBase.java

@@ -34,7 +34,6 @@ import jenkins.model.Jenkins;
 import org.kohsuke.stapler.StaplerFallback;
 import org.kohsuke.stapler.StaplerProxy;
 
-import java.io.IOException;
 import java.util.*;
 import java.util.concurrent.CopyOnWriteArraySet;
 import java.util.logging.Logger;
@@ -165,6 +164,7 @@ public abstract class AbstractCIBase extends Node implements ItemGroup<TopLevelI
      */
     protected void updateComputerList(final boolean automaticSlaveLaunch) {
         final Map<Node,Computer> computers = getComputerMap();
+        final Set<Computer> old = new HashSet<Computer>(computers.size());
         Queue.withLock(new Runnable() {
             @Override
             public void run() {
@@ -174,9 +174,9 @@ public abstract class AbstractCIBase extends Node implements ItemGroup<TopLevelI
                     if (node == null)
                         continue;   // this computer is gone
                     byName.put(node.getNodeName(),c);
+                    old.add(c);
                 }
 
-                final Set<Computer> old = new HashSet<Computer>(computers.values());
                 Set<Computer> used = new HashSet<Computer>();
 
                 updateComputer(AbstractCIBase.this, byName, used, automaticSlaveLaunch);
@@ -192,11 +192,17 @@ public abstract class AbstractCIBase extends Node implements ItemGroup<TopLevelI
                 // when all executors exit, it will be removed from the computers map.
                 // so don't remove too quickly
                 old.removeAll(used);
+                // we need to start the process of reducing the executors on all computers as distinct
+                // from the killing action which should not excessively use the Queue lock.
                 for (Computer c : old) {
-                    killComputer(c);
+                    c.inflictMortalWound();
                 }
             }
         });
+        for (Computer c : old) {
+            // when we get to here, the number of executors should be zero so this call should not need the Queue.lock
+            killComputer(c);
+        }
         getQueue().scheduleMaintenance();
         for (ComputerListener cl : ComputerListener.all())
             cl.onConfigurationChange();

core/src/main/java/hudson/model/Computer.java

@@ -81,6 +81,7 @@ import org.kohsuke.stapler.export.ExportedBean;
 import org.kohsuke.args4j.Option;
 import org.kohsuke.stapler.interceptor.RequirePOST;
 
+import javax.annotation.concurrent.GuardedBy;
 import javax.servlet.ServletException;
 import java.io.File;
 import java.io.FilenameFilter;
@@ -756,6 +757,26 @@ public /*transient*/ abstract class Computer extends Actionable implements Acces
      * @see #onRemoved()
      */
     protected void kill() {
+        // On most code paths, this should already be zero, and thus this next call becomes a no-op... and more
+        // importantly it will not acquire a lock on the Queue... not that the lock is bad, more that the lock
+        // may delay unnecessarily
+        setNumExecutors(0);
+    }
+
+    /**
+     * Called by {@link Jenkins#updateComputerList()} to notify {@link Computer} that it will be discarded.
+     *
+     * <p>
+     * Note that at this point {@link #getNode()} returns null.
+     *
+     * <p>
+     * Note that the Queue lock is already held when this method is called.
+     *
+     * @see #onRemoved()
+     */
+    @Restricted(NoExternalUse.class)
+    @GuardedBy("hudson.model.Queue.lock")
+    /*package*/ void inflictMortalWound() {
         setNumExecutors(0);
     }