[tor-commits] [metrics-lib/master] Make NetworkStatusEntryImpl#parseSLine thread-safe.

[karsten at torproject.org]

commit bde697f44702c3f93cfff7953ff8f5962582e2e1
Author: Karsten Loesing <karsten.loesing at gmx.net>
Date:   Thu Oct 24 21:29:07 2019 +0200

    Make NetworkStatusEntryImpl#parseSLine thread-safe.
    
    The bug was that we accessed static class members, namely the two maps
    NetworkStatusEntryImpl#flagIndexes and #flagStrings, during instance
    creation without synchronization. This worked just fine with a single
    thread creating instances, but it breaks with multiple threads doing
    that at the same time.
    
    The fix is to keep a separate map per NetworkStatusImpl instance and
    share that between all its NetworkStatusEntryImpl instances. This
    doesn't save as much memory as sharing maps between all
    NetworksStatusEntryImpl instances ever created, but it's a reasonable
    compromise between memory and runtime efficiency. In contrast to that,
    synchronizing map access would have put a major runtime performance
    penalty on parsing.
    
    Fixes #32194.
---
 CHANGELOG.md                                       |  3 +++
 .../descriptor/impl/NetworkStatusEntryImpl.java    | 27 +++++++++++-----------
 .../descriptor/impl/NetworkStatusImpl.java         |  8 ++++++-
 .../impl/RelayNetworkStatusConsensusImpl.java      |  3 ++-
 4 files changed, 26 insertions(+), 15 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 9c7610a..73022cf 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,8 @@
 # Changes in version 2.?.? - 2019-??-??
 
+ * Medium changes
+   - Make NetworkStatusEntryImpl#parseSLine thread-safe.
+
 
 # Changes in version 2.8.0 - 2019-10-18
 
diff --git a/src/main/java/org/torproject/descriptor/impl/NetworkStatusEntryImpl.java b/src/main/java/org/torproject/descriptor/impl/NetworkStatusEntryImpl.java
index 2ddaaa0..e6a78f6 100644
--- a/src/main/java/org/torproject/descriptor/impl/NetworkStatusEntryImpl.java
+++ b/src/main/java/org/torproject/descriptor/impl/NetworkStatusEntryImpl.java
@@ -12,7 +12,6 @@ import org.torproject.descriptor.NetworkStatusEntry;
 import java.util.ArrayList;
 import java.util.BitSet;
 import java.util.EnumSet;
-import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
@@ -48,13 +47,19 @@ public class NetworkStatusEntryImpl implements NetworkStatusEntry {
     return lines;
   }
 
+  private Map<String, Integer> flagIndexes;
+
+  private Map<Integer, String> flagStrings;
+
   protected NetworkStatusEntryImpl(DescriptorImpl parent, int offset,
-      int length, boolean microdescConsensus)
-      throws DescriptorParseException {
+      int length, boolean microdescConsensus, Map<String, Integer> flagIndexes,
+      Map<Integer, String> flagStrings) throws DescriptorParseException {
     this.parent = parent;
     this.offset = offset;
     this.length = length;
     this.microdescConsensus = microdescConsensus;
+    this.flagIndexes = flagIndexes;
+    this.flagStrings = flagStrings;
     this.parseStatusEntryBytes();
     this.clearAtMostOnceKeys();
   }
@@ -160,21 +165,17 @@ public class NetworkStatusEntryImpl implements NetworkStatusEntry {
     this.orAddresses.add(parts[1]);
   }
 
