commit 1dab40d4505e731faccd5f722e5f7c250a2d4955 Author: iwakeh <iwakeh@torproject.org> Date: Fri Aug 19 13:23:07 2016 +0200 Added slf4j for logging support; implements #19643. Tests use logback; runtime will use whatever slf4j implementation supplied. Also removed TODOs about testing, b/c this is noticeable in our coverage report. --- build.xml | 3 +++ .../descriptor/impl/DescriptorCollectorImpl.java | 12 +++++++++--- .../descriptor/impl/DescriptorReaderImpl.java | 22 ++++++++++------------ .../impl/DirectoryKeyCertificateImpl.java | 2 -- .../descriptor/impl/DownloadCoordinatorImpl.java | 9 +++++++-- .../descriptor/impl/MicrodescriptorImpl.java | 1 - .../descriptor/impl/NetworkStatusEntryImpl.java | 1 - .../descriptor/impl/ServerDescriptorImpl.java | 1 - 8 files changed, 29 insertions(+), 22 deletions(-) diff --git a/build.xml b/build.xml index bb48dde..bfede7c 100644 --- a/build.xml +++ b/build.xml @@ -26,6 +26,7 @@ <patternset id="runtime" > <include name="commons-compress-1.9.jar"/> + <include name="slf4j-api-1.7.7.jar" /> <include name="xz-1.5.jar"/> </patternset> @@ -52,6 +53,8 @@ <fileset dir="${libs}"> <include name="junit4-4.11.jar"/> <include name="hamcrest-core-1.3.jar"/> + <include name="logback-core-1.1.2.jar" /> + <include name="logback-classic-1.1.2.jar" /> </fileset> </path> diff --git a/src/main/java/org/torproject/descriptor/impl/DescriptorCollectorImpl.java b/src/main/java/org/torproject/descriptor/impl/DescriptorCollectorImpl.java index 1b35d5f..f9bd60d 100644 --- a/src/main/java/org/torproject/descriptor/impl/DescriptorCollectorImpl.java +++ b/src/main/java/org/torproject/descriptor/impl/DescriptorCollectorImpl.java @@ -5,6 +5,9 @@ package org.torproject.descriptor.impl; import org.torproject.descriptor.DescriptorCollector; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + import java.io.BufferedInputStream; import java.io.BufferedOutputStream; import java.io.BufferedReader; @@ -31,6 +34,9 @@ import java.util.zip.GZIPInputStream; public class DescriptorCollectorImpl implements DescriptorCollector { + private static Logger log = LoggerFactory + .getLogger(DescriptorCollectorImpl.class); + @Override public void collectDescriptors(String collecTorBaseUrl, String[] remoteDirectories, long minLastModified, @@ -130,7 +136,7 @@ public class DescriptorCollectorImpl implements DescriptorCollector { br.close(); } } catch (IOException e) { - e.printStackTrace(); + log.error("Cannot fetch remote directory.", e); if (huc != null) { huc.disconnect(); } @@ -163,7 +169,7 @@ public class DescriptorCollectorImpl implements DescriptorCollector { } scanner.close(); } catch (ParseException e) { - e.printStackTrace(); + log.error("Cannot parse directory listing: " + directoryListing, e); return null; } return remoteFiles; @@ -225,7 +231,7 @@ public class DescriptorCollectorImpl implements DescriptorCollector { destinationFile.setLastModified(lastModifiedMillis); } } catch (IOException e) { - e.printStackTrace(); + log.error("Cannot fetch remote file.", e); if (huc != null) { huc.disconnect(); } diff --git a/src/main/java/org/torproject/descriptor/impl/DescriptorReaderImpl.java b/src/main/java/org/torproject/descriptor/impl/DescriptorReaderImpl.java index e3c07e5..fac9475 100644 --- a/src/main/java/org/torproject/descriptor/impl/DescriptorReaderImpl.java +++ b/src/main/java/org/torproject/descriptor/impl/DescriptorReaderImpl.java @@ -14,6 +14,9 @@ import org.apache.commons.compress.archivers.tar.TarArchiveInputStream; import org.apache.commons.compress.compressors.bzip2.BZip2CompressorInputStream; import org.apache.commons.compress.compressors.xz.XZCompressorInputStream; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + import java.io.BufferedInputStream; import java.io.BufferedReader; import java.io.BufferedWriter; @@ -34,6 +37,7 @@ import java.util.TreeMap; public class DescriptorReaderImpl implements DescriptorReader { + private static Logger log = LoggerFactory.getLogger(DescriptorReaderImpl.class); private boolean hasStartedReading = false; private List<File> directories = new ArrayList<>(); @@ -185,12 +189,8 @@ public class DescriptorReaderImpl implements DescriptorReader { this.readTarballs(); this.hasFinishedReading = true; } catch (Throwable t) { - /* We're usually not writing to stdout or stderr, but we shouldn't - * stay quiet about this potential bug. If we were to switch to a - * logging API, this would qualify as ERROR. */ - System.err.println("Bug: uncaught exception or error while " - + "reading descriptors:"); - t.printStackTrace(); + log.error("Bug: uncaught exception or error while " + + "reading descriptors: " + t.getMessage(), t); } finally { this.descriptorQueue.setOutOfDescriptors(); } @@ -209,7 +209,7 @@ public class DescriptorReaderImpl implements DescriptorReader { String line; while ((line = br.readLine()) != null) { if (!line.contains(" ")) { - /* TODO Handle this problem? */ + log.warn("Unexpected line structure in old history: " + line); continue; } long lastModifiedMillis = Long.parseLong(line.substring(0, @@ -218,10 +218,8 @@ public class DescriptorReaderImpl implements DescriptorReader { this.excludedFilesBefore.put(absolutePath, lastModifiedMillis); } br.close(); - } catch (IOException e) { - /* TODO Handle this exception. */ - } catch (NumberFormatException e) { - /* TODO Handle this exception. */ + } catch (IOException | NumberFormatException e) { + log.warn("Trouble reading old history.", e); } } @@ -246,7 +244,7 @@ public class DescriptorReaderImpl implements DescriptorReader { } bw.close(); } catch (IOException e) { - /* TODO Handle this exception. */ + log.warn("Trouble writing new history.", e); } } diff --git a/src/main/java/org/torproject/descriptor/impl/DirectoryKeyCertificateImpl.java b/src/main/java/org/torproject/descriptor/impl/DirectoryKeyCertificateImpl.java index 21a992d..443bd8b 100644 --- a/src/main/java/org/torproject/descriptor/impl/DirectoryKeyCertificateImpl.java +++ b/src/main/java/org/torproject/descriptor/impl/DirectoryKeyCertificateImpl.java @@ -18,8 +18,6 @@ import java.util.Set; import javax.xml.bind.DatatypeConverter; -/* TODO Add test class. */ - public class DirectoryKeyCertificateImpl extends DescriptorImpl implements DirectoryKeyCertificate { diff --git a/src/main/java/org/torproject/descriptor/impl/DownloadCoordinatorImpl.java b/src/main/java/org/torproject/descriptor/impl/DownloadCoordinatorImpl.java index 23826aa..05e3936 100644 --- a/src/main/java/org/torproject/descriptor/impl/DownloadCoordinatorImpl.java +++ b/src/main/java/org/torproject/descriptor/impl/DownloadCoordinatorImpl.java @@ -8,6 +8,9 @@ import org.torproject.descriptor.DescriptorRequest; import org.torproject.descriptor.DirSourceEntry; import org.torproject.descriptor.RelayNetworkStatusConsensus; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + import java.util.HashMap; import java.util.HashSet; import java.util.Iterator; @@ -20,6 +23,9 @@ import java.util.TreeSet; /* TODO This whole download logic is a mess and needs a cleanup. */ public class DownloadCoordinatorImpl implements DownloadCoordinator { + private static Logger log = LoggerFactory + .getLogger(DownloadCoordinatorImpl.class); + private BlockingIteratorImpl<DescriptorRequest> descriptorQueue = new BlockingIteratorImpl<>(); @@ -71,8 +77,7 @@ public class DownloadCoordinatorImpl implements DownloadCoordinator { if (this.directoryMirrors.isEmpty() && this.directoryAuthorities.isEmpty()) { this.descriptorQueue.setOutOfDescriptors(); - /* TODO Should we say anything if we don't have any directories - * configured? */ + log.warn("There were no directories configured. Nothing to download."); } else { GlobalTimer globalTimer = new GlobalTimer(this.globalTimeoutMillis, this); diff --git a/src/main/java/org/torproject/descriptor/impl/MicrodescriptorImpl.java b/src/main/java/org/torproject/descriptor/impl/MicrodescriptorImpl.java index ab859a0..ecad5cb 100644 --- a/src/main/java/org/torproject/descriptor/impl/MicrodescriptorImpl.java +++ b/src/main/java/org/torproject/descriptor/impl/MicrodescriptorImpl.java @@ -147,7 +147,6 @@ public class MicrodescriptorImpl extends DescriptorImpl + "'" + line + "'."); } /* TODO Add more checks. */ - /* TODO Add tests. */ this.orAddresses.add(parts[1]); } diff --git a/src/main/java/org/torproject/descriptor/impl/NetworkStatusEntryImpl.java b/src/main/java/org/torproject/descriptor/impl/NetworkStatusEntryImpl.java index 807dd76..f528033 100644 --- a/src/main/java/org/torproject/descriptor/impl/NetworkStatusEntryImpl.java +++ b/src/main/java/org/torproject/descriptor/impl/NetworkStatusEntryImpl.java @@ -154,7 +154,6 @@ public class NetworkStatusEntryImpl implements NetworkStatusEntry { + "status entry."); } /* TODO Add more checks. */ - /* TODO Add tests. */ this.orAddresses.add(parts[1]); } diff --git a/src/main/java/org/torproject/descriptor/impl/ServerDescriptorImpl.java b/src/main/java/org/torproject/descriptor/impl/ServerDescriptorImpl.java index 08b9271..622e9a4 100644 --- a/src/main/java/org/torproject/descriptor/impl/ServerDescriptorImpl.java +++ b/src/main/java/org/torproject/descriptor/impl/ServerDescriptorImpl.java @@ -260,7 +260,6 @@ public abstract class ServerDescriptorImpl extends DescriptorImpl + "'" + line + "'."); } /* TODO Add more checks. */ - /* TODO Add tests. */ this.orAddresses.add(partsNoOpt[1]); }