[FIXED JENKINS-9540] Failure of recursive copy on slave side should b…

…e reported. Instead it was being thrown in a Future that the master did not wait for: the writing side failed with a closed pipe exception, and this meaningless error was thrown up. Fixing by waiting for the reading side first, so any error there will be thrown up. (If there was _also_ an error on the writing side, we would miss it, but typically this will just be the closed stream.)
jenkinsci · Aug 29, 2013 · 574fe14 · 574fe14
1 parent d75c350
commit 574fe14
Show file tree

Hide file tree

Showing 3 changed files with 51 additions and 2 deletions.
diff --git a/changelog.html b/changelog.html
@@ -58,6 +58,9 @@
   <li class=bug>
     Deleting an external run did not immediately remove it from build list, leading to errors from log rotation.
     (<a href="https://issues.jenkins-ci.org/browse/JENKINS-19377">issue 19377</a>)
+  <li class=bug>
+    When copying a directory from master to slave fails due to an error on the slave, properly report it.
+    (<a href="https://issues.jenkins-ci.org/browse/JENKINS-9540">issue 9540</a>)
   <li class=rfe>
     Annotate the Advanced section if some fields are already customized.
     (<a href="https://issues.jenkins-ci.org/browse/JENKINS-3107">issue 3107</a>)

diff --git a/core/src/main/java/hudson/FilePath.java b/core/src/main/java/hudson/FilePath.java
@@ -1931,13 +1931,19 @@ public Void invoke(File f, VirtualChannel channel) throws IOException {
                     }
                 }
             });
-            int r = writeToTar(new File(remote),fileMask,excludes,TarCompression.GZIP.compress(pipe.getOut()));
+            Future<Integer> future2 = actAsync(new FileCallable<Integer>() {
+                private static final long serialVersionUID = 1L;
+                @Override public Integer invoke(File f, VirtualChannel channel) throws IOException, InterruptedException {
+                    return writeToTar(new File(remote),fileMask,excludes,TarCompression.GZIP.compress(pipe.getOut()));
+                }
+            });
             try {
+                // JENKINS-9540 in case the reading side failed, report that error first
                 future.get();
+                return future2.get();
             } catch (ExecutionException e) {
                 throw new IOException2(e);
             }
-            return r;
         } else {
             // remote -> local copy
             final Pipe pipe = Pipe.createRemoteToLocal();

diff --git a/core/src/test/java/hudson/FilePathTest.java b/core/src/test/java/hudson/FilePathTest.java
@@ -206,6 +206,46 @@ public void testRepeatCopyRecursiveTo() throws Exception {
         }
     }
 
+    @Bug(9540)
+    public void testErrorMessageInRemoteCopyRecursive() throws Exception {
+        File tmp = Util.createTempDir();
+        try {
+            File src = new File(tmp, "src");
+            File dst = new File(tmp, "dst");
+            FilePath from = new FilePath(src);
+            FilePath to = new FilePath(british, dst.getAbsolutePath());
+            for (int i = 0; i < 10000; i++) {
+                // TODO is there a simpler way to force the TarOutputStream to be flushed and the reader to start?
+                // Have not found a way to make the failure guaranteed.
+                OutputStream os = from.child("content" + i).write();
+                try {
+                    for (int j = 0; j < 1024; j++) {
+                        os.write('.');
+                    }
+                } finally {
+                    os.close();
+                }
+            }
+            FilePath toF = to.child("content0");
+            toF.write().close();
+            toF.chmod(0400);
+            try {
+                from.copyRecursiveTo(to);
+                // on Windows this may just succeed; OK, test did not prove anything then
+            } catch (IOException x) {
+                if (Functions.printThrowable(x).contains("content0")) {
+                    // Fine, error message talks about permission denied.
+                } else {
+                    throw x;
+                }
+            } finally {
+                toF.chmod(700);
+            }
+        } finally {
+            Util.deleteRecursive(tmp);
+        }
+    }
+
     public void testArchiveBug4039() throws Exception {
         File tmp = Util.createTempDir();
         try {