Merge pull request #77 from jglick/mapIterator-JENKINS-27421

[JENKINS-27421] Improve behavior of map iteration workaround a bit
jenkinsci · Oct 20, 2016 · a115a54 · a115a54
2 parents bee2879 + 33eddca
commit a115a54
Show file tree

Hide file tree

Showing 4 changed files with 17 additions and 15 deletions.
diff --git a/pom.xml b/pom.xml
@@ -89,7 +89,7 @@
         <dependency>
             <groupId>org.jenkins-ci.plugins</groupId>
             <artifactId>script-security</artifactId>
-            <version>1.23</version>
+            <version>1.24</version>
         </dependency>
         <dependency>
             <groupId>org.jenkins-ci.plugins</groupId>

diff --git a/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsThread.java b/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsThread.java
@@ -34,7 +34,6 @@
 import javax.annotation.CheckForNull;
 import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
-import java.io.IOException;
 import java.io.Serializable;
 import java.util.ArrayList;
 import java.util.List;
@@ -43,6 +42,7 @@
 
 import static java.util.logging.Level.*;
 import static org.jenkinsci.plugins.workflow.cps.persistence.PersistenceContext.*;
+import org.jenkinsci.plugins.workflow.support.concurrent.Futures;
 
 /**
  * Represents a {@link Continuable} that is either runnable or suspended (that waits for an
@@ -148,7 +148,7 @@ public StepExecution getStep() {
      * Executes CPS code synchronously a little bit more, until it hits
      * the point the workflow needs to be dehydrated.
      */
-    @Nonnull Outcome runNextChunk() throws IOException {
+    @Nonnull Outcome runNextChunk() {
         assert program!=null;
 
         Outcome outcome;
@@ -248,7 +248,9 @@ void fireCompletionHandlers(Outcome o) {
      *      Future that promises the completion of the next {@link #runNextChunk()}.
      */
     public Future<Object> resume(Outcome v) {
-        assert resumeValue==null;
+        if (resumeValue != null) {
+            return Futures.immediateFailedFuture(new IllegalStateException("Already resumed with " + resumeValue));
+        }
         resumeValue = v;
         promise = SettableFuture.create();
         group.scheduleRun();

diff --git a/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsThreadGroup.java b/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsThreadGroup.java
@@ -313,7 +313,7 @@ public boolean isPaused() {
      *      is akin to a Unix thread waiting for I/O completion.
      */
     @CpsVmThreadOnly("root")
-    private boolean run() throws IOException {
+    private boolean run() {
         boolean changed = false;
         boolean ending = false;
         boolean stillRunnable = false;
@@ -353,7 +353,11 @@ private boolean run() throws IOException {
         }
 
         if (changed) {
-            saveProgram();
+            try {
+                saveProgram();
+            } catch (IOException x) {
+                LOGGER.log(WARNING, "program state save failed", x);
+            }
         }
         if (ending) {
             execution.cleanUpHeap();
@@ -433,11 +437,9 @@ public void saveProgram(File f) throws IOException {
             Files.move(tmpFile.toPath(), f.toPath(), StandardCopyOption.ATOMIC_MOVE, StandardCopyOption.REPLACE_EXISTING);
             LOGGER.log(FINE, "program state saved");
         } catch (RuntimeException e) {
-            LOGGER.log(WARNING, "program state save failed",e);
             propagateErrorToWorkflow(e);
             throw new IOException("Failed to persist "+f,e);
         } catch (IOException e) {
-            LOGGER.log(WARNING, "program state save failed",e);
             propagateErrorToWorkflow(e);
             throw new IOException("Failed to persist "+f,e);
         } finally {

diff --git a/src/test/java/org/jenkinsci/plugins/workflow/SerializationTest.java b/src/test/java/org/jenkinsci/plugins/workflow/SerializationTest.java
@@ -3,7 +3,6 @@
 import groovy.lang.Closure;
 import hudson.model.Result;
 import hudson.slaves.DumbSlave;
-import org.jenkinsci.plugins.scriptsecurity.scripts.ScriptApproval;
 import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition;
 import org.jenkinsci.plugins.workflow.job.WorkflowJob;
 import org.jenkinsci.plugins.workflow.steps.AbstractStepDescriptorImpl;
@@ -186,17 +185,14 @@ public void stop(Throwable cause) throws Exception {
         });
     }
 
-    @Ignore("TODO java.io.NotSerializableException: java.util.LinkedHashMap$Entry")
     @Issue("JENKINS-27421")
     @Test public void mapIterator() {
         story.addStep(new Statement() {
             @Override public void evaluate() throws Throwable {
                 p = jenkins().createProject(WorkflowJob.class, "demo");
-                ScriptApproval.get().approveSignature("staticMethod org.codehaus.groovy.runtime.DefaultGroovyMethods collect java.util.Map groovy.lang.Closure");
-                ScriptApproval.get().approveSignature("method java.util.Map entrySet");
                 p.setDefinition(new CpsFlowDefinition(
                     "def map = [one: 1, two: 2]\n" +
-                    "@NonCPS def entries(m) {m.collect {k, v -> [k, v]}}; for (def e in entries(map)) {echo \"running flattened loop on ${e[0]} → ${e[1]}\"; semaphore \"C-${e[0]}\"}\n" +
+                    "@NonCPS def entrySet(m) {m.collect {k, v -> [key: k, value: v]}}; for (def e in entrySet(map)) {echo \"running flattened loop on ${e.key} → ${e.value}\"; semaphore \"C-${e.key}\"}\n" +
                     "for (def e : map.entrySet()) {echo \"running new-style loop on ${e.key} → ${e.value}\"; semaphore \"new-${e.key}\"}"
                     // TODO check also keySet(), values()
                     , true));
@@ -221,8 +217,12 @@ public void stop(Throwable cause) throws Exception {
                 SemaphoreStep.success("new-one/1", null);
                 SemaphoreStep.success("new-two/1", null);
                 story.j.waitForCompletion(b);
+                /* TODO desired behavior:
                 story.j.assertBuildStatusSuccess(b);
                 story.j.assertLogContains("running new-style loop on two → 2", b);
+                */
+                story.j.assertBuildStatus(Result.FAILURE, b);
+                story.j.assertLogContains("java.io.NotSerializableException: java.util.LinkedHashMap$Entry", b);
             }
         });
     }
@@ -289,8 +289,6 @@ public void stop(Throwable cause) throws Exception {
     @Test public void eachClosureNonCps() {
         story.addStep(new Statement() {
             @Override public void evaluate() throws Throwable {
-                ScriptApproval.get().approveSignature("staticMethod org.codehaus.groovy.runtime.DefaultGroovyMethods plus java.util.Collection java.lang.Object");
-                ScriptApproval.get().approveSignature("staticMethod org.codehaus.groovy.runtime.DefaultGroovyMethods plus java.util.List java.lang.Object"); // Groovy 2
                 p = jenkins().createProject(WorkflowJob.class, "demo");
                 p.setDefinition(new CpsFlowDefinition(
                     "@NonCPS def fine() {\n" +