[onionoo/release] Stop sharing the JNDI DirContext - tor-commits

3 Aug 2018

commit 2b6916bdc0249d129b5aa08ab84e20e892ad7ad5
Author: Iain R. Learmonth irl@fsfe.org
Date:   Tue Jul 31 14:50:42 2018 +0100
Stop sharing the JNDI DirContext
In the implementation of more accurate DNS resolution, the DirContext
    used by JNDI lookups was shared in a utility class. It turns out that
    InitialDirContext, the implementation we used, is very not thread-safe
    and so as a result lookups would often fail to complete.
This commit creates a new DirContext for every lookup. The overhead in
    doing so is minimal and ensures that we do not have thread safety
    issues.
This commit also increases RDNS_LOOKUP_WORKERS_NUM to 30 (from 5). The
    majority of time in these threads is spent waiting for replies and so
    there are gains to be had in increasing the number of requests in
    flight.
Fixes: #26963
---
 .../onionoo/updater/RdnsLookupRequest.java         | 48 ++++++++++++----
 .../onionoo/updater/RdnsLookupWorker.java          |  2 +-
 .../onionoo/updater/ReverseDomainNameResolver.java | 10 +---
 .../torproject/onionoo/util/DomainNameSystem.java  | 66 ----------------------
 4 files changed, 38 insertions(+), 88 deletions(-)

diff --git a/src/main/java/org/torproject/onionoo/updater/RdnsLookupRequest.java b/src/main/java/org/torproject/onionoo/updater/RdnsLookupRequest.java
index 7f14350..3859262 100644
--- a/src/main/java/org/torproject/onionoo/updater/RdnsLookupRequest.java
+++ b/src/main/java/org/torproject/onionoo/updater/RdnsLookupRequest.java
@@ -3,19 +3,22 @@
package org.torproject.onionoo.updater;
-import org.torproject.onionoo.util.DomainNameSystem;
-
 import java.util.ArrayList;
+import java.util.Hashtable;
 import java.util.List;
+import java.util.Set;
+import java.util.TreeSet;
+import javax.naming.Context;
+import javax.naming.NamingEnumeration;
 import javax.naming.NamingException;
