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);
+ }
+ }
}
}