[FIXED JENKINS-29568] A better fix

- 252e129 doesn't work for all cases as there are code paths where a pending launch can be removed from the pending list and not have spent() called. - There was no reason for iterating the list twice anyway, as all of this takes place with the locks held - My notifying each one as we process, if there is an Error, we will not leave any stranded. The next run through, if there is one, will cover those instances.
jenkinsci · Jul 23, 2015 · 4f0ca16 · KostyaSha · Jul 23, 2015 · 4f0ca16
1 parent 4ece8c0
commit 4f0ca16
Showing 1 changed file with 35 additions and 33 deletions.
diff --git a/core/src/main/java/hudson/slaves/NodeProvisioner.java b/core/src/main/java/hudson/slaves/NodeProvisioner.java
@@ -213,48 +213,50 @@ public void run() {
                     // bring up.
 
                     int plannedCapacitySnapshot = 0;
-                    List<PlannedNode> completedLaunches = new ArrayList<PlannedNode>();
 
                     for (Iterator<PlannedNode> itr = pendingLaunches.iterator(); itr.hasNext(); ) {
                         PlannedNode f = itr.next();
                         if (f.future.isDone()) {
-                            completedLaunches.add(f);
-                            itr.remove();
+                            try {
+                                Node node = f.future.get();
+                                for (CloudProvisioningListener cl : CloudProvisioningListener.all()) {
+                                    cl.onComplete(f, node);
+                                }
+
+                                jenkins.addNode(node);
+                                LOGGER.log(Level.INFO,
+                                        "{0} provisioning successfully completed. " 
+                                                + "We have now {1,number,integer} computer(s)",
+                                        new Object[]{f.displayName, jenkins.getComputers().length});
+                            } catch (InterruptedException e) {
+                                throw new AssertionError(e); // since we confirmed that the future is already done
+                            } catch (ExecutionException e) {
+                                LOGGER.log(Level.WARNING, "Provisioned slave " + f.displayName + " failed to launch",
+                                        e.getCause());
+                                for (CloudProvisioningListener cl : CloudProvisioningListener.all()) {
+                                    cl.onFailure(f, e.getCause());
+                                }
+                            } catch (IOException e) {
+                                LOGGER.log(Level.WARNING, "Provisioned slave " + f.displayName + " failed to launch", 
+                                        e);
+                                for (CloudProvisioningListener cl : CloudProvisioningListener.all()) {
+                                    cl.onFailure(f, e);
+                                }
+                            } catch (Error e) {
+                                // we are not supposed to try and recover from Errors
+                                throw e;
+                            } catch (Throwable e) {
+                                LOGGER.log(Level.SEVERE, "Unexpected uncaught exception encountered while " 
+                                        + "processing provisioned slave " + f.displayName, e);
+                            } finally {
+                                itr.remove();
+                                f.spent();
+                            }
                         } else {
                             plannedCapacitySnapshot += f.numExecutors;
                         }
                     }
 
-                    for (PlannedNode f : completedLaunches) {
-                        try {
-                            Node node = f.future.get();
-                            for (CloudProvisioningListener cl : CloudProvisioningListener.all()) {
-                                cl.onComplete(f, node);
-                            }
-
-                            jenkins.addNode(node);
-                            LOGGER.log(Level.INFO,
-                                    "{0} provisioning successfully completed. We have now {1,number,integer} computer"
-                                            + "(s)",
-                                    new Object[]{f.displayName, jenkins.getComputers().length});
-                        } catch (InterruptedException e) {
-                            throw new AssertionError(e); // since we confirmed that the future is already done
-                        } catch (ExecutionException e) {
-                            LOGGER.log(Level.WARNING, "Provisioned slave " + f.displayName + " failed to launch",
-                                    e.getCause());
-                            for (CloudProvisioningListener cl : CloudProvisioningListener.all()) {
-                                cl.onFailure(f, e.getCause());
-                            }
-                        } catch (IOException e) {
-                            LOGGER.log(Level.WARNING, "Provisioned slave " + f.displayName + " failed to launch", e);
-                            for (CloudProvisioningListener cl : CloudProvisioningListener.all()) {
-                                cl.onFailure(f, e);
-                            }
-                        } finally {
-                            f.spent();
-                        }
-                    }
-
                     float plannedCapacity = plannedCapacitySnapshot;
                     plannedCapacitiesEMA.update(plannedCapacity);