[JENKINS-26411] Do not interrupt build abruptly when exception is thr…

…own from build step
jenkinsci · Jan 15, 2015 · f61f2cd · f61f2cd
1 parent 2efb002
commit f61f2cd
Show file tree

Hide file tree

Showing 3 changed files with 101 additions and 28 deletions.
diff --git a/core/src/main/java/hudson/model/AbstractBuild.java b/core/src/main/java/hudson/model/AbstractBuild.java
@@ -532,12 +532,18 @@ public Result run(@Nonnull BuildListener listener) throws Exception {
 
             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 Result run(@Nonnull BuildListener listener) throws Exception {
                 }
             }
 
-            // 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 @@ protected final boolean perform(BuildStep bs, BuildListener listener) throws Int
             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 DependencyChange(AbstractProject<?,?> project, int fromId, int toId) {
         public List<AbstractBuild> getBuilds() {
             List<AbstractBuild> r = new ArrayList<AbstractBuild>();
 
-            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
@@ -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
@@ -1,19 +1,25 @@
 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;
 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 @@ private Executor getExecutorByNumber(Computer c, int executorNumber) {
      */
     @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<FreeStyleBuild> r = p.scheduleBuild2(0);
-        e.block();  // wait until we are safe to interrupt
-        assertTrue(p.getLastBuild().isBuilding());
+        Future<FreeStyleBuild> 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 boolean perform(AbstractBuild<?, ?> build, Launcher launcher, BuildListen
         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<FreeStyleBuild> 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<FreeStyleBuild> startBlockingBuild(FreeStyleProject p) throws Exception {
+        final OneShotEvent e = new OneShotEvent();
+
+        p.getBuildersList().add(new BlockingBuilder(e));
+
+        Future<FreeStyleBuild> 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);
+            }
+        }
     }
 }