[JENKINS-30057] NodeProperties should be owned by the corresponding S…

…aveable. Node objects are owned by the Nodes class not by Jenkins so NodeProperties and anything modifiying them needs to be updated. Also modifying Nodes requires a lock on the Queue so use this lock not Jenkins to prevent the deadlock observed in JENKINS-30057 (cherry picked from commit 06f690d)
jenkinsci · Sep 10, 2015 · 0181650 · 0181650
1 parent 8af3ab5
commit 0181650
Show file tree

Hide file tree

Showing 3 changed files with 29 additions and 13 deletions.
diff --git a/core/src/main/java/hudson/model/Computer.java b/core/src/main/java/hudson/model/Computer.java
@@ -40,6 +40,7 @@
 import hudson.model.labels.LabelAtom;
 import hudson.model.queue.WorkUnit;
 import hudson.node_monitors.NodeMonitor;
+import hudson.remoting.Callable;
 import hudson.remoting.Channel;
 import hudson.remoting.VirtualChannel;
 import hudson.security.ACL;
@@ -67,6 +68,7 @@
 import jenkins.model.queue.AsynchronousExecution;
 import jenkins.util.ContextResettingExecutorService;
 import jenkins.security.MasterToSlaveCallable;
+
 import org.kohsuke.accmod.Restricted;
 import org.kohsuke.accmod.restrictions.DoNotUse;
 import org.kohsuke.accmod.restrictions.NoExternalUse;
@@ -87,6 +89,7 @@
 
 import javax.annotation.concurrent.GuardedBy;
 import javax.servlet.ServletException;
+
 import java.io.File;
 import java.io.FilenameFilter;
 import java.io.IOException;
@@ -108,6 +111,7 @@
 import java.net.Inet4Address;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
+
 import javax.annotation.CheckForNull;
 import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
@@ -1426,21 +1430,23 @@ public void doConfigDotXml(StaplerRequest req, StaplerResponse rsp)
     /**
      * Replaces the current {@link Node} by another one.
      */
-    private void replaceBy(Node newNode) throws ServletException, IOException {
+    private void replaceBy(final Node newNode) throws ServletException, IOException {
         final Jenkins app = Jenkins.getInstance();
 
-        // replace the old Node object by the new one
-        synchronized (app) {
-            List<Node> nodes = new ArrayList<Node>(app.getNodes());
-            Node node = getNode();
-            int i  = (node != null) ? nodes.indexOf(node) : -1;
-            if(i<0) {
-                throw new IOException("This slave appears to be removed while you were editing the configuration");
+        // use the queue lock until Nodes has a way of directly modifying a single node.
+        Queue.withLock(new Callable<Void, IOException>() {
+            public Void call() throws IOException {
+                List<Node> nodes = new ArrayList<Node>(app.getNodes());
+                Node node = getNode();
+                int i  = (node != null) ? nodes.indexOf(node) : -1;
+                if(i<0) {
+                    throw new IOException("This slave appears to be removed while you were editing the configuration");
+                }
+                nodes.set(i, newNode);
+                app.setNodes(nodes);
+                return null;
             }
-
-            nodes.set(i, newNode);
-            app.setNodes(nodes);
-        }
+        });
     }
 
     /**

diff --git a/core/src/main/java/hudson/model/Slave.java b/core/src/main/java/hudson/model/Slave.java
@@ -127,7 +127,7 @@ public abstract class Slave extends Node implements Serializable {
      */
     private String label="";
 
-    private /*almost final*/ DescribableList<NodeProperty<?>,NodePropertyDescriptor> nodeProperties = new DescribableList<NodeProperty<?>,NodePropertyDescriptor>(Jenkins.getInstance());
+    private /*almost final*/ DescribableList<NodeProperty<?>,NodePropertyDescriptor> nodeProperties = new DescribableList<NodeProperty<?>,NodePropertyDescriptor>(Jenkins.getInstance().getNodesObject());
 
     /**
      * Lazily computed set of labels from {@link #label}.

diff --git a/core/src/main/java/jenkins/model/Jenkins.java b/core/src/main/java/jenkins/model/Jenkins.java
@@ -1702,6 +1702,16 @@ public List<Node> getNodes() {
         return nodes.getNodes();
     }
 
+    /**
+     * Get the {@code Nodes}object that handles maintaining {@code Node}s.
+     * @return The Nodes object.
+     */
+    @Restricted(NoExternalUse.class)
+    public Nodes getNodesObject() {
+        // TODO replace this with something better when we properly expose Nodes.
+        return nodes;
+    }
+
     /**
      * Adds one more {@link Node} to Jenkins.
      */