[FIXED JENKINS-25889] InputAction was improperly storing InputStepExe…

…cution instances, which should only have been stored in the program data file, never XStream. (Arguably it should have no state at all—FlowExecution knows the running input steps; but that could have threading issues.) Originally-Committed-As: 92a1a756870e7fd7c9f44382eaf48ade880e8c79
jenkinsci · Nov 18, 2015 · acb1337 · acb1337
1 parent 5cc9b91
commit acb1337
Show file tree

Hide file tree

Showing 4 changed files with 109 additions and 19 deletions.
diff --git a/...egator/src/test/java/org/jenkinsci/plugins/workflow/steps/input/InputStepRestartTest.java b/...egator/src/test/java/org/jenkinsci/plugins/workflow/steps/input/InputStepRestartTest.java
@@ -24,14 +24,24 @@
 
 package org.jenkinsci.plugins.workflow.steps.input;
 
+import java.io.File;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.commons.io.FileUtils;
 import org.jenkinsci.plugins.workflow.SingleJobTestBase;
 import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition;
+import org.jenkinsci.plugins.workflow.graph.FlowGraphWalker;
+import org.jenkinsci.plugins.workflow.graph.FlowNode;
 import org.jenkinsci.plugins.workflow.job.WorkflowJob;
+import org.jenkinsci.plugins.workflow.job.WorkflowRun;
+import org.jenkinsci.plugins.workflow.support.actions.PauseAction;
 import org.jenkinsci.plugins.workflow.support.steps.input.InputAction;
+import org.jenkinsci.plugins.workflow.support.steps.input.InputStepExecution;
 import static org.junit.Assert.*;
 import org.junit.Test;
 import org.junit.runners.model.Statement;
 import org.jvnet.hudson.test.Issue;
