commit 9a6198305599d86e8f76d6b3f5bd6cdb8e15925c Author: Karsten Loesing <karsten.loesing@gmx.net> Date: Tue Jan 3 16:15:34 2017 +0100 Log more, throw fewer RuntimeExceptions. --- .../descriptor/index/DescriptorIndexCollector.java | 65 +++++++++++++++++----- .../org/torproject/descriptor/index/FileNode.java | 6 +- .../index/DescriptorIndexCollectorTest.java | 19 ++----- 3 files changed, 59 insertions(+), 31 deletions(-) diff --git a/src/main/java/org/torproject/descriptor/index/DescriptorIndexCollector.java b/src/main/java/org/torproject/descriptor/index/DescriptorIndexCollector.java index d32f811..9b269fe 100644 --- a/src/main/java/org/torproject/descriptor/index/DescriptorIndexCollector.java +++ b/src/main/java/org/torproject/descriptor/index/DescriptorIndexCollector.java @@ -43,14 +43,23 @@ public class DescriptorIndexCollector implements DescriptorCollector { public void collectDescriptors(String collecTorIndexUrlString, String[] remoteDirectories, long minLastModified, File localDirectory, boolean deleteExtraneousLocalFiles) { + log.info("Starting descriptor collection."); if (minLastModified < 0) { throw new IllegalArgumentException("A negative minimum " + "last-modified time is not permitted."); } - if (null == localDirectory || localDirectory.isFile()) { - throw new IllegalArgumentException("Local directory already exists " - + "and is not a directory."); + if (null == collecTorIndexUrlString || null == remoteDirectories + || null == localDirectory) { + throw new IllegalArgumentException("Null values are not permitted for " + + "CollecTor base URL, remote directories, or local directory."); } + if (localDirectory.isFile()) { + throw new IllegalArgumentException("A non-directory file exists at {} " + + "which is supposed to be the local directory for storing remotely " + + "fetched files. Move this file away or delete it. Aborting " + + "descriptor collection."); + } + log.info("Indexing local directory {}.", localDirectory.getAbsolutePath()); SortedMap<String, Long> localFiles = statLocalDirectory(localDirectory); SortedMap<String, FileNode> remoteFiles = null; IndexNode index = null; @@ -61,20 +70,29 @@ public class DescriptorIndexCollector implements DescriptorCollector { if (indexUrl.getPath().isEmpty()) { indexUrlString += "/index/index.json"; } + log.info("Fetching remote index file {}.", indexUrlString); index = IndexNode.fetchIndex(indexUrlString); remoteFiles = index.retrieveFilesIn(remoteDirectories); } catch (Exception ex) { - throw new RuntimeException("Cannot fetch index from '" - + indexUrlString + "'.", ex); + log.warn("Cannot fetch index file {} and hence cannot determine which " + + "remote files to fetch. Aborting descriptor collection.", + indexUrlString, ex); + return; + } + log.info("Fetching remote files from {}.", index.path); + if (!this.fetchRemoteFiles(index.path, remoteFiles, minLastModified, + localDirectory, localFiles)) { + return; } - this.fetchRemoteFiles(index.path, remoteFiles, minLastModified, - localDirectory, localFiles); if (deleteExtraneousLocalFiles) { - this.deleteExtraneousLocalFiles(remoteFiles, localDirectory, localFiles); + log.info("Deleting extraneous files from local directory {}.", + localDirectory); + deleteExtraneousLocalFiles(remoteFiles, localDirectory, localFiles); } + log.info("Finished descriptor collection."); } - void fetchRemoteFiles(String baseUrl, SortedMap<String, FileNode> remotes, + boolean fetchRemoteFiles(String baseUrl, SortedMap<String, FileNode> remotes, long minLastModified, File localDir, SortedMap<String, Long> locals) { for (Map.Entry<String, FileNode> entry : remotes.entrySet()) { String filepathname = entry.getKey(); @@ -88,10 +106,17 @@ public class DescriptorIndexCollector implements DescriptorCollector { continue; } if (!filepath.exists() && !filepath.mkdirs()) { - throw new RuntimeException("Cannot create dir: " + filepath); + log.warn("Cannot create local directory {} to store remote file {}. " + + "Aborting descriptor collection.", filepath, filename); + return false; } File destinationFile = new File(filepath, filename); File tempDestinationFile = new File(filepath, "." + filename); + log.debug("Fetching remote file {} with expected size of {} bytes from " + + "{}, storing locally to temporary file {}, then renaming to {}.", + filepathname, entry.getValue().size, baseUrl, + tempDestinationFile.getAbsolutePath(), + destinationFile.getAbsolutePath()); try (InputStream is = new URL(baseUrl + "/" + filepathname) .openStream()) { Files.copy(is, tempDestinationFile.toPath()); @@ -99,13 +124,18 @@ public class DescriptorIndexCollector implements DescriptorCollector { tempDestinationFile.renameTo(destinationFile); destinationFile.setLastModified(lastModifiedMillis); } else { - log.warn("File sizes don't match. Expected {}, but received {}.", - entry.getValue().size, tempDestinationFile.length()); + log.warn("Fetched remote file {} from {} has a size of {} bytes " + + "which is different from the expected {} bytes. Not storing " + + "this file.", + filename, baseUrl, tempDestinationFile.length(), + entry.getValue().size); } } catch (IOException e) { - log.error("Cannot fetch remote file: {}", filename, e); + log.warn("Cannot fetch remote file {} from {}. Skipping that file.", + filename, baseUrl, e); } } + return true; } static void deleteExtraneousLocalFiles( @@ -113,7 +143,10 @@ public class DescriptorIndexCollector implements DescriptorCollector { File localDir, SortedMap<String, Long> locals) { for (String localPath : locals.keySet()) { if (!remoteFiles.containsKey(localPath)) { - new File(localDir, localPath).delete(); + File extraneousLocalFile = new File(localDir, localPath); + log.debug("Deleting extraneous local file {}.", + extraneousLocalFile.getAbsolutePath()); + extraneousLocalFile.delete(); } } } @@ -137,7 +170,9 @@ public class DescriptorIndexCollector implements DescriptorCollector { } }); } catch (IOException ioe) { - log.error("Cannot stat local directory.", ioe); + log.warn("Cannot index local directory {} to skip any remote files that " + + "already exist locally. Continuing with an either empty or " + + "incomplete index of local files.", ioe); } return locals; } diff --git a/src/main/java/org/torproject/descriptor/index/FileNode.java b/src/main/java/org/torproject/descriptor/index/FileNode.java index 02e82da..6af5aa3 100644 --- a/src/main/java/org/torproject/descriptor/index/FileNode.java +++ b/src/main/java/org/torproject/descriptor/index/FileNode.java @@ -67,8 +67,10 @@ public class FileNode implements Comparable<FileNode> { try { lastModifiedMillis = dateTimeFormat.parse(this.lastModified).getTime(); } catch (ParseException ex) { - log.warn("Cannot parse date-time. Setting lastModifiedMillis to -1L.", - ex); + log.warn("Cannot parse last-modified time {} of remote file entry {}. " + + "Fetching remote file regardless of configured last-modified " + + "time. The following error message provides more details.", + this.lastModified, this.path, ex); this.lastModifiedMillis = -1L; } } diff --git a/src/test/java/org/torproject/descriptor/index/DescriptorIndexCollectorTest.java b/src/test/java/org/torproject/descriptor/index/DescriptorIndexCollectorTest.java index c5a6a5c..8886c85 100644 --- a/src/test/java/org/torproject/descriptor/index/DescriptorIndexCollectorTest.java +++ b/src/test/java/org/torproject/descriptor/index/DescriptorIndexCollectorTest.java @@ -210,11 +210,12 @@ public class DescriptorIndexCollectorTest { assertTrue("found " + res, res.isEmpty()); } - @Test(expected = RuntimeException.class) + @Test() public void testMinimalArgs() throws IOException { File fakeDir = tmpf.newFolder("fantasy-dir"); new DescriptorIndexCollector() - .collectDescriptors(null, new String[]{}, 100L, fakeDir, true); + .collectDescriptors("http://localhost", new String[]{}, 100L, fakeDir, + true); } @Test(expected = IllegalArgumentException.class) @@ -227,7 +228,7 @@ public class DescriptorIndexCollectorTest { public void testIllegalDirectory() throws IOException { File fakeDir = tmpf.newFile("fantasy-dir"); new DescriptorIndexCollector().collectDescriptors( - null, new String[]{}, 100L, fakeDir, false); + "", new String[]{}, 100L, fakeDir, false); } @Test(expected = IllegalArgumentException.class) @@ -236,24 +237,14 @@ public class DescriptorIndexCollectorTest { null, new String[]{}, 100L, null, false); } - @Test(expected = IllegalArgumentException.class) - public void testExistingFile() throws IOException { - File fakeDir = tmpf.newFile("fantasy-dir"); - new DescriptorIndexCollector() - .collectDescriptors(null, null, 100L, fakeDir, false); - } - @Test() public void testExistingDir() throws IOException { File dir = tmpf.newFolder(); dir.setWritable(false); SortedMap<String, FileNode> fm = new TreeMap<>(); fm.put("readonly", new FileNode("w", 2L, "2100-01-01 01:01")); - thrown.expect(RuntimeException.class); - thrown.expectMessage("Cannot create dir: " + dir.toString() - + "/readonly"); new DescriptorIndexCollector() - .fetchRemoteFiles(null, fm, 100L, dir, new TreeMap<String, Long>()); + .fetchRemoteFiles("", fm, 100L, dir, new TreeMap<String, Long>()); } }