Merge branch 'master' into CpsVmExecutorService.shutdown

Author: jglick

pom.xml

@@ -75,7 +75,7 @@
             <dependency>
                 <groupId>io.jenkins.tools.bom</groupId>
                 <artifactId>bom-2.332.x</artifactId>
-                <version>1289.v5c4b_1c43511b_</version>
+                <version>1382.v7d694476f340</version>
                 <scope>import</scope>
                 <type>pom</type>
             </dependency>
@@ -89,6 +89,7 @@
         <dependency>
             <groupId>org.jenkins-ci.plugins.workflow</groupId>
             <artifactId>workflow-api</artifactId>
+            <version>1162.va_1e49062a_00e</version>
         </dependency>
         <dependency>
             <groupId>org.jenkins-ci.plugins.workflow</groupId>

src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java

@@ -52,6 +52,7 @@
 import hudson.model.Action;
 import hudson.model.Result;
 import hudson.util.Iterators;
+import io.jenkins.lib.versionnumber.JavaSpecificationVersion;
 import jenkins.model.CauseOfInterruption;
 import jenkins.model.Jenkins;
 import org.jboss.marshalling.Unmarshaller;
@@ -276,6 +277,13 @@ public class CpsFlowExecution extends FlowExecution implements BlockableResume {
 
     boolean resumeBlocked = false;
 
+    /**
+     * Whether {@link CpsThreadGroup#isPaused} when loaded from disk.
+     * @see #loadProgramAsync
+     * @see #afterStepExecutionsResumed
+     */
+    private transient boolean pausedWhenLoaded;
+
     /** Subdirectory string where we store {@link FlowNode}s */
     private String storageDir = null;
 
@@ -774,17 +782,8 @@ public void onSuccess(Unmarshaller u) {
                             try {
                                 CpsThreadGroup g = (CpsThreadGroup) u.readObject();
                                 result.set(g);
-                                try {
-                                    if (g.isPaused()) {
-                                        owner.getListener().getLogger().println("Still paused");
-                                    } else {
-                                        owner.getListener().getLogger().println("Ready to run at " + new Date());
-                                        // In case we last paused execution due to Jenkins.isQuietingDown, make sure we do something after we restart.
-                                        g.scheduleRun();
-                                    }
-                                } catch (IOException x) {
-                                    LOGGER.log(Level.WARNING, null, x);
-                                }
+                                pausedWhenLoaded = g.isPaused();
+                                g.pause();
                             } catch (Throwable t) {
                                 onFailure(t);
                             } finally {
@@ -870,6 +869,28 @@ void croak(Throwable t) {
         }
     }
 
+    @Override protected void afterStepExecutionsResumed() {
+        runInCpsVmThread(new FutureCallback<CpsThreadGroup>() {
+            @Override public void onSuccess(CpsThreadGroup g) {
+                try {
+                    if (pausedWhenLoaded) {
+                        owner.getListener().getLogger().println("Still paused");
+                    } else {
+                        owner.getListener().getLogger().println("Ready to run at " + new Date());
+                        // In case we last paused execution due to Jenkins.isQuietingDown, make sure we do something after we restart.
+                        g.unpause();
+                        g.saveProgramIfPossible(false); // ensure pausedWhenLoaded=false is persisted
+                    }
+                } catch (IOException x) {
+                    LOGGER.log(Level.WARNING, null, x);
+                }
+            }
+            @Override public void onFailure(Throwable t) {
+                LOGGER.log(Level.WARNING, "could not resume " + this, t);
+            }
+        });
+    }
+
     /**
      * Where we store {@link CpsThreadGroup}.
      */
@@ -1298,6 +1319,7 @@ private static void cleanUpLoader(ClassLoader loader, Set<ClassLoader> encounter
             if (encounteredClasses.add(clazz)) {
                 LOGGER.log(Level.FINER, "found {0}", clazz.getName());
                 Introspector.flushFromCaches(clazz);
+                cleanUpClassInfoCache(clazz);
                 cleanUpGlobalClassSet(clazz);
                 cleanUpClassHelperCache(clazz);
                 cleanUpObjectStreamClassCaches(clazz);
@@ -1358,6 +1380,44 @@ private static void cleanUpGlobalClassValue(@NonNull ClassLoader loader) throws
         }
     }
 
+    private static void cleanUpClassInfoCache(Class<?> clazz) {
+        JavaSpecificationVersion current = JavaSpecificationVersion.forCurrentJVM();
+        if (current.isNewerThan(new JavaSpecificationVersion("1.8"))
+                && current.isOlderThan(new JavaSpecificationVersion("16"))) {
+            try {
+                // TODO Work around JDK-8231454.
+                Class<?> classInfoC = Class.forName("com.sun.beans.introspect.ClassInfo");
+                Field cacheF = classInfoC.getDeclaredField("CACHE");
+                try {
+                    cacheF.setAccessible(true);
+                } catch (RuntimeException e) { // TODO Java 9+ InaccessibleObjectException
+                    /*
+                     * Not running with "--add-opens java.desktop/com.sun.beans.introspect=ALL-UNNAMED".
+                     * Until core adds this to its --add-opens configuration, and until that core
+                     * change is widely adopted, avoid unnecessary log spam and return early.
+                     */
+                    if (LOGGER.isLoggable(Level.FINER)) {
+                        LOGGER.log(Level.FINER, "Failed to clean up " + clazz.getName() + " from ClassInfo#CACHE. A metaspace leak may have occurred.", e);
+                    }
+                    return;
+                }
+                Object cache = cacheF.get(null);
+                Class<?> cacheC = Class.forName("com.sun.beans.util.Cache");
+                if (LOGGER.isLoggable(Level.FINER)) {
+                    LOGGER.log(Level.FINER, "Cleaning up " + clazz.getName() + " from ClassInfo#CACHE.");
+                }
+                Method removeM = cacheC.getMethod("remove", Object.class);
+                removeM.invoke(cache, clazz);
+            } catch (ReflectiveOperationException e) {
+                /*
+                 * Should never happen, but if it does, ensure the failure is isolated to this
+                 * method and does not prevent other cleanup logic from executing.
+                 */
+                LOGGER.log(Level.WARNING, "Failed to clean up " + clazz.getName() + " from ClassInfo#CACHE. A metaspace leak may have occurred.", e);
+            }
+        }
+    }
+
     private static void cleanUpGlobalClassSet(@NonNull Class<?> clazz) throws Exception {
         Class<?> classInfoC = Class.forName("org.codehaus.groovy.reflection.ClassInfo"); // or just ClassInfo.class, but unclear whether this will always be there
         Field globalClassSetF = classInfoC.getDeclaredField("globalClassSet");
@@ -1405,13 +1465,16 @@ private static void cleanUpObjectStreamClassCaches(@NonNull Class<?> clazz) thro
         for (String cacheFName : new String[] {"localDescs", "reflectors"}) {
             Field cacheF = cachesC.getDeclaredField(cacheFName);
             cacheF.setAccessible(true);
-            ConcurrentMap<Reference<Class<?>>, ?> cache = (ConcurrentMap) cacheF.get(null);
-            Iterator<? extends Entry<Reference<Class<?>>, ?>> iterator = cache.entrySet().iterator();
-            while (iterator.hasNext()) {
-                if (iterator.next().getKey().get() == clazz) {
-                    iterator.remove();
-                    LOGGER.log(Level.FINER, "cleaning up {0} from ObjectStreamClass.Caches.{1}", new Object[] {clazz.getName(), cacheFName});
-                    break;
+            Object cache = cacheF.get(null);
+            if (cache instanceof ConcurrentMap) {
+                // Prior to JDK-8277072
+                Iterator<? extends Entry<Reference<Class<?>>, ?>> iterator = ((ConcurrentMap) cache).entrySet().iterator();
+                while (iterator.hasNext()) {
+                    if (iterator.next().getKey().get() == clazz) {
+                        iterator.remove();
+                        LOGGER.log(Level.FINER, "cleaning up {0} from ObjectStreamClass.Caches.{1}", new Object[]{clazz.getName(), cacheFName});
+                        break;
+                    }
                 }
             }
         }

src/main/java/org/jenkinsci/plugins/workflow/cps/CpsStepContext.java

@@ -30,7 +30,6 @@
 import com.google.common.util.concurrent.SettableFuture;
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import groovy.lang.Closure;
-import hudson.Main;
 import hudson.model.Descriptor;
 import hudson.model.Result;
 import hudson.util.DaemonThreadFactory;
@@ -66,6 +65,7 @@
 import java.util.function.Function;
 import java.util.logging.Level;
 import java.util.logging.Logger;
+import java.util.stream.Stream;
 import jenkins.model.CauseOfInterruption;
 import jenkins.util.ContextResettingExecutorService;
 import org.codehaus.groovy.runtime.InvokerInvocationException;
@@ -328,39 +328,33 @@ private void completed(@NonNull Outcome newOutcome) {
             whenOutcomeDelivered = new Throwable();
         } else {
             Throwable failure = newOutcome.getAbnormal();
-            Level level;
-            if (Main.isUnitTest) {
-                if (failure instanceof FlowInterruptedException && ((FlowInterruptedException) failure).getCauses().stream().anyMatch(BodyFailed.class::isInstance)) {
-                    // Very common and generally uninteresting.
-                    level = Level.FINE;
-                } else {
-                    // Possibly a minor bug.
-                    level = Level.INFO;
-                }
-            } else {
-                // Typically harmless; do not alarm users.
-                level = Level.FINE;
-            }
-            if (LOGGER.isLoggable(level)) {
-                LOGGER.log(level, "already completed " + this, new IllegalStateException("delivered here"));
+            Throwable earlierFailure = outcome.getAbnormal();
+            if (LOGGER.isLoggable(Level.FINE)) {
+                LOGGER.log(Level.FINE, "already completed " + this, new IllegalStateException("delivered here"));
                 if (failure != null) {
-                    LOGGER.log(level, "new failure", failure);
+                    LOGGER.log(Level.FINE, "new failure", failure);
                 } else {
-                    LOGGER.log(level, "new success: {0}", outcome.getNormal());
+                    LOGGER.log(Level.FINE, "new success: {0}", outcome.getNormal());
                 }
                 if (whenOutcomeDelivered != null) {
-                    LOGGER.log(level, "previously delivered here", whenOutcomeDelivered);
+                    LOGGER.log(Level.FINE, "previously delivered here", whenOutcomeDelivered);
                 }
-                Throwable earlierFailure = outcome.getAbnormal();
                 if (earlierFailure != null) {
-                    LOGGER.log(level, "earlier failure", earlierFailure);
+                    LOGGER.log(Level.FINE, "earlier failure", earlierFailure);
                 } else {
-                    LOGGER.log(level, "earlier success: {0}", outcome.getNormal());
+                    LOGGER.log(Level.FINE, "earlier success: {0}", outcome.getNormal());
                 }
             }
+            if (failure != null && earlierFailure != null && !refersTo(failure, earlierFailure)) {
+                earlierFailure.addSuppressed(failure);
+            }
         }
     }
 
+    private static boolean refersTo(Throwable t1, Throwable t2) {
+        return t1 == t2 || t1.getCause() != null && refersTo(t1.getCause(), t2) || Stream.of(t1.getSuppressed()).anyMatch(t3 -> refersTo(t3, t2));
+    }
+
     /**
      * When this step context has completed execution (successful or otherwise), plan the next action.
      */

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);
+            }
+        }
+    }
 }

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"));
+        });
+    }
 }