diff --git a/core/src/main/java/hudson/util/ProcessTree.java b/core/src/main/java/hudson/util/ProcessTree.java index f1503dd7ff..d18d92e7a9 100644 --- a/core/src/main/java/hudson/util/ProcessTree.java +++ b/core/src/main/java/hudson/util/ProcessTree.java @@ -94,9 +94,20 @@ public abstract class ProcessTree implements Iterable, IProcessTree, * Lazily obtained {@link ProcessKiller}s to be applied on this process tree. */ private transient volatile List killers; + + /** + * Flag to skip the veto check since there aren't any. + */ + private boolean skipVetoes; // instantiation only allowed for subtypes in this class - private ProcessTree() {} + private ProcessTree() { + skipVetoes = false; + } + + private ProcessTree(boolean vetoesExist) { + skipVetoes = !vetoesExist; + } /** * Gets the process given a specific ID, or null if no such process exists. @@ -241,14 +252,26 @@ public abstract class ProcessTree implements Iterable, IProcessTree, * null if no one objects killing the process. */ protected @CheckForNull VetoCause getVeto() { - for (ProcessKillingVeto vetoExtension : ProcessKillingVeto.all()) { - VetoCause cause = vetoExtension.vetoProcessKilling(this); - if (cause != null) { - if (LOGGER.isLoggable(FINEST)) - LOGGER.finest("Killing of pid " + getPid() + " vetoed by " + vetoExtension.getClass().getName() + ": " + cause.getMessage()); - return cause; + String causeMessage = null; + + // Quick check, does anything exist to check against + if (!skipVetoes) { + try { + VirtualChannel channelToMaster = SlaveComputer.getChannelToMaster(); + if (channelToMaster!=null) { + CheckVetoes vetoCheck = new CheckVetoes(this); + causeMessage = channelToMaster.call(vetoCheck); + } + } catch (IOException e) { + LOGGER.log(Level.WARNING, "I/O Exception while checking for vetoes", e); + } catch (InterruptedException e) { + LOGGER.log(Level.WARNING, "Interrupted Exception while checking for vetoes", e); } } + + if (causeMessage != null) { + return new VetoCause(causeMessage); + } return null; } @@ -300,6 +323,27 @@ public abstract class ProcessTree implements Iterable, IProcessTree, Object writeReplace() { return new SerializedProcess(pid); } + + private class CheckVetoes extends SlaveToMasterCallable { + private IOSProcess process; + + public CheckVetoes(IOSProcess processToCheck) { + process = processToCheck; + } + + @Override + public String call() throws IOException { + for (ProcessKillingVeto vetoExtension : ProcessKillingVeto.all()) { + VetoCause cause = vetoExtension.vetoProcessKilling(process); + if (cause != null) { + if (LOGGER.isLoggable(FINEST)) + LOGGER.info("Killing of pid " + getPid() + " vetoed by " + vetoExtension.getClass().getName() + ": " + cause.getMessage()); + return cause.getMessage(); + } + } + return null; + } + } } /** @@ -338,6 +382,8 @@ public abstract class ProcessTree implements Iterable, IProcessTree, } + private static Boolean vetoersExist; + /** * Gets the {@link ProcessTree} of the current system * that JVM runs in, or in the worst case return the default one @@ -347,17 +393,30 @@ public abstract class ProcessTree implements Iterable, IProcessTree, if(!enabled) return DEFAULT; + // Check for the existance of vetoers if I don't know already + if (vetoersExist == null) { + try { + vetoersExist = SlaveComputer.getChannelToMaster().call(new DoVetoersExist()); + } + catch (Exception e) { + LOGGER.log(Level.WARNING, "Error while determining if vetoers exist", e); + } + } + + // Null-check in case the previous call worked + boolean vetoes = (vetoersExist == null ? true : vetoersExist); + try { if(File.pathSeparatorChar==';') - return new Windows(); + return new Windows(vetoes); String os = Util.fixNull(System.getProperty("os.name")); if(os.equals("Linux")) - return new Linux(); + return new Linux(vetoes); if(os.equals("SunOS")) - return new Solaris(); + return new Solaris(vetoes); if(os.equals("Mac OS X")) - return new Darwin(); + return new Darwin(vetoes); } catch (LinkageError e) { LOGGER.log(Level.WARNING,"Failed to load winp. Reverting to the default",e); enabled = false; @@ -365,6 +424,13 @@ public abstract class ProcessTree implements Iterable, IProcessTree, return DEFAULT; } + + private static class DoVetoersExist extends SlaveToMasterCallable { + @Override + public Boolean call() throws IOException { + return ProcessKillingVeto.all().size() > 0; + } + } // // @@ -512,7 +578,9 @@ public abstract class ProcessTree implements Iterable, IProcessTree, } private static final class Windows extends Local { - Windows() { + Windows(boolean vetoesExist) { + super(vetoesExist); + for (final WinProcess p : WinProcess.all()) { int pid = p.getPid(); if(pid == 0 || pid == 4) continue; // skip the System Idle and System processes @@ -574,6 +642,10 @@ public abstract class ProcessTree implements Iterable, IProcessTree, } static abstract class Unix extends Local { + public Unix(boolean vetoersExist) { + super(vetoersExist); + } + @Override public OSProcess get(Process proc) { try { @@ -595,7 +667,9 @@ public abstract class ProcessTree implements Iterable, IProcessTree, * {@link ProcessTree} based on /proc. */ static abstract class ProcfsUnix extends Unix { - ProcfsUnix() { + ProcfsUnix(boolean vetoersExist) { + super(vetoersExist); + File[] processes = new File("/proc").listFiles(new FileFilter() { public boolean accept(File f) { return f.isDirectory(); @@ -739,6 +813,10 @@ public abstract class ProcessTree implements Iterable, IProcessTree, static class Linux extends ProcfsUnix { + public Linux(boolean vetoersExist) { + super(vetoersExist); + } + protected LinuxProcess createProcess(int pid) throws IOException { return new LinuxProcess(pid); } @@ -853,6 +931,10 @@ public abstract class ProcessTree implements Iterable, IProcessTree, * when accessing this file. */ static class Solaris extends ProcfsUnix { + public Solaris(boolean vetoersExist) { + super(vetoersExist); + } + protected OSProcess createProcess(final int pid) throws IOException { return new SolarisProcess(pid); } @@ -1093,7 +1175,9 @@ public abstract class ProcessTree implements Iterable, IProcessTree, * Implementation for Mac OS X based on sysctl(3). */ private static class Darwin extends Unix { - Darwin() { + Darwin(boolean vetoersExist) { + super(vetoersExist); + String arch = System.getProperty("sun.arch.data.model"); if ("64".equals(arch)) { sizeOf_kinfo_proc = sizeOf_kinfo_proc_64; @@ -1312,8 +1396,13 @@ public abstract class ProcessTree implements Iterable, IProcessTree, * (The opposite of {@link Remote}.) */ public static abstract class Local extends ProcessTree { + @Deprecated Local() { } + + Local(boolean vetoesExist) { + super(vetoesExist); + } } /** @@ -1322,11 +1411,20 @@ public abstract class ProcessTree implements Iterable, IProcessTree, public static class Remote extends ProcessTree implements Serializable { private final IProcessTree proxy; + @Deprecated public Remote(ProcessTree proxy, Channel ch) { this.proxy = ch.export(IProcessTree.class,proxy); for (Entry e : proxy.processes.entrySet()) processes.put(e.getKey(),new RemoteProcess(e.getValue(),ch)); } + + public Remote(ProcessTree proxy, Channel ch, boolean vetoersExist) { + super(vetoersExist); + + this.proxy = ch.export(IProcessTree.class,proxy); + for (Entry e : proxy.processes.entrySet()) + processes.put(e.getKey(),new RemoteProcess(e.getValue(),ch)); + } @Override public OSProcess get(Process proc) { diff --git a/test/src/test/java/hudson/util/ProcessTreeKillerTest.java b/test/src/test/java/hudson/util/ProcessTreeKillerTest.java index 103abe6abb..e593baea1e 100644 --- a/test/src/test/java/hudson/util/ProcessTreeKillerTest.java +++ b/test/src/test/java/hudson/util/ProcessTreeKillerTest.java @@ -4,6 +4,7 @@ import static org.junit.Assert.*; import static org.junit.Assume.*; import java.io.File; import java.io.IOException; +import java.io.StringWriter; import hudson.EnvVars; import hudson.Functions; @@ -13,6 +14,7 @@ import hudson.model.AbstractBuild; import hudson.model.BuildListener; import hudson.model.FreeStyleBuild; import hudson.model.FreeStyleProject; +import hudson.model.Slave; import hudson.tasks.Maven; import hudson.tasks.Shell; import hudson.util.ProcessTreeRemoting.IOSProcess; @@ -27,6 +29,7 @@ import org.jvnet.hudson.test.TestBuilder; import org.jvnet.hudson.test.TestExtension; import com.google.common.collect.ImmutableMap; + public class ProcessTreeKillerTest { @Rule @@ -136,8 +139,44 @@ public class ProcessTreeKillerTest { // Means the process is still running } } + + @Test + @Issue("JENKINS-9104") + public void considersKillingVetosOnSlave() throws Exception { + // on some platforms where we fail to list any processes, this test will + // just not work + assumeTrue(ProcessTree.get() != ProcessTree.DEFAULT); + + // Define a process we (shouldn't) kill + ProcessBuilder pb = new ProcessBuilder(); + pb.environment().put("cookie", "testKeepDaemonsAlive"); + + if (File.pathSeparatorChar == ';') { + pb.command("cmd"); + } else { + pb.command("sleep", "5m"); + } + + // Create a slave so we can tell it to kill the process + Slave s = j.createSlave(); + s.toComputer().connect(false).get(); + + // Start the process + process = pb.start(); + + // Call killall (somewhat roundabout though) to (not) kill it + StringWriter out = new StringWriter(); + s.createLauncher(new StreamTaskListener(out)).kill(ImmutableMap.of("cookie", "testKeepDaemonsAlive")); + + try { + process.exitValue(); + fail("Process should have been excluded from the killing"); + } catch (IllegalThreadStateException e) { + // Means the process is still running + } + } - @TestExtension("considersKillingVetos") + @TestExtension({"considersKillingVetos", "considersKillingVetosOnSlave"}) public static class VetoAllKilling extends ProcessKillingVeto { @Override public VetoCause vetoProcessKilling(IOSProcess p) {