[JENKINS-43496] - Add handling of the null Node#createComputer() resu…

…lt. (#2922) * [JENKINS-43496] - Add handling of the null Node#createComputer() result. it is a follow-up to #2836 (comment) De-facto many Cloud plugins return `null` in `Node#createLauncher()`, but it has never been documented. In order to prevent possible API misusages in the future, I have added annotations and fixed handling of the extension point in `AbstractCIBase#updateComputer()` which may fail in the case of `null` or `RuntimeException` in the Node implementation. * [JENKINS-43496] - Use ProtectedExternally to protect Node#createComputer() * [JENKINS-43496] - Remove the erroneous Nonnull annotation after the feedback from @jglick * [JENKINS-43496] - Fix typos noticed by @daniel-beck
jenkinsci · Jul 1, 2017 · bcf55ec · bcf55ec
1 parent e7cdd65
commit bcf55ec
Show file tree

Hide file tree

Showing 4 changed files with 32 additions and 5 deletions.
diff --git a/core/src/main/java/hudson/model/AbstractCIBase.java b/core/src/main/java/hudson/model/AbstractCIBase.java
@@ -39,6 +39,7 @@
 import java.util.logging.Level;
 import java.util.logging.Logger;
 import javax.annotation.CheckForNull;
+import javax.annotation.Nonnull;
 
 import jenkins.model.Configuration;
 
@@ -125,7 +126,18 @@ private void updateComputer(Node n, Map<String,Computer> byNameMap, Set<Computer
         } else {
             // we always need Computer for the master as a fallback in case there's no other Computer.
             if(n.getNumExecutors()>0 || n==Jenkins.getInstance()) {
-                computers.put(n, c = n.createComputer());
+                try {
+                    c = n.createComputer();
+                } catch(RuntimeException ex) { // Just in case there is a bogus extension
+                    LOGGER.log(Level.WARNING, "Error retrieving computer for node " + n.getNodeName() + ", continuing", ex);
+                }
+                if (c == null) {
+                    LOGGER.log(Level.WARNING, "Cannot create computer for node {0}, the {1}#createComputer() method returned null. Skipping this node", 
+                            new Object[]{n.getNodeName(), n.getClass().getName()});
+                    return;
+                }
+
+                computers.put(n, c);
                 if (!n.isHoldOffLaunchUntilSave() && automaticSlaveLaunch) {
                     RetentionStrategy retentionStrategy = c.getRetentionStrategy();
                     if (retentionStrategy != null) {
@@ -136,8 +148,11 @@ private void updateComputer(Node n, Map<String,Computer> byNameMap, Set<Computer
                         c.connect(true);
                     }
                 }
+                used.add(c);
+            } else {
+                // TODO: Maybe it should be allowed, but we would just get NPE in the original logic before JENKINS-43496
+                LOGGER.log(Level.WARNING, "Node {0} has no executors. Cannot update the Computer instance of it", n.getNodeName());
             }
-            used.add(c);
         }
     }
 
@@ -184,7 +199,7 @@ public void run() {
                     byName.put(node.getNodeName(),c);
                 }
 
-                Set<Computer> used = new HashSet<Computer>(old.size());
+                Set<Computer> used = new HashSet<>(old.size());
 
                 updateComputer(AbstractCIBase.this, byName, used, automaticSlaveLaunch);
                 for (Node s : getNodes()) {

diff --git a/core/src/main/java/hudson/model/Node.java b/core/src/main/java/hudson/model/Node.java
@@ -40,6 +40,7 @@
 import hudson.security.ACL;
 import hudson.security.AccessControlled;
 import hudson.security.Permission;
+import hudson.slaves.Cloud;
 import hudson.slaves.ComputerListener;
 import hudson.slaves.NodeDescriptor;
 import hudson.slaves.NodeProperty;
@@ -65,6 +66,8 @@
 import net.sf.json.JSONObject;
 import org.acegisecurity.Authentication;
 import org.jvnet.localizer.Localizable;
+import org.kohsuke.accmod.Restricted;
+import org.kohsuke.accmod.restrictions.ProtectedExternally;
 import org.kohsuke.stapler.BindInterceptor;
 import org.kohsuke.stapler.Stapler;
 import org.kohsuke.stapler.StaplerRequest;
@@ -214,8 +217,13 @@ public final VirtualChannel getChannel() {
 
     /**
      * Creates a new {@link Computer} object that acts as the UI peer of this {@link Node}.
+     * 
      * Nobody but {@link Jenkins#updateComputerList()} should call this method.
+     * @return Created instance of the computer.
+     *         Can be {@code null} if the {@link Node} implementation does not support it (e.g. {@link Cloud} agent).
      */
+    @CheckForNull
+    @Restricted(ProtectedExternally.class)
     protected abstract Computer createComputer();
 
     /**

diff --git a/core/src/main/java/hudson/slaves/RetentionStrategy.java b/core/src/main/java/hudson/slaves/RetentionStrategy.java
@@ -39,6 +39,7 @@
 import java.util.concurrent.TimeUnit;
 import java.util.logging.Level;
 import java.util.logging.Logger;
+import javax.annotation.Nonnull;
 
 /**
  * Controls when to take {@link Computer} offline, bring it back online, or even to destroy it.
@@ -57,7 +58,7 @@ public abstract class RetentionStrategy<T extends Computer> extends AbstractDesc
      *         rechecked earlier or later that this!
      */
     @GuardedBy("hudson.model.Queue.lock")
-    public abstract long check(T c);
+    public abstract long check(@Nonnull T c);
 
     /**
      * This method is called to determine whether manual launching of the agent is allowed at this point in time.
@@ -92,9 +93,10 @@ public boolean isAcceptingTasks(T c) {
      * The default implementation of this method delegates to {@link #check(Computer)},
      * but this allows {@link RetentionStrategy} to distinguish the first time invocation from the rest.
      *
+     * @param c Computer instance
      * @since 1.275
      */
-    public void start(final T c) {
+    public void start(final @Nonnull T c) {
         Queue.withLock(new Runnable() {
             @Override
             public void run() {

diff --git a/core/src/main/java/jenkins/model/Jenkins.java b/core/src/main/java/jenkins/model/Jenkins.java
@@ -3051,6 +3051,8 @@ public LabelAtom getSelfLabel() {
         return getLabelAtom("master");
     }
 
+    @Override
+    @Nonnull
     public Computer createComputer() {
         return new Hudson.MasterComputer();
     }