[FIXED JENKINS-31055] Make Node implement Saveable

- Also tidy up some of the mess logic in both #1866 and #1860

changelog.html

@@ -56,6 +56,9 @@ Upcoming changes</a>
 <div id="trunk" style="display:none"><!--=TRUNK-BEGIN=-->
 <ul class=image>
   <li class=>
+  <li class="rfe">
+    Make Node implement Saveable.
+    (<a href="https://issues.jenkins-ci.org/browse/JENKINS-31055">issue 31055</a>)
 </ul>
 </div><!--=TRUNK-END=-->
 <h3><a name=v1.634>What's new in 1.634</a> (2015/10/18)</h3>

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

@@ -695,7 +695,6 @@ public /*transient*/ abstract class Computer extends Actionable implements Acces
         if (node != null) {
             node.setTemporaryOfflineCause(offlineCause);
         }
-        Jenkins.getInstance().getQueue().scheduleMaintenance();
         synchronized (statusChangeLock) {
             statusChangeLock.notifyAll();
         }

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

@@ -25,7 +25,12 @@
 package hudson.model;
 
 import com.infradna.tool.bridge_method_injector.WithBridgeMethods;
-import hudson.*;
+import hudson.Extension;
+import hudson.ExtensionPoint;
+import hudson.FilePath;
+import hudson.FileSystemProvisioner;
+import hudson.Launcher;
+import hudson.Util;
 import hudson.model.Descriptor.FormException;
 import hudson.model.Queue.Task;
 import hudson.model.labels.LabelAtom;
@@ -46,17 +51,15 @@ import hudson.util.DescribableList;
 import hudson.util.EnumConverter;
 import hudson.util.TagCloud;
 import hudson.util.TagCloud.WeightFunction;
-
 import java.io.IOException;
 import java.lang.reflect.Type;
 import java.util.Collections;
 import java.util.HashSet;
-import java.util.Set;
 import java.util.List;
+import java.util.Set;
 import java.util.logging.Logger;
 import javax.annotation.CheckForNull;
 import javax.annotation.Nonnull;
-
 import jenkins.model.Jenkins;
 import jenkins.util.io.OnMaster;
 import net.sf.json.JSONObject;
@@ -65,8 +68,8 @@ import org.jvnet.localizer.Localizable;
 import org.kohsuke.stapler.BindInterceptor;
 import org.kohsuke.stapler.Stapler;
 import org.kohsuke.stapler.StaplerRequest;
-import org.kohsuke.stapler.export.ExportedBean;
 import org.kohsuke.stapler.export.Exported;
+import org.kohsuke.stapler.export.ExportedBean;
 
 /**
  * Base type of Jenkins slaves (although in practice, you probably extend {@link Slave} to define a new slave type).
@@ -88,7 +91,7 @@ import org.kohsuke.stapler.export.Exported;
  * @see Computer
  */
 @ExportedBean
-public abstract class Node extends AbstractModelObject implements ReconfigurableDescribable<Node>, ExtensionPoint, AccessControlled, OnMaster {
+public abstract class Node extends AbstractModelObject implements ReconfigurableDescribable<Node>, ExtensionPoint, AccessControlled, OnMaster, Saveable {
 
     private static final Logger LOGGER = Logger.getLogger(Node.class.getName());
 
@@ -114,6 +117,18 @@ public abstract class Node extends AbstractModelObject implements Reconfigurable
         return holdOffLaunchUntilSave;
     }
 
+    /**
+     * {@inheritDoc}
+     * @since 1.635
+     */
+    @Override
+    public void save() throws IOException {
+        final Jenkins jenkins = Jenkins.getInstance();
+        if (jenkins != null) {
+            jenkins.updateNode(this);
+        }
+    }
+
     /**
      * Name of this node.
      *
@@ -237,7 +252,7 @@ public abstract class Node extends AbstractModelObject implements Reconfigurable
         try {
             if (temporaryOfflineCause != cause) {
                 temporaryOfflineCause = cause;
-                Jenkins.getInstance().save(); // Gotta be a better way to do this
+                save();
             }
         } catch (java.io.IOException e) {
             LOGGER.warning("Unable to complete save, temporary offline status will not be persisted: " + e.getMessage());

core/src/main/java/jenkins/model/Jenkins.java

@@ -147,7 +147,6 @@ import hudson.security.csrf.CrumbIssuer;
 import hudson.slaves.Cloud;
 import hudson.slaves.ComputerListener;
 import hudson.slaves.DumbSlave;
-import hudson.slaves.EphemeralNode;
 import hudson.slaves.NodeDescriptor;
 import hudson.slaves.NodeList;
 import hudson.slaves.NodeProperty;
@@ -289,7 +288,6 @@ import java.util.concurrent.ThreadPoolExecutor;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.TimeoutException;
 import java.util.concurrent.atomic.AtomicBoolean;
-import java.util.concurrent.locks.ReentrantReadWriteLock;
 import java.util.logging.Level;
 import java.util.logging.LogRecord;
 import java.util.logging.Logger;
@@ -1752,7 +1750,12 @@ public class Jenkins extends AbstractCIBase implements DirectlyModifiableTopLeve
     }
 
     /**
-     * Updates an existing {@link Node} on disk.
+     * Saves an existing {@link Node} on disk, called by {@link Node#save()}. This method is preferred in those cases
+     * where you need to determine atomically that the node being saved is actually in the list of nodes.
+     *
+     * @param n the node to be updated.
+     * @return {@code true}, if the node was updated. {@code false}, if the node was not in the list of nodes.
+     * @throws IOException if the node could not be persisted.
      * @see Nodes#updateNode
      * @since 1.634
      */

core/src/main/java/jenkins/model/Nodes.java

@@ -34,6 +34,7 @@ import hudson.model.Saveable;
 import hudson.model.listeners.SaveableListener;
 import hudson.slaves.EphemeralNode;
 import hudson.slaves.OfflineCause;
+import java.util.concurrent.Callable;
 import org.kohsuke.accmod.Restricted;
 import org.kohsuke.accmod.restrictions.NoExternalUse;
 
@@ -157,6 +158,7 @@ public class Nodes implements Saveable {
             xmlFile.write(node);
             SaveableListener.fireOnChange(this, xmlFile);
         }
+        jenkins.getQueue().scheduleMaintenance();
     }
 
     /**
@@ -166,17 +168,27 @@ public class Nodes implements Saveable {
      * @param node the node to be updated.
      * @return {@code true}, if the node was updated. {@code false}, if the node was not in the list of nodes.
      * @throws IOException if the node could not be persisted.
+     * @since 1.634
      */
     public boolean updateNode(final @Nonnull Node node) throws IOException {
-        if (node == nodes.get(node.getNodeName())) {
-            Queue.withLock(new Runnable() {
+        boolean exists;
+        try {
+            exists = Queue.withLock(new Callable<Boolean>() {
                 @Override
-                public void run() {
-                    jenkins.trimLabels();
+                public Boolean call() throws Exception {
+                    if (node == nodes.get(node.getNodeName())) {
+                        jenkins.trimLabels();
+                        return true;
+                    }
+                    return false;
                 }
             });
+        } catch (Exception e) {
+            // can never happen
+            exists = false;
+        }
+        if (exists) {
             persistNode(node);
-            jenkins.getQueue().scheduleMaintenance();
             return true;
         }
         return false;