[JENKINS-45327] Pickle EnvActionImpl to prevent multiple instances fr…

…om existing after resumption (#539) * [JENKINS-45327] Pickle EnvActionImpl to prevent multiple instances from existing after resumption * [JENKINS-45327] Simplify impl and test based on review feedback
jenkinsci · May 26, 2022 · 71dd22b · 71dd22b
1 parent 0449852
commit 71dd22b
Show file tree

Hide file tree

Showing 2 changed files with 53 additions and 2 deletions.
diff --git a/src/main/java/org/jenkinsci/plugins/workflow/cps/EnvActionImpl.java b/src/main/java/org/jenkinsci/plugins/workflow/cps/EnvActionImpl.java
@@ -24,6 +24,8 @@
 
 package org.jenkinsci.plugins.workflow.cps;
 
+import com.google.common.util.concurrent.Futures;
+import com.google.common.util.concurrent.ListenableFuture;
 import groovy.lang.GroovyObjectSupport;
 import hudson.EnvVars;
 import hudson.Extension;
@@ -32,28 +34,30 @@
 import hudson.model.TaskListener;
 import hudson.util.LogTaskListener;
 import java.io.IOException;
-import java.io.Serializable;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.Map;
 import java.util.TreeMap;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 import edu.umd.cs.findbugs.annotations.NonNull;
+import hudson.model.Queue;
 import jenkins.model.RunAction2;
 import org.jenkinsci.plugins.workflow.flow.FlowCopier;
 import org.jenkinsci.plugins.workflow.flow.FlowExecution;
 import org.jenkinsci.plugins.workflow.flow.FlowExecutionOwner;
 import org.jenkinsci.plugins.workflow.graph.FlowNode;
+import org.jenkinsci.plugins.workflow.pickles.Pickle;
 import org.jenkinsci.plugins.workflow.steps.EnvironmentExpander;
 import org.jenkinsci.plugins.workflow.support.actions.EnvironmentAction;
+import org.jenkinsci.plugins.workflow.support.pickles.SingleTypedPickleFactory;
 import org.kohsuke.accmod.Restricted;
 import org.kohsuke.accmod.restrictions.DoNotUse;
 import org.kohsuke.stapler.export.Exported;
 import org.kohsuke.stapler.export.ExportedBean;
 
 @ExportedBean
-public class EnvActionImpl extends GroovyObjectSupport implements EnvironmentAction.IncludingOverrides, Serializable, RunAction2 {
+public class EnvActionImpl extends GroovyObjectSupport implements EnvironmentAction.IncludingOverrides, RunAction2 {
 
     private static final Logger LOGGER = Logger.getLogger(EnvActionImpl.class.getName());
     private static final long serialVersionUID = 1;
@@ -199,4 +203,30 @@ public Collection<EnvironmentContributor> getEnvironmentContributors() {
 
     }
 
+    @Extension public static class EnvActionImplPickleFactory extends SingleTypedPickleFactory<EnvActionImpl> {
+        @Override
+        protected Pickle pickle(EnvActionImpl object) {
+            return new EnvActionImplPickle();
+        }
+    }
+
+    /**
+     * Prevents multiple instances of {@link EnvActionImpl} from existing for a single Pipeline after a Jenkins restart
+     * in case {@code env} is serialized into the program.
+     */
+    private static class EnvActionImplPickle extends Pickle {
+        @Override
+        public ListenableFuture<?> rehydrate(FlowExecutionOwner owner) {
+            try {
+                Queue.Executable executable = owner.getExecutable();
+                if (executable instanceof Run) {
+                    return Futures.immediateFuture(EnvActionImpl.forRun((Run)executable));
+                } else {
+                    return Futures.immediateFailedFuture(new IllegalStateException("Invalid executable: " + executable));
+                }
+            } catch (IOException e) {
+                return Futures.immediateFailedFuture(e);
+            }
+        }
+    }
 }
diff --git a/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecutionTest.java b/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecutionTest.java
@@ -63,6 +63,7 @@
 import org.jenkinsci.plugins.workflow.test.steps.SemaphoreStep;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.hasItem;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
@@ -527,4 +528,24 @@ public boolean isAllowed(String groovyResourceUrl) {
             return groovyResourceUrl.endsWith("/trusted/foo.groovy");
         }
     }
+
+    @Issue("JENKINS-45327")
+    @Test public void envActionImplPickle() throws Throwable {
+        sessions.then(r -> {
+            WorkflowJob p = r.createProject(WorkflowJob.class, "p");
+            p.setDefinition(new CpsFlowDefinition(
+                    "def e = env\n" +
+                    "semaphore('wait')\n" + // An instance of EnvActionImpl is part of the program's state at this point.
+                    "e.foo = 'bar'\n", true)); // Without EnvActionImplPickle, this throws an NPE in EnvActionImpl.setProperty because owner is null.
+            WorkflowRun b = p.scheduleBuild2(0).waitForStart();
+            SemaphoreStep.waitForStart("wait/1", b);
+        });
+        sessions.then(r -> {
+            WorkflowJob p = r.jenkins.getItemByFullName("p", WorkflowJob.class);
+            WorkflowRun b = p.getLastBuild();
+            SemaphoreStep.success("wait/1", null);
+            r.assertBuildStatus(Result.SUCCESS, r.waitForCompletion(b));
+            assertThat(EnvActionImpl.forRun(b).getEnvironment().get("foo"), equalTo("bar"));
+        });
+    }
 }