[JENKINS-67237] BuildTrigger waits until the dependency graph has b…

…een updated (jenkinsci#6450)

core/src/main/java/hudson/tasks/BuildTrigger.java

@@ -59,6 +59,8 @@
 import java.util.Comparator;
 import java.util.List;
 import java.util.StringTokenizer;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.Future;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 import jenkins.model.DependencyDeclarer;
@@ -256,8 +258,23 @@ public static boolean execute(AbstractBuild build, BuildListener listener, Build
     public static boolean execute(AbstractBuild build, BuildListener listener) {
         PrintStream logger = listener.getLogger();
         // Check all downstream Project of the project, not just those defined by BuildTrigger
-        // TODO this may not yet be up to date if rebuildDependencyGraphAsync has been used; need a method to wait for the last call made before now to finish
-        final DependencyGraph graph = Jenkins.get().getDependencyGraph();
+
+        DependencyGraph graphTemp;
+        try {
+            // Note: futureDependencyGraph can be null, if no asynchronous computation of the
+            // dependency graph has been performed.
+            Future<DependencyGraph> futureDependencyGraph = Jenkins.get().getFutureDependencyGraph();
+            if (futureDependencyGraph != null) {
+                graphTemp = futureDependencyGraph.get();
+            } else {
+                graphTemp = Jenkins.get().getDependencyGraph();
+            }
+        } catch (IllegalStateException | InterruptedException | ExecutionException e) {
+            // Use old version of dependency graph instead
+            graphTemp = Jenkins.get().getDependencyGraph();
+        }
+        DependencyGraph graph = graphTemp;
+
         List<Dependency> downstreamProjects = new ArrayList<>(
                 graph.getDownstreamDependencies(build.getProject()));
         // Sort topologically

core/src/main/java/jenkins/model/Jenkins.java

@@ -237,6 +237,7 @@
 import java.util.TimerTask;
 import java.util.TreeMap;
 import java.util.TreeSet;
+import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.CopyOnWriteArrayList;
 import java.util.concurrent.CountDownLatch;
@@ -247,7 +248,6 @@
 import java.util.concurrent.ThreadPoolExecutor;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.TimeoutException;
-import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.function.Predicate;
 import java.util.logging.Level;
 import java.util.logging.LogRecord;
@@ -498,7 +498,9 @@ public class Jenkins extends AbstractCIBase implements DirectlyModifiableTopLeve
     private volatile List<JDK> jdks = new ArrayList<>();
 
     private transient volatile DependencyGraph dependencyGraph;
-    private final transient AtomicBoolean dependencyGraphDirty = new AtomicBoolean();
+    private transient Future<DependencyGraph> scheduledFutureDependencyGraph;
+    private transient Future<DependencyGraph> calculatingFutureDependencyGraph;
+    private transient Object dependencyGraphLock = new Object();
 
     /**
      * Currently active Views tab bar.
@@ -3579,6 +3581,8 @@ public void cleanUp() {
 
             final Set<Future<?>> pending = _cleanUpDisconnectComputers(errors);
 
+            _cleanUpCancelDependencyGraphCalculation();
+
             _cleanUpInterruptReloadThread(errors);
 
             _cleanUpShutdownTriggers(errors);
@@ -3929,6 +3933,18 @@ private void _cleanUpReleaseAllLoggers(List<Throwable> errors) {
         }
     }
 
+    private void _cleanUpCancelDependencyGraphCalculation() {
+        synchronized (dependencyGraphLock) {
+            LOGGER.log(Level.FINE, "Canceling internal dependency graph calculation");
+            if (scheduledFutureDependencyGraph != null && !scheduledFutureDependencyGraph.isDone()) {
+                scheduledFutureDependencyGraph.cancel(true);
+            }
+            if (calculatingFutureDependencyGraph != null && !calculatingFutureDependencyGraph.isDone()) {
+                calculatingFutureDependencyGraph.cancel(true);
+            }
+        }
+    }
+
     public Object getDynamic(String token) {
         for (Action a : getActions()) {
             String url = a.getUrlName();
@@ -4899,6 +4915,23 @@ public static boolean isCheckURIEncodingEnabled() {
         return ExtensionList.lookupSingleton(URICheckEncodingMonitor.class).isCheckEnabled();
     }
 
+    public Future<DependencyGraph> getFutureDependencyGraph() {
+        synchronized (dependencyGraphLock) {
+            // Scheduled future will be the most recent one --> Return
+            if (scheduledFutureDependencyGraph != null) {
+                return scheduledFutureDependencyGraph;
+            }
+
+            // Calculating future will be the most recent one --> Return
+            if (calculatingFutureDependencyGraph != null) {
+                return calculatingFutureDependencyGraph;
+            }
+
+            // No scheduled or calculating future --> Already completed dependency graph is the most recent one
+            return CompletableFuture.completedFuture(dependencyGraph);
+        }
+    }
+
     /**
      * Rebuilds the dependency map.
      */
@@ -4908,7 +4941,6 @@ public void rebuildDependencyGraph() {
         // volatile acts a as a memory barrier here and therefore guarantees
         // that graph is fully build, before it's visible to other threads
         dependencyGraph = graph;
-        dependencyGraphDirty.set(false);
     }
 
     /**
@@ -4921,13 +4953,44 @@ public void rebuildDependencyGraph() {
      * @since 1.522
      */
     public Future<DependencyGraph> rebuildDependencyGraphAsync() {
-        dependencyGraphDirty.set(true);
+        synchronized (dependencyGraphLock) {
+            // Collect calls to this method to avoid unnecessary calculation of the dependency graph
+            if (scheduledFutureDependencyGraph != null) {
+                return scheduledFutureDependencyGraph;
+            }
+            // Schedule new calculation
+            return scheduledFutureDependencyGraph = scheduleCalculationOfFutureDependencyGraph(500, TimeUnit.MILLISECONDS);
+        }
+    }
+
+    private Future<DependencyGraph> scheduleCalculationOfFutureDependencyGraph(int delay, TimeUnit unit) {
         return Timer.get().schedule(() -> {
-            if (dependencyGraphDirty.get()) {
-                rebuildDependencyGraph();
+            // Wait for the currently running calculation to finish without blocking rebuildDependencyGraphAsync()
+            Future<DependencyGraph> temp = null;
+            synchronized (dependencyGraphLock) {
+                if (calculatingFutureDependencyGraph != null) {
+                    temp = calculatingFutureDependencyGraph;
+                }
+            }
+
+            if (temp != null) {
+                temp.get();
             }
+
+            synchronized (dependencyGraphLock) {
+                // Scheduled future becomes the currently calculating future
+                calculatingFutureDependencyGraph = scheduledFutureDependencyGraph;
+                scheduledFutureDependencyGraph = null;
+            }
+
+            rebuildDependencyGraph();
+
+            synchronized (dependencyGraphLock) {
+                calculatingFutureDependencyGraph = null;
+            }
+
             return dependencyGraph;
-        }, 500, TimeUnit.MILLISECONDS);
+        }, delay, unit);
     }
 
     public DependencyGraph getDependencyGraph() {

test/src/test/java/jenkins/model/JenkinsFutureDependencyGraphTest.java

@@ -0,0 +1,145 @@
+package jenkins.model;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.not;
+import static org.mockito.Mockito.doAnswer;
+import static org.mockito.Mockito.spy;
+
+import hudson.model.DependencyGraph;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.Future;
+import org.junit.Rule;
+import org.junit.Test;
+import org.jvnet.hudson.test.Issue;
+import org.jvnet.hudson.test.JenkinsRule;
+import org.mockito.invocation.InvocationOnMock;
+import org.mockito.stubbing.Answer;
+
+public class JenkinsFutureDependencyGraphTest {
+
+    @Rule
+    public JenkinsRule j = new JenkinsRule();
+
+    @Issue("JENKINS-67237")
+    @Test
+    public void testGetFutureDependencyGraphWithoutASingleRebuildBeforeHand() throws InterruptedException, ExecutionException {
+        Jenkins jenkins = j.jenkins;
+
+        DependencyGraph resultingGraph = jenkins.getFutureDependencyGraph().get();
+        // If no dependency graph was calculated asynchronously, jenkins should return the synchronously calculated dependency graph.
+        assertThat("The asynchronously calculated dependency graph should be equal to the synchronously calculated dependency graph but wasn't.", resultingGraph, is(jenkins.getDependencyGraph()));
+    }
+
+    @Issue("JENKINS-67237")
+    @Test
+    public void testStartRebuildOfDependecyGraphWhileScheduled() throws InterruptedException, ExecutionException {
+        Jenkins jenkins = j.jenkins;
+
+        Future<DependencyGraph> firstFutureDependencyGraph = jenkins.rebuildDependencyGraphAsync();
+        Future<DependencyGraph> secondFutureDependencyGraph = jenkins.rebuildDependencyGraphAsync();
+
+        assertThat("Two future dependency graphs that were scheduled in short succession should be equal, but weren't", firstFutureDependencyGraph, is(secondFutureDependencyGraph));
+        assertThat("Last scheduled future dependency graph should have been returned, but wasn't.", secondFutureDependencyGraph, is(jenkins.getFutureDependencyGraph()));
+    }
+
+    @Issue("JENKINS-67237")
+    @Test
+    public void testStartRebuildOfDependencyGraphWhileAlreadyRebuilding() throws InterruptedException, ExecutionException {
+        RebuildDependencyGraphController rebuildDependencyGraphController = new RebuildDependencyGraphController();
+        Jenkins jenkins = mockJenkinsWithControllableDependencyGraph(rebuildDependencyGraphController);
+
+        Future<DependencyGraph> firstFutureDependencyGraph = jenkins.rebuildDependencyGraphAsync();
+        // Wait until rebuild has started
+        while (rebuildDependencyGraphController.getNumberOfStartedBuilds() < 1) {
+            Thread.sleep(500);
+        }
+
+        Future<DependencyGraph> secondFutureDependencyGraph = jenkins.rebuildDependencyGraphAsync();
+
+        assertThat("Starting a new rebuild of the dependency graph while already rebuilding should result in two distinct future dependency graphs, but didn't.", firstFutureDependencyGraph, is(not(secondFutureDependencyGraph)));
+
+        rebuildDependencyGraphController.setLetBuildFinish(true);
+        // Wait for both builds to complete
+        firstFutureDependencyGraph.get();
+        secondFutureDependencyGraph.get();
+
+        assertThat("Two dependency graphs should have been built, but weren't.", rebuildDependencyGraphController.getNumberOfFinishedBuilds(), is(2));
+    }
+
+    @Issue("JENKINS-67237")
+    @Test
+    public void testStartRebuildOfDependencyGraphWhileAlreadyRebuildingAndAnotherOneScheduled() throws InterruptedException, ExecutionException {
+        RebuildDependencyGraphController rebuildDependencyGraphController = new RebuildDependencyGraphController();
+        Jenkins jenkins = mockJenkinsWithControllableDependencyGraph(rebuildDependencyGraphController);
+
+        jenkins.rebuildDependencyGraphAsync();
+        // Wait until rebuild has started
+        while (rebuildDependencyGraphController.getNumberOfStartedBuilds() < 1) {
+            Thread.sleep(500);
+        }
+
+        Future<DependencyGraph> secondFutureDependencyGraph = jenkins.rebuildDependencyGraphAsync();
+        Future<DependencyGraph> thirdFutureDependencyGraph = jenkins.rebuildDependencyGraphAsync();
+
+        assertThat("Two future dependency graphs that were scheduled in short succession should be equal, but weren't", secondFutureDependencyGraph, is(thirdFutureDependencyGraph));
+        assertThat("Last scheduled future dependency graph should have been returned, but wasn't.", jenkins.getFutureDependencyGraph(), is(thirdFutureDependencyGraph));
+
+        rebuildDependencyGraphController.setLetBuildFinish(true);
+        // Wait for builds to complete
+        thirdFutureDependencyGraph.get();
+
+        assertThat("Two dependency graphs should have been built, but weren't.", rebuildDependencyGraphController.getNumberOfFinishedBuilds(), is(2));
+    }
+
+    private Jenkins mockJenkinsWithControllableDependencyGraph(RebuildDependencyGraphController rebuildDependencyGraphController) {
+        Jenkins mockedJenkins = spy(j.jenkins);
+        doAnswer(new Answer<Void>() {
+            @Override
+            public Void answer(InvocationOnMock invocation) throws Throwable {
+
+                rebuildDependencyGraphController.increaseNumberOfStartedBuilds();
+                if (!rebuildDependencyGraphController.isLetBuildFinish()) {
+                    // NOOP
+                }
+                invocation.callRealMethod();
+
+                rebuildDependencyGraphController.increaseNumberOfFinishedBuilds();
+                return null;
+            }
+        }).when(mockedJenkins).rebuildDependencyGraph();
+
+        return mockedJenkins;
+    }
+
+    class RebuildDependencyGraphController {
+
+        private volatile boolean letBuildFinish = false;
+        private volatile int numberOfStartedBuilds = 0;
+        private volatile int numberOfFinishedBuilds = 0;
+
+        public boolean isLetBuildFinish() {
+            return letBuildFinish;
+        }
+
+        public void setLetBuildFinish(boolean letBuildFinish) {
+            this.letBuildFinish = letBuildFinish;
+        }
+
+        public int getNumberOfStartedBuilds() {
+            return numberOfStartedBuilds;
+        }
+
+        public void increaseNumberOfStartedBuilds() {
+            this.numberOfStartedBuilds++;
+        }
+
+        public int getNumberOfFinishedBuilds() {
+            return numberOfFinishedBuilds;
+        }
+
+        public void increaseNumberOfFinishedBuilds() {
+            this.numberOfFinishedBuilds++;
+        }
+    }
+}

test/src/test/java/jenkins/triggers/ReverseBuildTriggerAfterRestartTest.java

@@ -0,0 +1,38 @@
+package jenkins.triggers;
+
+import static org.junit.Assert.assertNotNull;
+
+import hudson.model.FreeStyleProject;
+import org.junit.Rule;
+import org.junit.Test;
+import org.jvnet.hudson.test.Issue;
+import org.jvnet.hudson.test.JenkinsSessionRule;
+
+public class ReverseBuildTriggerAfterRestartTest {
+
+    @Rule
+    public JenkinsSessionRule rule = new JenkinsSessionRule();
+
+    @Issue("JENKINS-67237")
+    @Test
+    public void testExecutionOfReverseBuildTriggersAfterRestart() throws Throwable {
+        String nameOfUpstreamProject = "upstreamProject";
+        String nameOfDownstreamProject = "downstreamProject";
+
+        rule.then(j -> {
+            j.createFreeStyleProject(nameOfUpstreamProject);
+            FreeStyleProject downstreamProject = j.createFreeStyleProject(nameOfDownstreamProject);
+            downstreamProject.addTrigger(new ReverseBuildTrigger(nameOfUpstreamProject));
+            downstreamProject.save();
+        });
+
+        rule.then(j -> {
+            FreeStyleProject upstreamProject = j.jenkins.getItem(nameOfUpstreamProject, j.jenkins, FreeStyleProject.class);
+            j.buildAndAssertSuccess(upstreamProject);
+            j.waitUntilNoActivity();
+
+            FreeStyleProject downstreamProject = j.jenkins.getItem(nameOfDownstreamProject, j.jenkins, FreeStyleProject.class);
+            assertNotNull(downstreamProject.getLastBuild());
+        });
+    }
+}