Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Improve diagnostics for JarCache write errors (JENKINS-36947) (#91)
Improve diagnostics for JarCache write errors (related to JENKINS-36947)
  • Loading branch information
olivergondza authored and oleg-nenashev committed Aug 5, 2016
1 parent 7fbd009 commit 62aad35
Show file tree
Hide file tree
Showing 6 changed files with 100 additions and 29 deletions.
4 changes: 2 additions & 2 deletions pom.xml
Expand Up @@ -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>
Expand All @@ -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>
Expand Down
18 changes: 14 additions & 4 deletions 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;
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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 {
Expand Down Expand Up @@ -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.
*/
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/hudson/remoting/InitializeJarCacheMain.java
Expand Up @@ -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);
}
}
Expand Down
26 changes: 26 additions & 0 deletions 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;
Expand All @@ -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;

/**
Expand Down Expand Up @@ -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");
}
}
}
}
38 changes: 17 additions & 21 deletions src/test/java/hudson/remoting/FileSystemJarCacheTest.java
Expand Up @@ -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;
Expand All @@ -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";
Expand All @@ -52,6 +48,7 @@ public class FileSystemJarCacheTest {

@Before
public void setUp() throws Exception {
MockitoAnnotations.initMocks(this);
fileSystemJarCache = new FileSystemJarCache(tmp.getRoot(), true);

expectedChecksum = ChecksumTest.createdExpectedChecksum(
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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);
}
}
41 changes: 40 additions & 1 deletion src/test/java/hudson/remoting/UtilTest.java
Expand Up @@ -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);
Expand Down Expand Up @@ -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
}
}
}

0 comments on commit 62aad35

Please sign in to comment.