diff --git a/core/src/main/java/hudson/tasks/BuildTrigger.java b/core/src/main/java/hudson/tasks/BuildTrigger.java index 9325514e9d59..08c3a147f8d7 100644 --- a/core/src/main/java/hudson/tasks/BuildTrigger.java +++ b/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 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 downstreamProjects = new ArrayList<>( graph.getDownstreamDependencies(build.getProject())); // Sort topologically diff --git a/core/src/main/java/jenkins/model/Jenkins.java b/core/src/main/java/jenkins/model/Jenkins.java index 94f4b068c362..3aa50e0882a9 100644 --- a/core/src/main/java/jenkins/model/Jenkins.java +++ b/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 jdks = new ArrayList<>(); private transient volatile DependencyGraph dependencyGraph; - private final transient AtomicBoolean dependencyGraphDirty = new AtomicBoolean(); + private transient Future scheduledFutureDependencyGraph; + private transient Future calculatingFutureDependencyGraph; + private transient Object dependencyGraphLock = new Object(); /** * Currently active Views tab bar. @@ -3579,6 +3581,8 @@ public void cleanUp() { final Set> pending = _cleanUpDisconnectComputers(errors); + _cleanUpCancelDependencyGraphCalculation(); + _cleanUpInterruptReloadThread(errors); _cleanUpShutdownTriggers(errors); @@ -3929,6 +3933,18 @@ private void _cleanUpReleaseAllLoggers(List 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 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 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 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 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() { diff --git a/test/src/test/java/jenkins/model/JenkinsFutureDependencyGraphTest.java b/test/src/test/java/jenkins/model/JenkinsFutureDependencyGraphTest.java new file mode 100644 index 000000000000..0c006e529247 --- /dev/null +++ b/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 firstFutureDependencyGraph = jenkins.rebuildDependencyGraphAsync(); + Future 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 firstFutureDependencyGraph = jenkins.rebuildDependencyGraphAsync(); + // Wait until rebuild has started + while (rebuildDependencyGraphController.getNumberOfStartedBuilds() < 1) { + Thread.sleep(500); + } + + Future 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 secondFutureDependencyGraph = jenkins.rebuildDependencyGraphAsync(); + Future 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() { + @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++; + } + } +} diff --git a/test/src/test/java/jenkins/triggers/ReverseBuildTriggerAfterRestartTest.java b/test/src/test/java/jenkins/triggers/ReverseBuildTriggerAfterRestartTest.java new file mode 100644 index 000000000000..5694bc570030 --- /dev/null +++ b/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()); + }); + } +}