-  private static Map<String, Integer> flagIndexes = new HashMap<>();
-
-  private static Map<Integer, String> flagStrings = new HashMap<>();
-
   private void parseSLine(String[] parts)
       throws DescriptorParseException {
     this.parsedAtMostOnceKey(Key.S);
-    BitSet flags = new BitSet(flagIndexes.size());
+    BitSet flags = new BitSet(this.flagIndexes.size());
     for (int i = 1; i < parts.length; i++) {
       String flag = parts[i];
-      if (!flagIndexes.containsKey(flag)) {
-        flagStrings.put(flagIndexes.size(), flag);
-        flagIndexes.put(flag, flagIndexes.size());
+      if (!this.flagIndexes.containsKey(flag)) {
+        this.flagStrings.put(this.flagIndexes.size(), flag);
+        this.flagIndexes.put(flag, this.flagIndexes.size());
       }
-      flags.set(flagIndexes.get(flag));
+      flags.set(this.flagIndexes.get(flag));
     }
     this.flags = flags;
   }
@@ -353,7 +354,7 @@ public class NetworkStatusEntryImpl implements NetworkStatusEntry {
     if (this.flags != null) {
       for (int i = this.flags.nextSetBit(0); i >= 0;
           i = this.flags.nextSetBit(i + 1)) {
-        result.add(flagStrings.get(i));
+        result.add(this.flagStrings.get(i));
       }
     }
     return result;
diff --git a/src/main/java/org/torproject/descriptor/impl/NetworkStatusImpl.java b/src/main/java/org/torproject/descriptor/impl/NetworkStatusImpl.java
index 0694aff..b763f30 100644
--- a/src/main/java/org/torproject/descriptor/impl/NetworkStatusImpl.java
+++ b/src/main/java/org/torproject/descriptor/impl/NetworkStatusImpl.java
@@ -10,7 +10,9 @@ import org.torproject.descriptor.NetworkStatusEntry;
 
 import java.io.File;
 import java.util.ArrayList;
+import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
 import java.util.SortedMap;
 import java.util.TreeMap;
 
@@ -19,6 +21,10 @@ import java.util.TreeMap;
  * delegate the specific parts to the subclasses. */
 public abstract class NetworkStatusImpl extends DescriptorImpl {
 
+  protected Map<String, Integer> flagIndexes = new HashMap<>();
+
+  protected Map<Integer, String> flagStrings = new HashMap<>();
+
   protected NetworkStatusImpl(byte[] rawDescriptorBytes, int[] offsetAndLength,
       File descriptorFile, boolean containsDirSourceEntries,
       boolean blankLinesAllowed) throws DescriptorParseException {
@@ -140,7 +146,7 @@ public abstract class NetworkStatusImpl extends DescriptorImpl {
   protected void parseStatusEntry(int offset, int length)
       throws DescriptorParseException {
     NetworkStatusEntryImpl statusEntry = new NetworkStatusEntryImpl(
-        this, offset, length, false);
+        this, offset, length, false, this.flagIndexes, this.flagStrings);
     this.statusEntries.put(statusEntry.getFingerprint(), statusEntry);
     List<String> unrecognizedStatusEntryLines = statusEntry
         .getAndClearUnrecognizedLines();
diff --git a/src/main/java/org/torproject/descriptor/impl/RelayNetworkStatusConsensusImpl.java b/src/main/java/org/torproject/descriptor/impl/RelayNetworkStatusConsensusImpl.java
index 904dc35..66b8eaa 100644
--- a/src/main/java/org/torproject/descriptor/impl/RelayNetworkStatusConsensusImpl.java
+++ b/src/main/java/org/torproject/descriptor/impl/RelayNetworkStatusConsensusImpl.java
@@ -119,7 +119,8 @@ public class RelayNetworkStatusConsensusImpl extends NetworkStatusImpl
   protected void parseStatusEntry(int offset, int length)
       throws DescriptorParseException {
     NetworkStatusEntryImpl statusEntry = new NetworkStatusEntryImpl(this,
-        offset, length, this.microdescConsensus);
+        offset, length, this.microdescConsensus, this.flagIndexes,
+        this.flagStrings);
     this.statusEntries.put(statusEntry.getFingerprint(), statusEntry);
     List<String> unrecognizedStatusEntryLines = statusEntry
         .getAndClearUnrecognizedLines();