From d50564dbf6baac312d6d28086f57fa21ba2d9ad3 Mon Sep 17 00:00:00 2001 From: Georg Bremer Date: Tue, 22 Oct 2019 11:37:45 +0000 Subject: [PATCH] Always unblock executors waiting for dynamic trigger configuration Executors are waiting for the project to be ready. This is done so the list of dynamic trigger projects has finished loading. To ensure the tasks are not waiting forever, the list must always be set and the latch released, even when the job is disabled or the trigger was disabled, fetching of the trigger list failed or similar. Added two new tests, one for failing to fetch the trigger file, one for disabled job. Signed-off-by: Georg Bremer --- .../trigger/hudsontrigger/GerritTrigger.java | 22 ++- .../hudsontrigger/GerritTriggerTimerTask.java | 8 +- .../hudsontrigger/GerritTriggerTest.java | 161 ++++++++++++++++++ 3 files changed, 176 insertions(+), 15 deletions(-) diff --git a/src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/GerritTrigger.java b/src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/GerritTrigger.java index ac4122efd..97f82d446 100644 --- a/src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/GerritTrigger.java +++ b/src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/GerritTrigger.java @@ -1754,14 +1754,11 @@ public void updateTriggerConfigURL() { } triggerInformationAction.setErrorMessage(""); try { - dynamicGerritProjects = DynamicConfigurationCacheProxy.getInstance().fetchThroughCache(triggerConfigURL); - - // Now that the dynamic project list has been loaded, we can "count down" - // the latch so that the EventListener thread can begin to process events. - if (projectListIsReady.getCount() > 0) { - logger.debug("Trigger config URL updated: {}; latch is currently {}; decrementing it.", job.getName(), - projectListIsReady.getCount()); - projectListIsReady.countDown(); + // Check if dynamic trigger was disabled in the meantime + if (!dynamicTriggerConfiguration || job == null || !job.isBuildable()) { + dynamicGerritProjects = Collections.emptyList(); + } else { + dynamicGerritProjects = DynamicConfigurationCacheProxy.getInstance().fetchThroughCache(triggerConfigURL); } } catch (ParseException pe) { String logErrorMessage = MessageFormat.format( @@ -1796,6 +1793,15 @@ public void updateTriggerConfigURL() { String triggerInformationMessage = MessageFormat.format( "IOException when fetching dynamic trigger url: {0}", ioe.getMessage()); triggerInformationAction.setErrorMessage(triggerInformationMessage); + } finally { + // Now that the dynamic project list has been loaded, we can "count down" + // the latch so that the EventListener thread can begin to process events. + if (projectListIsReady.getCount() > 0) { + logger.debug("Trigger config URL updated: {}; latch is currently {}; decrementing it.", job.getName(), + projectListIsReady.getCount()); + } + // Always release all locks otherwise workers will be stuck forever + projectListIsReady.countDown(); } } diff --git a/src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/GerritTriggerTimerTask.java b/src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/GerritTriggerTimerTask.java index 7a967291d..6a05ffd62 100644 --- a/src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/GerritTriggerTimerTask.java +++ b/src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/GerritTriggerTimerTask.java @@ -29,7 +29,6 @@ import javax.annotation.Nonnull; import jenkins.model.Jenkins; -import org.apache.commons.lang.StringUtils; /** * TimerTasks that are created from a GerritTrigger and periodically calls @@ -60,12 +59,7 @@ public void run() { if (trigger == null) { return; } - if (StringUtils.isEmpty(trigger.getTriggerConfigURL())) { - return; - } - if (trigger.getJob() != null && !trigger.getJob().isBuildable()) { - return; - } + // Do not skip updates since tasks might wait for the update trigger.updateTriggerConfigURL(); } diff --git a/src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/GerritTriggerTest.java b/src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/GerritTriggerTest.java index 97e06d0c0..5507c6982 100644 --- a/src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/GerritTriggerTest.java +++ b/src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/GerritTriggerTest.java @@ -99,6 +99,7 @@ import java.util.GregorianCalendar; import java.util.LinkedList; import java.util.List; +import java.util.concurrent.CountDownLatch; //CS IGNORE LineLength FOR NEXT 11 LINES. REASON: static imports can get long import static com.sonyericsson.hudson.plugins.gerrit.trigger.hudsontrigger.GerritTriggerParameters.GERRIT_CHANGE_COMMIT_MESSAGE; @@ -1888,6 +1889,166 @@ public void testDynamicTriggerConfigurationTimeGap() throws Exception { java.nio.file.Files.delete(temporaryConfigFile); } + /** + * Tests that dynamic project configurations for disabled jobs do not block the event listener. + * @throws Exception on failure + */ + @PrepareForTest({ + GerritTrigger.class, + AbstractProject.class, + ToGerritRunListener.class, + PluginImpl.class, + Hudson.class, + Jenkins.class, + DependencyQueueTaskDispatcher.class, + EventListener.class }) + @Test + public void testDynamicTriggerDoesNotBlockForDisabledJobs() throws Exception { + AbstractProject project = PowerMockito.mock(AbstractProject.class); + when(project.getFullName()).thenReturn("MockedProject"); + when(project.isBuildable()).thenReturn(false); + + Queue queue = mockConfig(project); + + PowerMockito.mockStatic(ToGerritRunListener.class); + ToGerritRunListener listener = PowerMockito.mock(ToGerritRunListener.class); + PowerMockito.when(ToGerritRunListener.getInstance()).thenReturn(listener); + + GerritTrigger trigger = new GerritTrigger(null); + trigger.setDynamicTriggerConfiguration(true); + trigger.setTriggerConfigURL("url"); + + // Set up the job within the trigger. + Whitebox.setInternalState(trigger, "job", project); + + // We need to make sure that whenever anyone asks for the trigger for any job + // that it returns this one. + PowerMockito.mockStatic(GerritTrigger.class); + when(GerritTrigger.getTrigger(any(Job.class))).thenReturn(trigger); + assertEquals(trigger, GerritTrigger.getTrigger(project)); + + // Because the "stub" methodology doesn't work, we also need to manually replace the "createListener" + // static method. + EventListener myListener = new EventListener(project); + PowerMockito.when(GerritTrigger.createListener(any(Job.class))).thenReturn(myListener); + assertNotNull(trigger.createListener()); + + // Start the trigger. + trigger.start(project, true); + + // Make sure that the timer task started. + assertNotNull(Whitebox.getInternalState(trigger, "gerritTriggerTimerTask")); + + // Wait until the timer task has run for the first time. + Thread.sleep(GerritTriggerTimer.DELAY_MILLISECONDS + 1000); + + CountDownLatch latch = Whitebox.getInternalState(trigger, "projectListIsReady"); + assertNotNull(latch); + assertEquals(0, latch.getCount()); + } + + /** + * Tests that dynamic project configurations failing with exception does not block the event listener. + * @throws Exception on failure + */ + @PrepareForTest({ + GerritTrigger.class, + AbstractProject.class, + ToGerritRunListener.class, + PluginImpl.class, + Hudson.class, + Jenkins.class, + DependencyQueueTaskDispatcher.class, + DynamicConfigurationCacheProxy.class, + EventListener.class }) + @Test + public void testDynamicTriggerUpdateFailureDoesNotBlock() throws Exception { + AbstractProject project = PowerMockito.mock(AbstractProject.class); + when(project.getFullName()).thenReturn("MockedProject"); + when(project.isBuildable()).thenReturn(true); + + DynamicConfigurationCacheProxy configurationCacheProxy = PowerMockito.mock(DynamicConfigurationCacheProxy.class); + PowerMockito.mockStatic(DynamicConfigurationCacheProxy.class); + when(DynamicConfigurationCacheProxy.getInstance()).thenReturn(configurationCacheProxy); + when(configurationCacheProxy.fetchThroughCache("url")).thenThrow(new RuntimeException()); + + Queue queue = mockConfig(project); + + PowerMockito.mockStatic(ToGerritRunListener.class); + ToGerritRunListener listener = PowerMockito.mock(ToGerritRunListener.class); + PowerMockito.when(ToGerritRunListener.getInstance()).thenReturn(listener); + + GerritTrigger trigger = new GerritTrigger(null); + trigger.setDynamicTriggerConfiguration(true); + trigger.setTriggerConfigURL("url"); + + // Set up the job within the trigger. + Whitebox.setInternalState(trigger, "job", project); + + // We need to make sure that whenever anyone asks for the trigger for any job + // that it returns this one. + PowerMockito.mockStatic(GerritTrigger.class); + when(GerritTrigger.getTrigger(any(Job.class))).thenReturn(trigger); + assertEquals(trigger, GerritTrigger.getTrigger(project)); + + // Because the "stub" methodology doesn't work, we also need to manually replace the "createListener" + // static method. + EventListener myListener = new EventListener(project); + PowerMockito.when(GerritTrigger.createListener(any(Job.class))).thenReturn(myListener); + assertNotNull(trigger.createListener()); + + // Start the trigger. + trigger.start(project, true); + + // Make sure that the timer task started. + assertNotNull(Whitebox.getInternalState(trigger, "gerritTriggerTimerTask")); + + // Wait until the timer task has run for the first time. + Thread.sleep(GerritTriggerTimer.DELAY_MILLISECONDS + 1000); + + CountDownLatch latch = Whitebox.getInternalState(trigger, "projectListIsReady"); + assertNotNull(latch); + assertEquals(0, latch.getCount()); + } + + /** + * Tests that dynamic project configurations is read when configured. + * @throws Exception on failure + */ + @Test + public void testDynamicTriggerIsRead() throws Exception { + AbstractProject project = mockProject(); + when(project.getFullName()).thenReturn("MockedProject"); + when(project.isBuildable()).thenReturn(true); + Queue queue = mockConfig(project); + + java.nio.file.Path temporaryConfigFile = java.nio.file.Files.createTempFile("GerritTriggerTest", null); + java.nio.file.Files.write(temporaryConfigFile, "p=my-project\nb^**".getBytes()); + + GerritTrigger trigger = Setup.createDefaultTrigger(project); + trigger.setDynamicTriggerConfiguration(true); + trigger.setTriggerConfigURL("file://" + temporaryConfigFile.toString()); + Setup.setTrigger(trigger, project); + + trigger.start(project, true); + + // There should be no dynamic projects yet. They won't show up until the timer + // task runs once, and there's a constant delay before that happens. + List dynamicGerritProjects = trigger.getDynamicGerritProjects(); + assertNull(dynamicGerritProjects); + + // Wait until the timer task has run for the first time. + Thread.sleep(GerritTriggerTimer.DELAY_MILLISECONDS + 1000); + + // Now check the dynamic projects again. This time, there should be one. + dynamicGerritProjects = trigger.getDynamicGerritProjects(); + assertNotNull(dynamicGerritProjects); + assertEquals(1, dynamicGerritProjects.size()); + + // Get rid of the temporary file. + java.nio.file.Files.delete(temporaryConfigFile); + } + /** * Setup a ReplicationConfig mock * @return the ReplicationConfig mock