+import org.jvnet.hudson.test.recipes.LocalData;
 
 public class InputStepRestartTest extends SingleJobTestBase {
 
@@ -48,10 +58,44 @@ public class InputStepRestartTest extends SingleJobTestBase {
         story.addStep(new Statement() {
             @Override public void evaluate() throws Throwable {
                 rebuildContext(story.j);
-                InputAction a = b.getAction(InputAction.class);
-                assertEquals(1, a.getExecutions().size());
-                story.j.submit(story.j.createWebClient().getPage(b, a.getUrlName()).getFormByName(a.getExecutions().get(0).getId()), "proceed");
+                proceed(b);
                 story.j.assertBuildStatusSuccess(story.j.waitForCompletion(b));
+                sanity(b);
+            }
+        });
+    }
+
+    private void proceed(WorkflowRun b) throws Exception {
+        InputAction a = b.getAction(InputAction.class);
+        assertNotNull(a);
+        assertEquals(1, a.getExecutions().size());
+        story.j.submit(story.j.createWebClient().getPage(b, a.getUrlName()).getFormByName(a.getExecutions().get(0).getId()), "proceed");
+    }
+
+    private void sanity(WorkflowRun b) throws Exception {
+        List<PauseAction> pauses = new ArrayList<PauseAction>();
+        for (FlowNode n : new FlowGraphWalker(b.getExecution())) {
+            pauses.addAll(PauseAction.getPauseActions(n));
+        }
+        assertEquals(1, pauses.size());
+        assertFalse(pauses.get(0).isPaused());
+        String xml = FileUtils.readFileToString(new File(b.getRootDir(), "build.xml"));
+        assertFalse(xml, xml.contains(InputStepExecution.class.getName()));
+    }
+
+    @Issue("JENKINS-25889")
+    @LocalData // from 1.4.2
+    @Test public void oldFlow() {
+        story.addStep(new Statement() {
+            @Override public void evaluate() throws Throwable {
+                WorkflowJob p = jenkins().getItemByFullName("p", WorkflowJob.class);
+                assertNotNull(p);
+                WorkflowRun b = p.getLastBuild();
+                assertNotNull(b);
+                assertEquals(1, b.getNumber());
+                proceed(b);
+                story.j.assertBuildStatusSuccess(story.j.waitForCompletion(b));
+                sanity(b);
             }
         });
     }

diff --git a/...est/resources/org/jenkinsci/plugins/workflow/steps/input/InputStepRestartTest/oldFlow.zip b/...est/resources/org/jenkinsci/plugins/workflow/steps/input/InputStepRestartTest/oldFlow.zip
diff --git a/support/src/main/java/org/jenkinsci/plugins/workflow/support/steps/input/InputAction.java b/support/src/main/java/org/jenkinsci/plugins/workflow/support/steps/input/InputAction.java
@@ -6,16 +6,22 @@
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.logging.Level;
+import java.util.logging.Logger;
 import javax.annotation.Nonnull;
+import org.jenkinsci.plugins.workflow.flow.FlowExecution;
+import org.jenkinsci.plugins.workflow.flow.FlowExecutionList;
+import org.jenkinsci.plugins.workflow.steps.StepExecution;
 
 /**
  * Records the pending inputs required.
- *
- * @author Kohsuke Kawaguchi
  */
 public class InputAction implements RunAction2 {
 
-    private final List<InputStepExecution> executions = new ArrayList<InputStepExecution>();
+    private static final Logger LOGGER = Logger.getLogger(InputAction.class.getName());
+
+    private transient List<InputStepExecution> executions = new ArrayList<InputStepExecution>();
+    private List<String> ids = new ArrayList<String>();
 
     private transient Run<?,?> run;
 
@@ -27,9 +33,49 @@ public void onAttached(Run<?, ?> r) {
     @Override
     public void onLoad(Run<?, ?> r) {
         this.run = r;
-        assert executions != null && !executions.contains(null) : executions;
-        for (InputStepExecution step : executions) {
-            step.run = run;
+        if (ids == null) {
+            // Loading from before JENKINS-25889 fix. Load the IDs and discard the executions, which lack state anyway.
+            assert executions != null && !executions.contains(null) : executions;
+            ids = new ArrayList<String>();
+            for (InputStepExecution execution : executions) {
+                ids.add(execution.getId());
+            }
+            executions = null;
+        }
+    }
+
+    private synchronized void loadExecutions() {
+        if (executions == null) {
+            executions = new ArrayList<InputStepExecution>();
+            try {
+            FlowExecution execution = null;
+            for (FlowExecution _execution : FlowExecutionList.get()) {
+                if (_execution.getOwner().getExecutable() == run) {
+                    execution = _execution;
+                    break;
+                }
+            }
+            if (execution != null) {
+                // Futures.addCallback is the safer way to iterate, but we need to know that the result is in.
+                // And we cannot start the calculation during onLoad because we are still inside WorkflowRun.onLoad and the CpsFlowExecution is not yet initialized.
+                // WorkflowRun has getExecutionPromise but that is not accessible via API.
+                for (StepExecution se : execution.getCurrentExecutions(true).get()) {
+                    if (se instanceof InputStepExecution) {
+                        InputStepExecution ise = (InputStepExecution) se;
+                        if (ids.contains(ise.getId())) {
+                            executions.add(ise);
+                        }
+                    }
+                }
+                if (executions.size() < ids.size()) {
+                    LOGGER.log(Level.WARNING, "some input IDs not restored from {0}", run);
+                }
+            } else {
+                LOGGER.log(Level.WARNING, "no flow execution found for {0}", run);
+            }
+            } catch (Exception x) {
+                LOGGER.log(Level.WARNING, null, x);
+            }
         }
     }
 
@@ -39,12 +85,14 @@ public void onLoad(Run<?, ?> r) {
 
     @Override
     public String getIconFileName() {
+        loadExecutions();
         if (executions.isEmpty())    return null;
         else                    return "help.png";
     }
 
     @Override
     public String getDisplayName() {
+        loadExecutions();
         if (executions.isEmpty())    return null;
         else                    return "Paused for Input";
     }
@@ -55,11 +103,14 @@ public String getUrlName() {
     }
 
     public synchronized void add(@Nonnull InputStepExecution step) throws IOException {
+        loadExecutions();
         this.executions.add(step);
+        ids.add(step.getId());
         run.save();
     }
 
     public synchronized InputStepExecution getExecution(String id) {
+        loadExecutions();
         for (InputStepExecution e : executions) {
             if (e.input.getId().equals(id))
                 return e;
@@ -68,14 +119,17 @@ public synchronized InputStepExecution getExecution(String id) {
     }
 
     public synchronized List<InputStepExecution> getExecutions() {
+        loadExecutions();
         return new ArrayList<InputStepExecution>(executions);
     }
 
     /**
      * Called when {@link InputStepExecution} is completed to remove it from the active input list.
      */
     public synchronized void remove(InputStepExecution exec) throws IOException {
+        loadExecutions();
         executions.remove(exec);
+        ids.remove(exec.getId());
         run.save();
     }
 

diff --git a/.../src/main/java/org/jenkinsci/plugins/workflow/support/steps/input/InputStepExecution.java b/.../src/main/java/org/jenkinsci/plugins/workflow/support/steps/input/InputStepExecution.java
@@ -33,22 +33,14 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
-import java.util.logging.Level;
-import java.util.logging.Logger;
 import org.jenkinsci.plugins.workflow.steps.FlowInterruptedException;
 
 /**
  * @author Kohsuke Kawaguchi
  */
 public class InputStepExecution extends AbstractStepExecutionImpl implements ModelObject {
 
-    private static final Logger LOGGER = Logger.getLogger(InputStepExecution.class.getName());
-
-    /**
-     * Pause gets added here.
-     */
-    @StepContextParameter
-    /*package*/ transient Run run;
+    @StepContextParameter private transient Run run;
 
     @StepContextParameter private transient TaskListener listener;
 
@@ -233,7 +225,7 @@ private void postSettlement() throws IOException {
             if (node != null) {
                 PauseAction.endCurrentPause(node);
             } else {
-                LOGGER.log(Level.WARNING, "cannot set pause end time for {0} in {1}", new Object[] {getId(), run});
+                assert false : "cannot set pause end time for " + getId() + " in " + run;
             }
         }
     }