+import javax.naming.directory.Attribute;
+import javax.naming.directory.Attributes;
+import javax.naming.directory.DirContext;
+import javax.naming.directory.InitialDirContext;
class RdnsLookupRequest extends Thread {
-  private ReverseDomainNameResolver reverseDomainNameResolver;
-
-  private DomainNameSystem domainNameSystem;
-
   private RdnsLookupWorker parent;
private String address;
@@ -29,11 +32,7 @@ class RdnsLookupRequest extends Thread {
   private long lookupCompletedMillis = -1L;
public RdnsLookupRequest(
-      ReverseDomainNameResolver reverseDomainNameResolver,
       RdnsLookupWorker parent, String address) {
-    this.reverseDomainNameResolver = reverseDomainNameResolver;
-    this.domainNameSystem =
-            this.reverseDomainNameResolver.getDomainNameSystemInstance();
     this.parent = parent;
     this.address = address;
   }
@@ -51,11 +50,11 @@ class RdnsLookupRequest extends Thread {
                         + "." + bytes[2] + "." + bytes[1] + "." + bytes[0]
                         + ".in-addr.arpa";
         String[] reverseDnsRecords =
-            this.domainNameSystem.getRecords(reverseDnsDomain, "PTR");
+            this.getRecords(reverseDnsDomain, "PTR");
         for (String reverseDnsRecord : reverseDnsRecords) {
           boolean verified = false;
           String[] forwardDnsRecords =
-              this.domainNameSystem.getRecords(reverseDnsRecord, "A");
+              this.getRecords(reverseDnsRecord, "A");
           for (String forwardDnsRecord : forwardDnsRecords) {
             if (forwardDnsRecord.equals(this.address)) {
               verified = true;
@@ -83,6 +82,31 @@ class RdnsLookupRequest extends Thread {
     this.parent.interrupt();
   }
+  /**
+   * Returns all the values of DNS resource records found for a given host name
+   * and type.
+   */
+  public String[] getRecords(String hostName, String type)
+          throws NamingException {
+    Hashtable<String, String> envProps = new Hashtable<>();
+    envProps.put(Context.INITIAL_CONTEXT_FACTORY,
+        "com.sun.jndi.dns.DnsContextFactory");
+    final DirContext dnsContext = new InitialDirContext(envProps);
+    Set<String> results = new TreeSet<String>();
+    Attributes dnsEntries =
+        dnsContext.getAttributes(hostName, new String[] { type });
+    if (dnsEntries != null) {
+      Attribute dnsAttribute = dnsEntries.get(type);
+      if (dnsAttribute != null) {
+        NamingEnumeration<?> dnsEntryIterator = dnsEntries.get(type).getAll();
+        while (dnsEntryIterator.hasMoreElements()) {
+          results.add(dnsEntryIterator.next().toString());
+        }
+      }
+    }
+    return results.toArray(new String[results.size()]);
+  }
+
   public synchronized String getHostName() {
     List<String> verifiedHostNames = this.verifiedHostNames;
     if (null != verifiedHostNames && !verifiedHostNames.isEmpty() ) {
diff --git a/src/main/java/org/torproject/onionoo/updater/RdnsLookupWorker.java b/src/main/java/org/torproject/onionoo/updater/RdnsLookupWorker.java
index c94c334..953fdd2 100644
--- a/src/main/java/org/torproject/onionoo/updater/RdnsLookupWorker.java
+++ b/src/main/java/org/torproject/onionoo/updater/RdnsLookupWorker.java
@@ -30,7 +30,7 @@ class RdnsLookupWorker extends Thread {
         break;
       }
       RdnsLookupRequest request = new RdnsLookupRequest(
-          this.reverseDomainNameResolver, this, rdnsLookupJob);
+          this, rdnsLookupJob);
       request.setDaemon(true);
       request.start();
       try {
diff --git a/src/main/java/org/torproject/onionoo/updater/ReverseDomainNameResolver.java b/src/main/java/org/torproject/onionoo/updater/ReverseDomainNameResolver.java
index 698b637..4022dab 100644
--- a/src/main/java/org/torproject/onionoo/updater/ReverseDomainNameResolver.java
+++ b/src/main/java/org/torproject/onionoo/updater/ReverseDomainNameResolver.java
@@ -3,7 +3,6 @@
package org.torproject.onionoo.updater;
-import org.torproject.onionoo.util.DomainNameSystem;
 import org.torproject.onionoo.util.FormattingUtils;
import java.util.ArrayList;
@@ -23,9 +22,7 @@ public class ReverseDomainNameResolver {
   private static final long RDNS_LOOKUP_MAX_AGE_MILLIS =
       12L * 60L * 60L * 1000L;
-  private static final int RDNS_LOOKUP_WORKERS_NUM = 5;
-
-  private DomainNameSystem domainNameSystem;
+  private static final int RDNS_LOOKUP_WORKERS_NUM = 30;
private Map<String, Long> addressLastLookupTimes;
@@ -50,7 +47,6 @@ public class ReverseDomainNameResolver {
   /** Starts reverse domain name lookups in one or more background
    * threads and returns immediately. */
   public void startReverseDomainNameLookups() {
-    this.domainNameSystem = new DomainNameSystem();
     this.startedRdnsLookups = System.currentTimeMillis();
     this.rdnsLookupJobs = new HashSet<>();
     for (Map.Entry<String, Long> e :
@@ -113,10 +109,6 @@ public class ReverseDomainNameResolver {
     return this.startedRdnsLookups;
   }
-  public DomainNameSystem getDomainNameSystemInstance() {
-    return this.domainNameSystem;
-  }
-
   /** Returns a string with the number of performed reverse domain name
    * lookups and some simple statistics on lookup time. */
   public String getStatsString() {
diff --git a/src/main/java/org/torproject/onionoo/util/DomainNameSystem.java b/src/main/java/org/torproject/onionoo/util/DomainNameSystem.java
deleted file mode 100644
index b085963..0000000
--- a/src/main/java/org/torproject/onionoo/util/DomainNameSystem.java
+++ /dev/null
@@ -1,66 +0,0 @@
-/* Copyright 2018 The Tor Project
- * See LICENSE for licensing information */
-
-package org.torproject.onionoo.util;
-
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
-
-import java.util.Hashtable;
-import java.util.Set;
-import java.util.TreeSet;
-
-import javax.naming.Context;
-import javax.naming.NamingEnumeration;
-import javax.naming.NamingException;
-import javax.naming.directory.Attribute;
-import javax.naming.directory.Attributes;
-import javax.naming.directory.DirContext;
-import javax.naming.directory.InitialDirContext;
-
-public class DomainNameSystem {
-
-  private DirContext dnsContext;
-  private Logger log;
-
-  /** Creates a new instance. */
-  public DomainNameSystem() {
-    log = LoggerFactory.getLogger(DomainNameSystem.class);
-    Hashtable<String, String> envProps = new Hashtable<>();
-    envProps.put(Context.INITIAL_CONTEXT_FACTORY,
-        "com.sun.jndi.dns.DnsContextFactory");
-    try {
-      dnsContext = new InitialDirContext(envProps);
-    } catch (NamingException e) {
-      log.error(
-          "Unable to create directory context. "
-          + "Host name lookup will be disabled!");
-    }
-  }
-
-  /**
-   * Returns all the values of DNS resource records found for a given host name
-   * and type.
-   */
-  public String[] getRecords(String hostName, String type)
-          throws NamingException {
-    if (dnsContext == null) {
-      /* Initial setup failed, so all lookups will fail. */
-      throw new NamingException();
-    }
-    Set<String> results = new TreeSet<String>();
-    Attributes dnsEntries =
-        dnsContext.getAttributes(hostName, new String[] { type });
-    if (dnsEntries != null) {
-      Attribute dnsAttribute = dnsEntries.get(type);
-      if (dnsAttribute != null) {
-        NamingEnumeration<?> dnsEntryIterator = dnsEntries.get(type).getAll();
-        while (dnsEntryIterator.hasMoreElements()) {
-          results.add(dnsEntryIterator.next().toString());
-        }
-      }
-    }
-    return results.toArray(new String[results.size()]);
-  }
-
-}

    