diff --git a/core/src/main/java/hudson/Proc.java b/core/src/main/java/hudson/Proc.java index af8a1fd9bc..9b621c36eb 100644 --- a/core/src/main/java/hudson/Proc.java +++ b/core/src/main/java/hudson/Proc.java @@ -33,6 +33,7 @@ import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; import java.util.Map; +import java.util.concurrent.CancellationException; import java.util.concurrent.ExecutionException; import java.util.concurrent.Future; import java.util.logging.Level; @@ -281,7 +282,6 @@ public abstract class Proc { @Override public void kill() throws IOException, InterruptedException { process.cancel(true); - join(); } @Override @@ -296,6 +296,8 @@ public abstract class Proc { if(e.getCause() instanceof IOException) throw (IOException)e.getCause(); throw new IOException2("Failed to join the process",e); + } catch (CancellationException x) { + return -1; } } diff --git a/core/src/test/java/hudson/LauncherTest.java b/core/src/test/java/hudson/LauncherTest.java new file mode 100644 index 0000000000..c121b052e9 --- /dev/null +++ b/core/src/test/java/hudson/LauncherTest.java @@ -0,0 +1,91 @@ +/* + * The MIT License + * + * Copyright 2009 jglick. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +package hudson; + +import hudson.remoting.Channel; +import hudson.util.StreamTaskListener; +import java.io.File; +import java.io.PipedInputStream; +import java.io.PipedOutputStream; +import java.util.concurrent.Callable; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import junit.framework.TestCase; + +public class LauncherTest extends TestCase { + + public LauncherTest(String n) { + super(n); + } + + //@Bug(4611) + public void testRemoteKill() throws Exception { + if (File.pathSeparatorChar != ':') { + System.err.println("Skipping, currently Unix-specific test"); + return; + } + + // XXX setup stolen from FilePathTest; should it be refactored into common test utility? + final ExecutorService executors = Executors.newCachedThreadPool(); + final PipedInputStream p1i = new PipedInputStream(); + final PipedInputStream p2i = new PipedInputStream(); + final PipedOutputStream p1o = new PipedOutputStream(p1i); + final PipedOutputStream p2o = new PipedOutputStream(p2i); + Future f1 = executors.submit(new Callable() { + public Channel call() throws Exception { + return new Channel("This side of the channel", executors, p1i, p2o); + } + }); + Future f2 = executors.submit(new Callable() { + public Channel call() throws Exception { + return new Channel("The other side of the channel", executors, p2i, p1o); + } + }); + Channel french = f1.get(); + Channel british = f2.get(); + + File tmp = File.createTempFile("testRemoteKill", ""); + FilePath f = new FilePath(french, tmp.getPath()); + Launcher l = f.createLauncher(new StreamTaskListener(System.err)); + Proc p = l.launch().cmds("sh", "-c", "sleep 2; rm " + tmp.getPath()).stdout(System.out).stderr(System.err).start(); + long start = System.currentTimeMillis(); + p.kill(); + assertNotSame(0, p.join()); + long end = System.currentTimeMillis(); + assertTrue("join finished promptly", (end - start < 1000)); + Thread.sleep(2500); + assertTrue("was in fact killed before completion", tmp.isFile()); + + // Manual version of test: set up instance w/ one slave. Now in script console + // new hudson.FilePath(new java.io.File("/tmp")).createLauncher(new hudson.util.StreamTaskListener(System.err)). + // launch().cmds("sleep", "1d").stdout(System.out).stderr(System.err).start().kill() + // returns immediately and pgrep sleep => nothing. But without fix + // hudson.model.Hudson.instance.nodes[0].rootPath.createLauncher(new hudson.util.StreamTaskListener(System.err)). + // launch().cmds("sleep", "1d").stdout(System.out).stderr(System.err).start().kill() + // hangs and on slave machine pgrep sleep => one process; after manual kill, script returns. + } + +} diff --git a/remoting/src/main/java/hudson/remoting/Request.java b/remoting/src/main/java/hudson/remoting/Request.java index b52e24c26b..17e7f866da 100644 --- a/remoting/src/main/java/hudson/remoting/Request.java +++ b/remoting/src/main/java/hudson/remoting/Request.java @@ -25,6 +25,7 @@ package hudson.remoting; import java.io.IOException; import java.io.Serializable; +import java.util.concurrent.CancellationException; import java.util.concurrent.ExecutionException; import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; @@ -169,26 +170,41 @@ abstract class Request extends C channel.send(this); return new hudson.remoting.Future() { - /** - * The task cannot be cancelled. - */ + + private boolean cancelled; + public boolean cancel(boolean mayInterruptIfRunning) { - return false; + if (cancelled || isDone()) { + return false; + } + cancelled = true; + if (mayInterruptIfRunning) { + try { + channel.send(new Cancel(id)); + } catch (IOException x) { + return false; + } + } + return true; } public boolean isCancelled() { - return false; + return cancelled; } public boolean isDone() { - return response!=null; + return isCancelled() || response!=null; } public RSP get() throws InterruptedException, ExecutionException { synchronized(Request.this) { try { - while(response==null) + while(response==null) { + if (isCancelled()) { + throw new CancellationException(); + } Request.this.wait(); // wait until the response arrives + } } catch (InterruptedException e) { try { channel.send(new Cancel(id)); @@ -207,8 +223,12 @@ abstract class Request extends C public RSP get(long timeout, TimeUnit unit) throws InterruptedException, ExecutionException, TimeoutException { synchronized(Request.this) { - if(response==null) + if(response==null) { + if (isCancelled()) { + throw new CancellationException(); + } Request.this.wait(unit.toMillis(timeout)); // wait until the response arrives + } if(response==null) throw new TimeoutException(); diff --git a/remoting/src/test/java/hudson/remoting/SimpleTest.java b/remoting/src/test/java/hudson/remoting/SimpleTest.java index 01d76c4878..abcd4afa6e 100644 --- a/remoting/src/test/java/hudson/remoting/SimpleTest.java +++ b/remoting/src/test/java/hudson/remoting/SimpleTest.java @@ -23,6 +23,7 @@ */ package hudson.remoting; +import java.util.concurrent.CancellationException; import junit.framework.Test; import java.util.concurrent.ExecutionException; @@ -103,6 +104,35 @@ public class SimpleTest extends RmiTestBase { } } + /** + * Checks whether {@link Future#cancel} behaves according to spec. + * Currently seems to be used by MavenBuilder.call and Proc.RemoteProc.kill + * (in turn used by MercurialSCM.joinWithTimeout when polling on remote host). + */ + //@Bug(4611) + public void testCancellation() throws Exception { + Cancellable task = new Cancellable(); + Future r = channel.callAsync(task); + r.cancel(true); + try { + r.get(); + fail("should not return normally"); + } catch (CancellationException x) { + // right + } + assertTrue(r.isCancelled()); + assertFalse(task.ran); + // XXX ought to also test various other aspects: cancelling before start, etc. + } + private static class Cancellable implements Callable { + boolean ran; + public Integer call() throws InterruptedException { + Thread.sleep(9999); + ran = true; + return 0; + } + } + public static Test suite() throws Exception { return buildSuite(SimpleTest.class); }