Merge pull request #1213 from synopsys-arc-oss/node_classes_improvements

[READY] Additional null checks for node access methods.

core/src/main/java/hudson/cli/InstallToolCommand.java

@@ -122,6 +122,9 @@ public class InstallToolCommand extends CLICommand {
             throw new AbortException(b.getFullDisplayName()+" is not building");
 
         Node node = exec.getOwner().getNode();
+        if (node == null) {
+            throw new AbortException("The node " + exec.getOwner().getDisplayName() + " has been deleted");
+        }
 
         t = t.translate(node, EnvVars.getRemote(checkChannel()), new StreamTaskListener(stderr));
         stdout.println(t.getHome());

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

@@ -84,10 +84,11 @@ import java.util.Set;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 import javax.annotation.CheckForNull;
+import javax.annotation.Nonnull;
 import org.kohsuke.stapler.interceptor.RequirePOST;
 
 import static java.util.logging.Level.WARNING;
-import javax.annotation.Nonnull;
+
 import jenkins.model.lazy.BuildReference;
 import jenkins.model.lazy.LazyBuildMixIn;
 
@@ -436,6 +437,7 @@ public abstract class AbstractBuild<P extends AbstractProject<P,R>,R extends Abs
 
         /**
          * Returns the current {@link Node} on which we are building.
+         * @return Returns the current {@link Node}
          * @throws IllegalStateException if that cannot be determined
          */
         protected final @Nonnull Node getCurrentNode() throws IllegalStateException {
@@ -467,7 +469,7 @@ public abstract class AbstractBuild<P extends AbstractProject<P,R>,R extends Abs
          * @param wsl
          *      Passed in for the convenience. The returned path must be registered to this object.
          */
-        protected Lease decideWorkspace(Node n, WorkspaceList wsl) throws InterruptedException, IOException {
+        protected Lease decideWorkspace(@Nonnull Node n, WorkspaceList wsl) throws InterruptedException, IOException {
             String customWorkspace = getProject().getCustomWorkspace();
             if (customWorkspace != null) {
                 // we allow custom workspaces to be concurrently used between jobs.
@@ -477,8 +479,9 @@ public abstract class AbstractBuild<P extends AbstractProject<P,R>,R extends Abs
             return wsl.allocate(n.getWorkspaceFor((TopLevelItem)getProject()), getBuild());
         }
 
-        public Result run(BuildListener listener) throws Exception {
-            Node node = getCurrentNode();
+        public Result run(@Nonnull BuildListener listener) throws Exception {
+            final Node node = getCurrentNode();
+            
             assert builtOn==null;
             builtOn = node.getNodeName();
             hudsonVersion = Jenkins.VERSION;
@@ -566,8 +569,10 @@ public abstract class AbstractBuild<P extends AbstractProject<P,R>,R extends Abs
          * @param listener
          *      Always non-null. Connected to the main build output.
          */
-        protected Launcher createLauncher(BuildListener listener) throws IOException, InterruptedException {
-            Launcher l = getCurrentNode().createLauncher(listener);
+        @Nonnull
+        protected Launcher createLauncher(@Nonnull BuildListener listener) throws IOException, InterruptedException {
+            final Node currentNode = getCurrentNode();
+            Launcher l = currentNode.createLauncher(listener);
 
             if (project instanceof BuildableItemWithBuildWrappers) {
                 BuildableItemWithBuildWrappers biwbw = (BuildableItemWithBuildWrappers) project;
@@ -591,7 +596,7 @@ public abstract class AbstractBuild<P extends AbstractProject<P,R>,R extends Abs
                 }
             }
 
-            for (NodeProperty nodeProperty: Computer.currentComputer().getNode().getNodeProperties()) {
+            for (NodeProperty nodeProperty: currentNode.getNodeProperties()) {
                 Environment environment = nodeProperty.setUp(AbstractBuild.this, l, listener);
                 if (environment != null) {
                     buildEnvironments.add(environment);

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

@@ -165,9 +165,10 @@ public abstract class AbstractCIBase extends Node implements ItemGroup<TopLevelI
         synchronized(updateComputerLock) {// just so that we don't have two code updating computer list at the same time
             Map<String,Computer> byName = new HashMap<String,Computer>();
             for (Computer c : computers.values()) {
-                if(c.getNode()==null)
+                Node node = c.getNode();
+                if (node == null)
                     continue;   // this computer is gone
-                byName.put(c.getNode().getNodeName(),c);
+                byName.put(node.getNodeName(),c);
             }
 
             Set<Computer> old = new HashSet<Computer>(computers.values());

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

@@ -204,7 +204,7 @@ public /*transient*/ abstract class Computer extends Actionable implements Acces
 
     /**
      * This is where the log from the remote agent goes.
-     *
+     * The method also creates a log directory if required.
      * @see #relocateOldLogs()
      */
     protected File getLogFile() {
@@ -1196,7 +1196,11 @@ public /*transient*/ abstract class Computer extends Actionable implements Acces
             // read
             checkPermission(EXTENDED_READ);
             rsp.setContentType("application/xml");
-            Jenkins.XSTREAM2.toXMLUTF8(getNode(), rsp.getOutputStream());
+            Node node = getNode();
+            if (node == null) {
+                throw HttpResponses.notFound();
+            }
+            Jenkins.XSTREAM2.toXMLUTF8(node, rsp.getOutputStream());
             return;
         }
         if (req.getMethod().equals("POST")) {
@@ -1218,7 +1222,8 @@ public /*transient*/ abstract class Computer extends Actionable implements Acces
         // replace the old Node object by the new one
         synchronized (app) {
             List<Node> nodes = new ArrayList<Node>(app.getNodes());
-            int i = nodes.indexOf(getNode());
+            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");
             }
@@ -1348,7 +1353,12 @@ public /*transient*/ abstract class Computer extends Actionable implements Acces
             if (m.matches()) {
                 File newLocation = new File(dir, "logs/slaves/" + m.group(1) + "/slave.log" + Util.fixNull(m.group(2)));
                 newLocation.getParentFile().mkdirs();
-                f.renameTo(newLocation);
+                boolean relocationSuccessfull=f.renameTo(newLocation);
+                if (relocationSuccessfull) { // The operation will fail if mkdir fails 
+                    LOGGER.log(Level.INFO, "Relocated log file {0} to {1}",new Object[] {f.getPath(),newLocation.getPath()});
+                } else {
+                    LOGGER.log(Level.WARNING, "Cannot relocate log file {0} to {1}",new Object[] {f.getPath(),newLocation.getPath()});
+                }
             } else {
                 assert false;
             }

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

@@ -202,7 +202,7 @@ public abstract class Node extends AbstractModelObject implements Reconfigurable
             // At startup, we need to restore any previously in-effect temp offline cause.
             // We wait until the computer is started rather than getting the data to it sooner
             // so that the normal computer start up processing works as expected.
-            if (node.temporaryOfflineCause != null && node.temporaryOfflineCause != c.getOfflineCause()) {
+            if (node!= null && node.temporaryOfflineCause != null && node.temporaryOfflineCause != c.getOfflineCause()) {
                 c.setTemporarilyOffline(true, node.temporaryOfflineCause);
             }
         }

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

@@ -283,6 +283,7 @@ public class Queue extends ResourceController implements Saveable {
             return workUnit == null && !executor.getOwner().isOffline() && executor.getOwner().isAcceptingTasks();
         }
 
+        @CheckForNull
         public Node getNode() {
             return executor.getOwner().getNode();
         }
@@ -739,6 +740,8 @@ public class Queue extends ResourceController implements Saveable {
 
     private void _getBuildableItems(Computer c, ItemList<BuildableItem> col, List<BuildableItem> result) {
         Node node = c.getNode();
+        if (node == null)   // Deleted computers cannot take build items... 
+            return;
         for (BuildableItem p : col.values()) {
             if (node.canTake(p) == null)
                 result.add(p);

core/src/main/java/hudson/node_monitors/DiskSpaceMonitor.java

@@ -26,6 +26,7 @@ package hudson.node_monitors;
 import hudson.Extension;
 import hudson.FilePath;
 import hudson.model.Computer;
+import hudson.model.Node;
 import hudson.remoting.Callable;
 import jenkins.model.Jenkins;
 import hudson.node_monitors.DiskSpaceMonitorDescriptor.DiskSpace;
@@ -66,7 +67,10 @@ public class DiskSpaceMonitor extends AbstractDiskSpaceMonitor {
 
         @Override
         protected Callable<DiskSpace, IOException> createCallable(Computer c) {
-            FilePath p = c.getNode().getRootPath();
+            Node node = c.getNode();
+            if (node == null) return null;
+            
+            FilePath p = node.getRootPath();
             if(p==null) return null;
 
             return p.asCallableWith(new GetUsableSpace());

core/src/main/java/hudson/node_monitors/TemporarySpaceMonitor.java

@@ -27,6 +27,7 @@ import hudson.Extension;
 import hudson.FilePath;
 import hudson.FilePath.FileCallable;
 import hudson.model.Computer;
+import hudson.model.Node;
 import hudson.remoting.Callable;
 import jenkins.model.Jenkins;
 import hudson.node_monitors.DiskSpaceMonitorDescriptor.DiskSpace;
@@ -67,7 +68,10 @@ public class TemporarySpaceMonitor extends AbstractDiskSpaceMonitor {
 
         @Override
         protected Callable<DiskSpace,IOException> createCallable(Computer c) {
-            FilePath p = c.getNode().getRootPath();
+            Node node = c.getNode();
+            if (node == null) return null;
+            
+            FilePath p = node.getRootPath();
             if(p==null) return null;
 
             return p.asCallableWith(new GetTempSpace());

core/src/main/java/hudson/slaves/AbstractCloudComputer.java

@@ -29,6 +29,7 @@ import org.kohsuke.stapler.HttpResponse;
 import org.kohsuke.stapler.HttpResponses;
 
 import java.io.IOException;
+import javax.annotation.CheckForNull;
 
 /**
  * Partial implementation of {@link Computer} to be used in conjunction with
@@ -42,6 +43,7 @@ public class AbstractCloudComputer<T extends AbstractCloudSlave> extends SlaveCo
         super(slave);
     }
 
+    @CheckForNull
     @Override
     public T getNode() {
         return (T) super.getNode();
@@ -54,7 +56,10 @@ public class AbstractCloudComputer<T extends AbstractCloudSlave> extends SlaveCo
     public HttpResponse doDoDelete() throws IOException {
         checkPermission(DELETE);
         try {
-            getNode().terminate();
+            T node = getNode();
+            if (node != null) { // No need to terminate nodes again
+                node.terminate();
+            }
             return new HttpRedirect("..");
         } catch (InterruptedException e) {
             return HttpResponses.error(500,e);

core/src/main/java/hudson/slaves/CloudRetentionStrategy.java

@@ -23,10 +23,12 @@
  */
 package hudson.slaves;
 
+import hudson.model.Node;
 import java.io.IOException;
 import java.util.logging.Logger;
 
 import static hudson.util.TimeUnit2.*;
+import java.util.logging.Level;
 import static java.util.logging.Level.*;
 
 /**
@@ -43,13 +45,15 @@ public class CloudRetentionStrategy extends RetentionStrategy<AbstractCloudCompu
         this.idleMinutes = idleMinutes;
     }
 
+    @Override
     public synchronized long check(AbstractCloudComputer c) {
-        if (c.isIdle() && !disabled) {
+        AbstractCloudSlave computerNode = c.getNode();
+        if (c.isIdle() && !disabled && computerNode != null) {
             final long idleMilliseconds = System.currentTimeMillis() - c.getIdleStartMilliseconds();
             if (idleMilliseconds > MINUTES.toMillis(idleMinutes)) {
-                LOGGER.info("Disconnecting "+c.getName());
+                LOGGER.log(Level.INFO, "Disconnecting {0}",c.getName());
                 try {
-                    c.getNode().terminate();
+                    computerNode.terminate();
                 } catch (InterruptedException e) {
                     LOGGER.log(WARNING,"Failed to terminate "+c.getName(),e);
                 } catch (IOException e) {

core/src/main/java/hudson/slaves/CommandLauncher.java

@@ -23,10 +23,12 @@
  */
 package hudson.slaves;
 
+import hudson.AbortException;
 import hudson.EnvVars;
 import hudson.Util;
 import hudson.Extension;
 import hudson.model.Descriptor;
+import hudson.model.Slave;
 import jenkins.model.Jenkins;
 import hudson.model.TaskListener;
 import hudson.remoting.Channel;
@@ -87,6 +89,11 @@ public class CommandLauncher extends ComputerLauncher {
         EnvVars _cookie = null;
         Process _proc = null;
         try {
+            Slave node = computer.getNode();
+            if (node == null) { 
+                throw new AbortException("Cannot launch commands on deleted nodes");
+            }
+            
             listener.getLogger().println(hudson.model.Messages.Slave_Launching(getTimestamp()));
             if(getCommand().trim().length()==0) {
                 listener.getLogger().println(Messages.CommandLauncher_NoLaunchCommand());
@@ -96,8 +103,8 @@ public class CommandLauncher extends ComputerLauncher {
 
             ProcessBuilder pb = new ProcessBuilder(Util.tokenize(getCommand()));
             final EnvVars cookie = _cookie = EnvVars.createCookie();
-            pb.environment().putAll(cookie);
-            pb.environment().put("WORKSPACE", computer.getNode().getRemoteFS()); //path for local slave log
+            pb.environment().putAll(cookie);      
+            pb.environment().put("WORKSPACE", node.getRemoteFS()); //path for local slave log
 
             {// system defined variables
                 String rootUrl = Jenkins.getInstance().getRootUrl();

core/src/main/java/hudson/slaves/NodeSpecific.java

@@ -24,6 +24,7 @@
 
 package hudson.slaves;
 
+import edu.umd.cs.findbugs.annotations.NonNull;
 import hudson.model.Node;
 import hudson.model.EnvironmentSpecific;
 import hudson.model.TaskListener;
@@ -44,5 +45,5 @@ public interface NodeSpecific<T extends NodeSpecific<T>> {
     /**
      * Returns a specialized copy of T for functioning in the given node.
      */
-    T forNode(Node node, TaskListener log) throws IOException, InterruptedException;
+    T forNode(@NonNull Node node, TaskListener log) throws IOException, InterruptedException;
 }

core/src/main/java/hudson/slaves/RetentionStrategy.java

@@ -193,6 +193,7 @@ public abstract class RetentionStrategy<T extends Computer> extends AbstractDesc
             return idleDelay;
         }
 
+        @Override
         public synchronized long check(SlaveComputer c) {
             if (c.isOffline() && c.isLaunchSupported()) {
                 final HashMap<Computer, Integer> availableComputers = new HashMap<Computer, Integer>();
@@ -211,7 +212,8 @@ public abstract class RetentionStrategy<T extends Computer> extends AbstractDesc
                     // assume the answer is no until we can find such an executor
                     boolean needExecutor = true;
                     for (Computer o : Collections.unmodifiableSet(availableComputers.keySet())) {
-                        if (o.getNode().canTake(item) == null) {
+                        Node otherNode = o.getNode();
+                        if (otherNode != null && otherNode.canTake(item) == null) {
                             needExecutor = false;
                             final int availableExecutors = availableComputers.remove(o);
                             if (availableExecutors > 1) {
@@ -224,7 +226,8 @@ public abstract class RetentionStrategy<T extends Computer> extends AbstractDesc
                     }
 
                     // this 'item' cannot be built by any of the existing idle nodes, but it can be built by 'c'
-                    if (needExecutor && c.getNode().canTake(item) == null) {
+                    Node checkedNode = c.getNode();
+                    if (needExecutor && checkedNode != null && checkedNode.canTake(item) == null) {
                         demandMilliseconds = System.currentTimeMillis() - item.buildableStartMilliseconds;
                         needComputer = demandMilliseconds > inDemandDelay * 1000 * 60 /*MINS->MILLIS*/;
                         break;
@@ -251,6 +254,7 @@ public abstract class RetentionStrategy<T extends Computer> extends AbstractDesc
 
         @Extension
         public static class DescriptorImpl extends Descriptor<RetentionStrategy<?>> {
+            @Override
             public String getDisplayName() {
                 return Messages.RetentionStrategy_Demand_displayName();
             }

core/src/main/java/hudson/slaves/SlaveComputer.java

@@ -78,8 +78,11 @@ import java.util.logging.Level;
 import java.util.logging.LogRecord;
 import java.util.logging.Logger;
 
+import javax.annotation.CheckForNull;
+import jenkins.model.Jenkins;
 import static hudson.slaves.SlaveComputer.LogHolder.*;
 
+
 /**
  * {@link Computer} for {@link Slave}s.
  *
@@ -168,6 +171,7 @@ public class SlaveComputer extends Computer {
         return isUnix;
     }
 
+    @CheckForNull
     @Override
     public Slave getNode() {
         Node node = super.getNode();
@@ -418,7 +422,7 @@ public class SlaveComputer extends Computer {
     }
 
     /**
-     * Sets up the connection through an exsting channel.
+     * Sets up the connection through an existing channel.
      *
      * @since 1.444
      */
@@ -454,10 +458,15 @@ public class SlaveComputer extends Computer {
 
         String defaultCharsetName = channel.call(new DetectDefaultCharset());
 
-        String remoteFs = getNode().getRemoteFS();
+        Slave node = getNode();
+        if (node == null) { // Node has been disabled/removed during the connection
+            throw new IOException("Node "+nodeName+" has been deleted during the channel setup");
+        }
+        
+        String remoteFs = node.getRemoteFS();
         if(_isUnix && !remoteFs.contains("/") && remoteFs.contains("\\"))
             log.println("WARNING: "+remoteFs+" looks suspiciously like Windows path. Maybe you meant "+remoteFs.replace('\\','/')+"?");
-        FilePath root = new FilePath(channel,getNode().getRemoteFS());
+        FilePath root = new FilePath(channel,remoteFs);
 
         // reference counting problem is known to happen, such as JENKINS-9017, and so as a preventive measure
         // we pin the base classloader so that it'll never get GCed. When this classloader gets released,

core/src/main/java/hudson/tools/ToolInstallation.java

@@ -41,6 +41,7 @@ import java.util.List;
 import com.thoughtworks.xstream.annotations.XStreamSerializable;
 import com.thoughtworks.xstream.converters.UnmarshallingContext;
 import javax.annotation.CheckForNull;
+import javax.annotation.Nonnull;
 import jenkins.model.Jenkins;
 
 /**
@@ -163,7 +164,7 @@ public abstract class ToolInstallation extends AbstractDescribableImpl<ToolInsta
      * @see EnvironmentSpecific
      * @since 1.460
      */
-    public ToolInstallation translate(Node node, EnvVars envs, TaskListener listener) throws IOException, InterruptedException {
+    public ToolInstallation translate(@Nonnull Node node, EnvVars envs, TaskListener listener) throws IOException, InterruptedException {
         ToolInstallation t = this;
         if (t instanceof NodeSpecific) {
             NodeSpecific n = (NodeSpecific) t;