From f61f2cd3caffcb97bc1d3d681a4110690df97b5d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20Gond=C5=BEa?= Date: Wed, 14 Jan 2015 15:12:02 +0100 Subject: [PATCH] [JENKINS-26411] Do not interrupt build abruptly when exception is thrown from build step --- .../main/java/hudson/model/AbstractBuild.java | 27 ++++--- .../hudson/model/AbstractBuildTest.groovy | 22 ++++- .../test/java/hudson/model/ExecutorTest.java | 80 ++++++++++++++----- 3 files changed, 101 insertions(+), 28 deletions(-) diff --git a/core/src/main/java/hudson/model/AbstractBuild.java b/core/src/main/java/hudson/model/AbstractBuild.java index 28ef3ff5e8..ea378bc6c6 100644 --- a/core/src/main/java/hudson/model/AbstractBuild.java +++ b/core/src/main/java/hudson/model/AbstractBuild.java @@ -532,12 +532,18 @@ public abstract class AbstractBuild

,R extends Abs Result result = doRun(listener); - Computer c = node.toComputer(); - if (c==null || c.isOffline()) { + if (node.getChannel() != null) { + // kill run-away processes that are left + // use multiple environment variables so that people can escape this massacre by overriding an environment + // variable for some processes + launcher.kill(getCharacteristicEnvVars()); + } else { // As can be seen in HUDSON-5073, when a build fails because of the slave connectivity problem, // error message doesn't point users to the slave. So let's do it here. listener.hyperlink("/computer/"+builtOn+"/log","Looks like the node went offline during the build. Check the slave log for the details."); + // TODO: Offline cause seems more reliable here. Plus, this misses permission check log.jelly does. + Computer c = node.toComputer(); if (c != null) { // grab the end of the log file. This might not work very well if the slave already // starts reconnecting. Fixing this requires a ring buffer in slave logs. @@ -550,11 +556,6 @@ public abstract class AbstractBuild

,R extends Abs } } - // kill run-away processes that are left - // use multiple environment variables so that people can escape this massacre by overriding an environment - // variable for some processes - launcher.kill(getCharacteristicEnvVars()); - // this is ugly, but for historical reason, if non-null value is returned // it should become the final result. if (result==null) result = getResult(); @@ -767,7 +768,15 @@ public abstract class AbstractBuild

,R extends Abs for (BuildStepListener bsl : BuildStepListener.all()) { bsl.started(AbstractBuild.this, bs, listener); } - boolean canContinue = mon.perform(bs, AbstractBuild.this, launcher, listener); + + boolean canContinue = false; + try { + canContinue = mon.perform(bs, AbstractBuild.this, launcher, listener); + } catch (RuntimeException ex) { + + ex.printStackTrace(listener.error("Build step failed with exception")); + } + for (BuildStepListener bsl : BuildStepListener.all()) { bsl.finished(AbstractBuild.this, bs, listener, canContinue); } @@ -1307,7 +1316,7 @@ public abstract class AbstractBuild

