From 957ffd30096a4309fc7da779d03086edb46e4fe3 Mon Sep 17 00:00:00 2001 From: Kohsuke Kawaguchi Date: Mon, 14 Mar 2011 13:26:11 -0700 Subject: [PATCH] [FIXED JENKINS-7871] Fixed a race condition in FilePath.copyTo that results in missing tail bits when the master is heavily loaded. --- core/src/main/java/hudson/FilePath.java | 4 + core/src/test/java/hudson/FilePathTest.java | 95 +++++++++++++++++++ .../main/java/hudson/remoting/Channel.java | 23 +++-- .../java/hudson/remoting/LocalChannel.java | 4 + .../java/hudson/remoting/VirtualChannel.java | 8 ++ 5 files changed, 125 insertions(+), 9 deletions(-) diff --git a/core/src/main/java/hudson/FilePath.java b/core/src/main/java/hudson/FilePath.java index 84184eaeff..8b84f09000 100644 --- a/core/src/main/java/hudson/FilePath.java +++ b/core/src/main/java/hudson/FilePath.java @@ -1405,6 +1405,10 @@ public final class FilePath implements Serializable { } } }); + + // make sure the write fully happens before we return. + if (channel!=null) + channel.syncLocalIO(); } /** diff --git a/core/src/test/java/hudson/FilePathTest.java b/core/src/test/java/hudson/FilePathTest.java index 4dfdd8eff5..df82a2403b 100644 --- a/core/src/test/java/hudson/FilePathTest.java +++ b/core/src/test/java/hudson/FilePathTest.java @@ -23,12 +23,24 @@ */ package hudson; +import hudson.remoting.Future; import hudson.remoting.VirtualChannel; +import hudson.util.IOException2; import hudson.util.NullStream; import java.io.File; +import java.io.FileOutputStream; +import java.io.IOException; +import java.io.OutputStream; +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.Callable; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import org.apache.commons.io.output.CountingOutputStream; import org.apache.commons.io.output.NullOutputStream; +import org.jvnet.hudson.test.Bug; /** * @author Kohsuke Kawaguchi @@ -61,6 +73,89 @@ public class FilePathTest extends ChannelTestCase { } } + /** + * As we moved the I/O handling to another thread, there's a race condition in + * {@link FilePath#copyTo(OutputStream)} — this method can return before + * all the writes are delivered to {@link OutputStream}. + * + *

+ * To reproduce that problem, we use a large number of threads, so that we can + * maximize the chance of out-of-order execution, and make sure we are + * seeing the right byte count at the end. + * + * Also see JENKINS-7897 + */ + @Bug(7871) + public void testCopyTo3() throws Exception { + final File tmp = File.createTempFile("testCopyTo3",""); + + FileOutputStream os = new FileOutputStream(tmp); + final int size = 90000; + byte[] buf = new byte[size]; + for (int i=0; i r = new ArrayList(); + for (int i=0; i<100; i++) { + r.add(es.submit(new Callable() { + public Object call() throws Exception { + class Sink extends OutputStream { + private Exception closed; + private volatile int count; + + private void checkNotClosed() throws IOException2 { + if (closed != null) + throw new IOException2(closed); + } + + @Override + public void write(int b) throws IOException { + count++; + checkNotClosed(); + } + + @Override + public void write(byte[] b) throws IOException { + count+=b.length; + checkNotClosed(); + } + + @Override + public void write(byte[] b, int off, int len) throws IOException { + count+=len; + checkNotClosed(); + } + + @Override + public void close() throws IOException { + closed = new Exception(); + if (size!=count) + fail(); + } + } + + FilePath f = new FilePath(french, tmp.getPath()); + Sink sink = new Sink(); + f.copyTo(sink); + assertEquals(size,sink.count); + return null; + } + })); + } + + for (java.util.concurrent.Future f : r) + f.get(); + } finally { + es.shutdown(); + } + } + + + public void testRepeatCopyRecursiveTo() throws Exception { // local->local copy used to return 0 if all files were "up to date" // should return number of files processed, whether or not they were copied or already current diff --git a/remoting/src/main/java/hudson/remoting/Channel.java b/remoting/src/main/java/hudson/remoting/Channel.java index 54ed0186bc..94049f82b1 100644 --- a/remoting/src/main/java/hudson/remoting/Channel.java +++ b/remoting/src/main/java/hudson/remoting/Channel.java @@ -932,17 +932,22 @@ public class Channel implements VirtualChannel, IChannel { call(new IOSyncer()); } + public void syncLocalIO() throws InterruptedException { + try { + pipeWriter.submit(new Runnable() { + public void run() { + // noop + } + }).get(); + } catch (ExecutionException e) { + throw new AssertionError(e); // impossible + } + } + private static final class IOSyncer implements Callable { public Object call() throws InterruptedException { - try { - return Channel.current().pipeWriter.submit(new Runnable() { - public void run() { - // noop - } - }).get(); - } catch (ExecutionException e) { - throw new AssertionError(e); // impossible - } + Channel.current().syncLocalIO(); + return null; } private static final long serialVersionUID = 1L; diff --git a/remoting/src/main/java/hudson/remoting/LocalChannel.java b/remoting/src/main/java/hudson/remoting/LocalChannel.java index 251626c1b9..8ea1b4a2ad 100644 --- a/remoting/src/main/java/hudson/remoting/LocalChannel.java +++ b/remoting/src/main/java/hudson/remoting/LocalChannel.java @@ -98,4 +98,8 @@ public class LocalChannel implements VirtualChannel { public T export(Class intf, T instance) { return instance; } + + public void syncLocalIO() throws InterruptedException { + // noop + } } diff --git a/remoting/src/main/java/hudson/remoting/VirtualChannel.java b/remoting/src/main/java/hudson/remoting/VirtualChannel.java index 9d035de86e..6cf9622ec6 100644 --- a/remoting/src/main/java/hudson/remoting/VirtualChannel.java +++ b/remoting/src/main/java/hudson/remoting/VirtualChannel.java @@ -103,4 +103,12 @@ public interface VirtualChannel { * will invoke the same method on the given local instance object. */ T export( Class type, T instance); + + /** + * Blocks until all the I/O packets sent from remote is fully locally executed, then return. + * + * @since 1.402 + */ + void syncLocalIO() throws InterruptedException; } +