Improve diagnostics for JarCache write errors (JENKINS-36947) (#91)

Improve diagnostics for JarCache write errors (related to JENKINS-36947)

pom.xml

@@ -327,7 +327,7 @@ THE SOFTWARE.
         <!-- make sure our code doesn't have 1.6 dependencies except where we know it -->
         <groupId>org.codehaus.mojo</groupId>
         <artifactId>animal-sniffer-maven-plugin</artifactId>
-        <version>1.14</version>
+        <version>1.15</version>
         <executions>
           <execution>
             <goals>
@@ -344,7 +344,7 @@ THE SOFTWARE.
           <ignores>
             <!--
                  this reference comes from args4j. animal-sniffer doesn't seem to let me specify
-                 the classes not to scan, and instead only let me ignore references. 
+                 the classes not to scan, and instead only let me ignore references.
             -->
             <ignore>java.nio.file.Paths</ignore>
           </ignores>

src/main/java/hudson/remoting/FileSystemJarCache.java

@@ -1,5 +1,6 @@
 package hudson.remoting;
 
+import javax.annotation.Nonnull;
 import java.io.File;
 import java.io.FileOutputStream;
 import java.io.IOException;
@@ -36,7 +37,12 @@ public FileSystemJarCache(File rootDir, boolean touch) {
         this.touch = touch;
         if (rootDir==null)
             throw new IllegalArgumentException();
-        rootDir.mkdirs();
+
+        try {
+            Util.mkdirs(rootDir);
+        } catch (IOException ex) {
+            throw new RuntimeException("Root directory not writable");
+        }
     }
 
     @Override
@@ -55,7 +61,6 @@ protected URL lookInCache(Channel channel, long sum1, long sum2) throws IOExcept
     @Override
     protected URL retrieve(Channel channel, long sum1, long sum2) throws IOException, InterruptedException {
         File target = map(sum1, sum2);
-        File parent = target.getParentFile();
 
         if (target.exists()) {
             // Assume its already been fetched correctly before. ie. We are not going to validate
@@ -64,9 +69,8 @@ protected URL retrieve(Channel channel, long sum1, long sum2) throws IOException
             return target.toURI().toURL();
         }
 
-        parent.mkdirs();
         try {
-            File tmp = File.createTempFile(target.getName(), "tmp", parent);
+            File tmp = createTempJar(target);
             try {
                 RemoteOutputStream o = new RemoteOutputStream(new FileOutputStream(tmp));
                 try {
@@ -113,6 +117,12 @@ protected URL retrieve(Channel channel, long sum1, long sum2) throws IOException
         }
     }
 
+    /*package for testing*/ File createTempJar(@Nonnull File target) throws IOException {
+        File parent = target.getParentFile();
+        Util.mkdirs(parent);
+        return File.createTempFile(target.getName(), "tmp", parent);
+    }
+
     /**
      * Map to the cache jar file name.
      */

src/main/java/hudson/remoting/InitializeJarCacheMain.java

@@ -46,7 +46,7 @@ public static void main(String[] argv) throws Exception {
             Checksum checksum = Checksum.forFile(jar);
             File newJarLocation = jarCache.map(checksum.sum1, checksum.sum2);
 
-            newJarLocation.getParentFile().mkdirs();
+            Util.mkdirs(newJarLocation.getParentFile());
             copyFile(jar, newJarLocation);
         }
     }

src/main/java/hudson/remoting/Util.java

@@ -1,5 +1,7 @@
 package hudson.remoting;
 
+import org.jvnet.animal_sniffer.IgnoreJRERequirement;
+
 import java.io.ByteArrayOutputStream;
 import java.io.File;
 import java.io.FileOutputStream;
@@ -14,8 +16,10 @@
 import java.net.Proxy;
 import java.net.SocketAddress;
 import java.net.URL;
+import javax.annotation.Nonnull;
 import javax.net.ssl.HttpsURLConnection;
 import javax.net.ssl.SSLSocketFactory;
+import java.nio.file.Files;
 import java.util.Iterator;
 
 /**
@@ -247,4 +251,26 @@ static InetSocketAddress getResolvedHttpProxyAddress(String host, int port) thro
         }
         return targetAddress;
     }
+
+    @IgnoreJRERequirement @SuppressWarnings("Since15")
+    static void mkdirs(@Nonnull File file) throws IOException {
+        if (file.isDirectory()) return;
+
+        try {
+            Class.forName("java.nio.file.Files");
+            Files.createDirectories(file.toPath());
+            return;
+        } catch (ClassNotFoundException e) {
+            // JDK6
+        } catch (ExceptionInInitializerError e) {
+            // JDK7 on multibyte encoding (http://bugs.java.com/bugdatabase/view_bug.do?bug_id=7050570)
+        }
+
+        // Fallback
+        if (!file.mkdirs()) {
+            if (!file.isDirectory()) {
+                throw new IOException("Directory not created");
+            }
+        }
+    }
 }

src/test/java/hudson/remoting/FileSystemJarCacheTest.java

@@ -8,12 +8,10 @@
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
 import org.junit.rules.TemporaryFolder;
-import org.junit.runner.RunWith;
 import org.mockito.Mock;
+import org.mockito.MockitoAnnotations;
 import org.mockito.invocation.InvocationOnMock;
 import org.mockito.stubbing.Answer;
-import org.powermock.core.classloader.annotations.PrepareForTest;
-import org.powermock.modules.junit4.PowerMockRunner;
 
 import java.io.File;
 import java.io.IOException;
@@ -27,17 +25,15 @@
 import static org.mockito.Matchers.any;
 import static org.mockito.Matchers.eq;
 import static org.mockito.Mockito.doAnswer;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.when;
-import static org.powermock.api.mockito.PowerMockito.mockStatic;
-import static org.powermock.api.mockito.PowerMockito.spy;
 
 /**
  * Tests for {@link FileSystemJarCache}.
  *
  * @author Akshay Dayal
  */
-@RunWith(PowerMockRunner.class)
-@PrepareForTest({FileSystemJarCache.class, File.class})
 public class FileSystemJarCacheTest {
 
     private static final String CONTENTS = "These are the contents";
@@ -52,6 +48,7 @@
 
     @Before
     public void setUp() throws Exception {
+        MockitoAnnotations.initMocks(this);
         fileSystemJarCache = new FileSystemJarCache(tmp.getRoot(), true);
 
         expectedChecksum = ChecksumTest.createdExpectedChecksum(
@@ -111,14 +108,13 @@ public Void answer(InvocationOnMock invocationOnMock) throws Throwable {
                 mockChannel, expectedChecksum.sum1, expectedChecksum.sum2);
     }
 
-/*  for whatever reason I just can't seem to make this one test work. HELP!
     @Test
     public void testRenameFailsAndNoTarget() throws Exception {
         File expectedFile = fileSystemJarCache.map(expectedChecksum.sum1, expectedChecksum.sum2);
         File spy = spy(tmp.newFile());
-        mockStatic(File.class);
-        when(File.createTempFile(expectedFile.getName(), "tmp", expectedFile.getParentFile()))
-                .thenReturn(spy);
+        FileSystemJarCache jarCache = spy(fileSystemJarCache);
+        doReturn(spy).when(jarCache).createTempJar(any(File.class));
+
         when(mockChannel.getProperty(JarLoader.THEIRS)).thenReturn(mockJarLoader);
         doAnswer(new Answer<Void>() {
             @Override
@@ -131,23 +127,23 @@ public Void answer(InvocationOnMock invocationOnMock) throws Throwable {
                 eq(expectedChecksum.sum1),
                 eq(expectedChecksum.sum2),
                 any(RemoteOutputStream.class));
+
         when(spy.renameTo(expectedFile)).thenReturn(false);
         assertFalse(expectedFile.exists());
 
         expectedEx.expect(IOException.class);
         expectedEx.expectCause(hasMessage(StringContains.containsString("Unable to create")));
-        fileSystemJarCache.retrieve(
-                mockChannel, expectedChecksum.sum1, expectedChecksum.sum2);
+
+        jarCache.retrieve(mockChannel, expectedChecksum.sum1, expectedChecksum.sum2);
     }
-*/
 
     @Test
     public void testRenameFailsAndBadPreviousTarget() throws Exception {
         final File expectedFile = fileSystemJarCache.map(expectedChecksum.sum1, expectedChecksum.sum2);
-        File spy = spy(tmp.newFile());
-        mockStatic(File.class);
-        when(File.createTempFile(expectedFile.getName(), "tmp", expectedFile.getParentFile()))
-                .thenReturn(spy);
+        File fileSpy = spy(tmp.newFile());
+        FileSystemJarCache jarCache = spy(fileSystemJarCache);
+        doReturn(fileSpy).when(jarCache).createTempJar(any(File.class));
+
         when(mockChannel.getProperty(JarLoader.THEIRS)).thenReturn(mockJarLoader);
         doAnswer(new Answer<Void>() {
             @Override
@@ -168,12 +164,12 @@ public Boolean answer(InvocationOnMock invocationOnMock) throws Throwable {
                 Files.append("Some other contents", expectedFile, Charset.forName("UTF-8"));
                 return false;
             }
-        }).when(spy).renameTo(expectedFile);
+        }).when(fileSpy).renameTo(expectedFile);
 
         expectedEx.expect(IOException.class);
         expectedEx.expectCause(hasMessage(StringContains.containsString(
                 "Incorrect checksum of previous jar")));
-        fileSystemJarCache.retrieve(
-                mockChannel, expectedChecksum.sum1, expectedChecksum.sum2);
+
+        jarCache.retrieve(mockChannel, expectedChecksum.sum1, expectedChecksum.sum2);
     }
 }

src/test/java/hudson/remoting/UtilTest.java

@@ -26,20 +26,26 @@
 
 import junit.framework.TestCase;
 import org.junit.Before;
+import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
 import org.junit.runner.RunWith;
-import org.mockito.Mockito;
 import org.powermock.api.mockito.PowerMockito;
 import org.powermock.core.classloader.annotations.PrepareForTest;
 import org.powermock.modules.junit4.PowerMockRunner;
 
+import java.io.File;
+import java.io.IOException;
+
 /**
  * @author Etienne Bec
  */
 @RunWith(PowerMockRunner.class)
 @PrepareForTest(Util.class)
 public class UtilTest extends TestCase {
 
+    @Rule public TemporaryFolder temp = new TemporaryFolder();
+
     @Before
     public void mockSystem() {
         PowerMockito.mockStatic(System.class);
@@ -131,4 +137,37 @@ public void testMixed() {
         assertEquals(false, Util.inNoProxyEnvVar("sub.foobar.org"));
         assertEquals(false, Util.inNoProxyEnvVar("sub.jenkins.org"));
     }
+
+    @Test
+    public void mkdirs() throws IOException {
+        File sandbox = temp.newFolder();
+        // Dir exists already
+        Util.mkdirs(sandbox);
+        assertTrue(sandbox.exists());
+
+        // Create nested subdir
+        File subdir = new File(sandbox, "sub/dir");
+        Util.mkdirs(subdir);
+        assertTrue(subdir.exists());
+
+        // Do not overwrite a file
+        File file = new File(sandbox, "regular.file");
+        file.createNewFile();
+        try {
+            Util.mkdirs(file);
+        } catch (IOException ex) {
+            // Expected
+        }
+        assertTrue(file.exists());
+        assertTrue(file.isFile());
+
+        // Fail to create aloud
+        try {
+            File forbidden = new File("/proc/nonono");
+            Util.mkdirs(forbidden);
+            fail();
+        } catch (IOException ex) {
+            // Expected
+        }
+    }
 }