,R extends Abs public List getBuilds() { List r = new ArrayList(); - AbstractBuild b = (AbstractBuild)project.getNearestBuild(fromId); + AbstractBuild b = project.getNearestBuild(fromId); if (b!=null && b.getNumber()==fromId) b = b.getNextBuild(); // fromId exclusive diff --git a/test/src/test/groovy/hudson/model/AbstractBuildTest.groovy b/test/src/test/groovy/hudson/model/AbstractBuildTest.groovy index f237f9b12e..8ce911b9da 100644 --- a/test/src/test/groovy/hudson/model/AbstractBuildTest.groovy +++ b/test/src/test/groovy/hudson/model/AbstractBuildTest.groovy @@ -23,19 +23,23 @@ */ package hudson.model +import java.io.IOException; + import com.gargoylesoftware.htmlunit.Page +import hudson.Launcher; import hudson.slaves.EnvironmentVariablesNodeProperty import hudson.slaves.EnvironmentVariablesNodeProperty.Entry import org.jvnet.hudson.test.CaptureEnvironmentBuilder import org.jvnet.hudson.test.GroovyJenkinsRule - import org.jvnet.hudson.test.FakeChangeLogSCM import org.jvnet.hudson.test.FailureBuilder import org.jvnet.hudson.test.UnstableBuilder import static org.junit.Assert.* +import static org.hamcrest.CoreMatchers.*; + import org.junit.Rule import org.junit.Test import org.jvnet.hudson.test.Issue @@ -136,4 +140,20 @@ public class AbstractBuildTest { assertEquals(null, b1.getNextBuild()); } + @Test void doNotInteruptBuildAbruptlyWhenExceptionThrownFromBuildStep() { + FreeStyleProject p = j.createFreeStyleProject(); + p.getBuildersList().add(new ThrowBuilder()); + def build = p.scheduleBuild2(0).get(); + j.assertBuildStatus(Result.FAILURE, build); + + def log = build.log; + assertThat(log, containsString("Finished: FAILURE")); + assertThat(log, containsString("Build step 'Bogus' marked build as failure")); + } + + private static class ThrowBuilder extends org.jvnet.hudson.test.TestBuilder { + @Override public boolean perform(AbstractBuild build, Launcher launcher, BuildListener listener) throws InterruptedException, IOException { + throw new NullPointerException(); + } + } } diff --git a/test/src/test/java/hudson/model/ExecutorTest.java b/test/src/test/java/hudson/model/ExecutorTest.java index 28c6c7c7c6..4c90be633e 100644 --- a/test/src/test/java/hudson/model/ExecutorTest.java +++ b/test/src/test/java/hudson/model/ExecutorTest.java @@ -1,12 +1,17 @@ package hudson.model; +import static org.hamcrest.Matchers.containsString; import static org.junit.Assert.*; import com.gargoylesoftware.htmlunit.html.HtmlPage; + import hudson.Launcher; +import hudson.slaves.DumbSlave; +import hudson.slaves.OfflineCause; import hudson.util.OneShotEvent; import jenkins.model.CauseOfInterruption.UserInterruption; import jenkins.model.InterruptedBuildAction; + import org.junit.Rule; import org.junit.Test; import org.jvnet.hudson.test.Issue; @@ -14,6 +19,7 @@ import org.jvnet.hudson.test.JenkinsRule; import org.jvnet.hudson.test.TestBuilder; import java.io.IOException; +import java.util.concurrent.ExecutionException; import java.util.concurrent.Future; /** @@ -95,23 +101,10 @@ public class ExecutorTest { */ @Test public void abortCause() throws Exception { - final OneShotEvent e = new OneShotEvent(); - FreeStyleProject p = j.createFreeStyleProject(); - p.getBuildersList().add(new TestBuilder() { - @Override - public boolean perform(AbstractBuild build, Launcher launcher, BuildListener listener) throws InterruptedException, IOException { - e.signal(); // we are safe to be interrupted - synchronized (this) { - wait(); - } - throw new AssertionError(); - } - }); - Future r = p.scheduleBuild2(0); - e.block(); // wait until we are safe to interrupt - assertTrue(p.getLastBuild().isBuilding()); + Future r = startBlockingBuild(p); + User johnny = User.get("Johnny"); p.getLastBuild().getExecutor().interrupt(Result.FAILURE, new UserInterruption(johnny), // test the merge semantics @@ -120,12 +113,63 @@ public class ExecutorTest { FreeStyleBuild b = r.get(); // make sure this information is recorded - assertEquals(b.getResult(),Result.FAILURE); + assertEquals(b.getResult(), Result.FAILURE); InterruptedBuildAction iba = b.getAction(InterruptedBuildAction.class); assertEquals(1,iba.getCauses().size()); - assertEquals(((UserInterruption) iba.getCauses().get(0)).getUser(),johnny); + assertEquals(((UserInterruption) iba.getCauses().get(0)).getUser(), johnny); // make sure it shows up in the log - assertTrue(b.getLog().contains("Johnny")); + assertTrue(b.getLog().contains(johnny.getId())); + } + + @Test + public void disconnectCause() throws Exception { + DumbSlave slave = j.createOnlineSlave(); + FreeStyleProject p = j.createFreeStyleProject(); + p.setAssignedNode(slave); + + Future r = startBlockingBuild(p); + User johnny = User.get("Johnny"); + + p.getLastBuild().getBuiltOn().toComputer().disconnect( + new OfflineCause.UserCause(johnny, "Taking offline to break your build") + ); + + FreeStyleBuild b = r.get(); + + String log = b.getLog(); + assertEquals(b.getResult(), Result.FAILURE); + assertThat(log, containsString("Finished: FAILURE")); + assertThat(log, containsString("Build step 'Bogus' marked build as failure")); + } + + private Future startBlockingBuild(FreeStyleProject p) throws Exception { + final OneShotEvent e = new OneShotEvent(); + + p.getBuildersList().add(new BlockingBuilder(e)); + + Future r = p.scheduleBuild2(0); + e.block(); // wait until we are safe to interrupt + assertTrue(p.getLastBuild().isBuilding()); + + return r; + } + + private static final class BlockingBuilder extends TestBuilder { + private final OneShotEvent e; + + private BlockingBuilder(OneShotEvent e) { + this.e = e; + } + + @Override + public boolean perform(AbstractBuild build, Launcher launcher, BuildListener listener) throws InterruptedException, IOException { + e.signal(); // we are safe to be interrupted + for (;;) { + // Keep using the channel + build.getBuiltOn().getClockDifference(); + Thread.sleep(100); + } + } } }