[FIXED JENKINS-29571] Make CpsFlowExecution.owner transient (JENKINS-…

…27704) to avoid problems after renames/moves.
jenkinsci · Jul 22, 2015 · b7014e6 · b7014e6
1 parent 9d087ca
commit b7014e6
Show file tree

Hide file tree

Showing 6 changed files with 50 additions and 30 deletions.
diff --git a/CHANGES.md b/CHANGES.md
@@ -8,6 +8,7 @@ Only noting significant user-visible or major API changes, not internal code cle
 * [JENKINS-26860](https://issues.jenkins-ci.org/browse/JENKINS-26860): added _Execute concurrent builds if necessary_ option for Workflow projects.
 * [JENKINS-28756](https://issues.jenkins-ci.org/browse/JENKINS-28756): dropdown for _General SCM_ step incorrectly listed SCMs not compatible with Workflow.
 * [JENKINS-29221](https://issues.jenkins-ci.org/browse/JENKINS-29221): better robustness when the Groovy script is uncompilable.
+* [JENKINS-29571](https://issues.jenkins-ci.org/browse/JENKINS-29571): corrupt build record after renaming a job.
 
 ## 1.8 (Jun 01 2015)
 

diff --git a/aggregator/src/test/java/org/jenkinsci/plugins/workflow/WorkflowRunTest.java b/aggregator/src/test/java/org/jenkinsci/plugins/workflow/WorkflowRunTest.java
@@ -56,7 +56,6 @@
 import org.jenkinsci.plugins.workflow.test.steps.SemaphoreStep;
 import static org.junit.Assert.*;
 import org.junit.ClassRule;
-import org.junit.Ignore;
 import org.junit.Rule;
 import org.junit.Test;
 import org.jvnet.hudson.test.BuildWatcher;
@@ -231,7 +230,6 @@ public void failedToStartRun() throws Exception {
         r.assertBuildStatusSuccess(r.waitForCompletion(b));
     }
 
-    @Ignore("TODO as per JENKINS-27704, CpsFlowExecution.owner should be transient")
     @Issue("JENKINS-29571")
     @Test public void buildRecordAfterRename() throws Exception {
         {

diff --git a/api/src/main/java/org/jenkinsci/plugins/workflow/flow/FlowExecution.java b/api/src/main/java/org/jenkinsci/plugins/workflow/flow/FlowExecution.java
@@ -25,6 +25,7 @@
 package org.jenkinsci.plugins.workflow.flow;
 
 import com.google.common.util.concurrent.ListenableFuture;
+import hudson.Util;
 import hudson.model.Executor;
 import jenkins.model.CauseOfInterruption;
 import org.jenkinsci.plugins.workflow.actions.ErrorAction;
@@ -76,7 +77,16 @@ public abstract class FlowExecution implements FlowActionStorage {
     /**
      * Should be called by the flow owner after it is deserialized.
      */
-    public abstract void onLoad();
+    public /*abstract*/ void onLoad(FlowExecutionOwner owner) throws IOException {
+        if (Util.isOverridden(FlowExecution.class, getClass(), "onLoad")) {
+            onLoad();
+        }
+    }
+
+    @Deprecated
+    public void onLoad() {
+        throw new AbstractMethodError("you must implement the new overload of onLoad");
+    }
 
     public abstract FlowExecutionOwner getOwner();
 

diff --git a/cps/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java b/cps/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java
@@ -37,7 +37,6 @@
 import com.google.common.util.concurrent.ListenableFuture;
 import com.google.common.util.concurrent.SettableFuture;
 import com.thoughtworks.xstream.XStream;
-import com.thoughtworks.xstream.XStreamException;
 import com.thoughtworks.xstream.converters.Converter;
 import com.thoughtworks.xstream.converters.MarshallingContext;
 import com.thoughtworks.xstream.converters.UnmarshallingContext;
@@ -201,7 +200,7 @@ public class CpsFlowExecution extends FlowExecution {
     /*package*/ /*final*/ Map<String,String> loadedScripts = new HashMap<String, String>();
 
     private final boolean sandbox;
-    private /*almost final*/ FlowExecutionOwner owner;
+    private transient /*almost final*/ FlowExecutionOwner owner;
 
     /**
      * Loading of the program is asynchronous because it requires us to re-obtain stateful objects.
@@ -225,10 +224,12 @@ public class CpsFlowExecution extends FlowExecution {
      * Start nodes that have been created, whose {@link BlockEndNode} is not yet created.
      */
     @GuardedBy("this")
-    /*package*/ final Stack<BlockStartNode> startNodes = new Stack<BlockStartNode>();
+    /*package*/ /* almost final*/ Stack<BlockStartNode> startNodes = new Stack<BlockStartNode>();
+    private transient List<String> startNodesSerial; // used only between unmarshal and onLoad
 
     @GuardedBy("this")
-    private final NavigableMap<Integer,FlowHead> heads = new TreeMap();
+    private /* almost final*/ NavigableMap<Integer,FlowHead> heads = new TreeMap<Integer,FlowHead>();
+    private transient Map<Integer,String> headsSerial; // used only between unmarshal and onLoad
 
     private final AtomicInteger iota = new AtomicInteger();
 
@@ -383,7 +384,23 @@ public int iota() {
     }
 
     @Override
-    public void onLoad() {
+    public void onLoad(FlowExecutionOwner owner) throws IOException {
+        this.owner = owner;
+        storage = createStorage();
+        // heads could not be restored in unmarshal, so doing that now:
+        heads = new TreeMap<Integer,FlowHead>();
+        for (Map.Entry<Integer,String> entry : headsSerial.entrySet()) {
+            FlowHead h = new FlowHead(this, entry.getKey());
+            h.setForDeserialize(storage.getNode(entry.getValue()));
+            heads.put(h.getId(), h);
+        }
+        headsSerial = null;
+        // Same for startNodes:
+        startNodes = new Stack<BlockStartNode>();
+        for (String id : startNodesSerial) {
+            startNodes.add((BlockStartNode) storage.getNode(id));
+        }
+        startNodesSerial = null;
         try {
             if (!isComplete())
                 loadProgramAsync(getProgramDataFile());
@@ -766,12 +783,10 @@ void notifyListeners(FlowNode node) {
     // so that the execution gets suspended while we are getting serialized
 
     public static final class ConverterImpl implements Converter {
-        private final XStream xs;
         private final ReflectionProvider ref;
         private final Mapper mapper;
 
         public ConverterImpl(XStream xs) {
-            this.xs = xs;
             this.ref = xs.getReflectionProvider();
             this.mapper = xs.getMapper();
         }
@@ -786,7 +801,6 @@ public void marshal(Object source, HierarchicalStreamWriter w, MarshallingContex
             writeChild(w, context, "result", e.result, Result.class);
             writeChild(w, context, "script", e.script, String.class);
             writeChild(w, context, "loadedScripts", e.loadedScripts, Map.class);
-            writeChild(w, context, "owner", e.owner, Object.class);
             if (e.user != null) {
                 writeChild(w, context, "user", e.user, String.class);
             }
@@ -814,17 +828,15 @@ private <T> void writeChild(HierarchicalStreamWriter w, MarshallingContext conte
         }
 
         public Object unmarshal(HierarchicalStreamReader reader, final UnmarshallingContext context) {
-            try {
                 CpsFlowExecution result;
                 if (context.currentObject()!=null) {
                     result = (CpsFlowExecution) context.currentObject();
                 } else {
                     result = (CpsFlowExecution) ref.newInstance(CpsFlowExecution.class);
                 }
 
-                FlowNodeStorage storage = result.storage;
-                Stack<BlockStartNode> startNodes = new Stack<BlockStartNode>();
-                Map<Integer,FlowHead> heads = new TreeMap<Integer, FlowHead>();
+                result.startNodesSerial = new ArrayList<String>();
+                result.headsSerial = new TreeMap<Integer,String>();
 
                 while (reader.hasMoreChildren()) {
                     reader.moveDown();
@@ -843,39 +855,29 @@ public Object unmarshal(HierarchicalStreamReader reader, final UnmarshallingCont
                         setField(result, "loadedScripts", loadedScripts);
                     } else
                     if (nodeName.equals("owner")) {
-                        FlowExecutionOwner owner = (FlowExecutionOwner) readChild(reader, context, Object.class, result);
-                        setField(result, "owner", owner);
-                        setField(result, "storage", storage = result.createStorage());
+                        readChild(reader, context, Object.class, result); // for compatibility; discarded
                     } else
                     if (nodeName.equals("user")) {
                         String user = readChild(reader, context, String.class, result);
                         setField(result, "user", user);
                     } else
                     if (nodeName.equals("head")) {
                         String[] head = readChild(reader, context, String.class, result).split(":");
-                        FlowHead h = new FlowHead(result, Integer.parseInt(head[0]));
-                        h.setForDeserialize(storage.getNode(head[1]));
-                        heads.put(h.getId(), h);
+                        result.headsSerial.put(Integer.parseInt(head[0]), head[1]);
                     } else
                     if (nodeName.equals("iota")) {
                         Integer iota = readChild(reader, context, Integer.class, result);
                         setField(result, "iota", new AtomicInteger(iota));
                     } else
                     if (nodeName.equals("start")) {
                         String id = readChild(reader, context, String.class, result);
-                        startNodes.add((BlockStartNode) storage.getNode(id));
+                        result.startNodesSerial.add(id);
                     }
 
                     reader.moveUp();
                 }
 
-                setField(result, "startNodes", startNodes);
-                setField(result, "heads", heads);
-
                 return result;
-            } catch (IOException e) {
-                throw new XStreamException("Failed to read back CpsFlowExecution",e);
-            }
         }
 
         private void setField(CpsFlowExecution result, String fieldName, Object value) {

diff --git a/cps/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsFlowDefinition2Test.java b/cps/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsFlowDefinition2Test.java
@@ -24,6 +24,7 @@
 
 package org.jenkinsci.plugins.workflow.cps;
 
+import org.jenkinsci.plugins.workflow.flow.FlowExecutionOwner;
 import org.jenkinsci.plugins.workflow.test.steps.SemaphoreStep;
 import static org.junit.Assert.*;
 import org.junit.Test;
@@ -46,8 +47,9 @@ public class CpsFlowDefinition2Test extends AbstractCpsFlowTest {
 
         assertFalse("Expected the execution to be suspended but it has completed", exec.isComplete());
 
+        FlowExecutionOwner owner = exec.getOwner();
         exec = roundtripXStream(exec);    // poor man's simulation of Jenkins restart
-        exec.onLoad();
+        exec.onLoad(owner);
 
         // now resume workflow execution
         SemaphoreStep.success("watch/1", null);

diff --git a/job/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java b/job/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java
@@ -419,7 +419,14 @@ private String key() {
             System.err.printf("loading %s @%h%n", this, this);
         }
         if (execution != null) {
-            execution.onLoad();
+            try {
+                execution.onLoad(new Owner(this));
+            } catch (IOException x) {
+                LOGGER.log(Level.WARNING, null, x);
+                execution = null; // probably too broken to use
+            }
+        }
+        if (execution != null) {
             execution.addListener(new GraphL());
             executionPromise.set(execution);
             if (!execution.isComplete()) {