Address thread safety concerns with SparseEdgeMap

sharwell · Aug 5, 2014 · 4deb0d0 · 4deb0d0
1 parent 5e03746
commit 4deb0d0
Show file tree

Hide file tree

Showing 2 changed files with 45 additions and 86 deletions.
diff --git a/runtime/Java/src/org/antlr/v4/runtime/dfa/ArrayEdgeMap.java b/runtime/Java/src/org/antlr/v4/runtime/dfa/ArrayEdgeMap.java
@@ -118,13 +118,15 @@ public ArrayEdgeMap<T> putAll(EdgeMap<? extends T> m) {
 			return put(other.getKey(), other.getValue());
 		} else if (m instanceof SparseEdgeMap<?>) {
 			SparseEdgeMap<? extends T> other = (SparseEdgeMap<? extends T>)m;
-			int[] keys = other.getKeys();
-			List<? extends T> values = other.getValues();
-			ArrayEdgeMap<T> result = this;
-			for (int i = 0; i < values.size(); i++) {
-				result = result.put(keys[i], values.get(i));
+			synchronized (other) {
+				int[] keys = other.getKeys();
+				List<? extends T> values = other.getValues();
+				ArrayEdgeMap<T> result = this;
+				for (int i = 0; i < values.size(); i++) {
+					result = result.put(keys[i], values.get(i));
+				}
+				return result;
 			}
-			return result;
 		} else {
 			throw new UnsupportedOperationException(String.format("EdgeMap of type %s is supported yet.", m.getClass().getName()));
 		}

diff --git a/runtime/Java/src/org/antlr/v4/runtime/dfa/SparseEdgeMap.java b/runtime/Java/src/org/antlr/v4/runtime/dfa/SparseEdgeMap.java
@@ -32,11 +32,9 @@
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
-import java.util.Iterator;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
-import java.util.NoSuchElementException;
 import java.util.Set;
 
 /**
@@ -65,9 +63,11 @@ private SparseEdgeMap(@NotNull SparseEdgeMap<T> map, int maxSparseSize) {
 			throw new IllegalArgumentException();
 		}
 
-		keys = Arrays.copyOf(map.keys, maxSparseSize);
-		values = new ArrayList<T>(maxSparseSize);
-		values.addAll(map.values);
+		synchronized (map) {
+			keys = Arrays.copyOf(map.keys, maxSparseSize);
+			values = new ArrayList<T>(maxSparseSize);
+			values.addAll(map.values);
+		}
 	}
 
 	public int[] getKeys() {
@@ -98,17 +98,19 @@ public boolean containsKey(int key) {
 	}
 
 	@Override
-	public synchronized T get(int key) {
-		int index = Arrays.binarySearch(keys, 0, size(), key);
-		if (index < 0) {
-			return null;
-		}
+	public T get(int key) {
+		synchronized (this) {
+			int index = Arrays.binarySearch(keys, 0, size(), key);
+			if (index < 0) {
+				return null;
+			}
 
-		return values.get(index);
+			return values.get(index);
+		}
 	}
 
 	@Override
-	public synchronized AbstractEdgeMap<T> put(int key, T value) {
+	public AbstractEdgeMap<T> put(int key, T value) {
 		if (key < minIndex || key > maxIndex) {
 			return this;
 		}
@@ -117,7 +119,7 @@ public synchronized AbstractEdgeMap<T> put(int key, T value) {
 			return remove(key);
 		}
 
-		synchronized (values) {
+		synchronized (this) {
 			int index = Arrays.binarySearch(keys, 0, size(), key);
 			if (index >= 0) {
 				// replace existing entry
@@ -155,20 +157,22 @@ public synchronized AbstractEdgeMap<T> put(int key, T value) {
 
 	@Override
 	public SparseEdgeMap<T> remove(int key) {
-		int index = Arrays.binarySearch(keys, 0, size(), key);
-		if (index < 0) {
-			return this;
-		}
+		synchronized (this) {
+			int index = Arrays.binarySearch(keys, 0, size(), key);
+			if (index < 0) {
+				return this;
+			}
 
-		if (index == values.size() - 1) {
-			values.remove(index);
-			return this;
-		}
+			if (index == values.size() - 1) {
+				values.remove(index);
+				return this;
+			}
 
-		SparseEdgeMap<T> result = new SparseEdgeMap<T>(this, getMaxSparseSize());
-		System.arraycopy(result.keys, index + 1, result.keys, index, size() - index - 1);
-		result.values.remove(index);
-		return result;
+			SparseEdgeMap<T> result = new SparseEdgeMap<T>(this, getMaxSparseSize());
+			System.arraycopy(result.keys, index + 1, result.keys, index, size() - index - 1);
+			result.values.remove(index);
+			return result;
+		}
 	}
 
 	@Override
@@ -188,65 +192,18 @@ public Map<Integer, T> toMap() {
 			return Collections.emptyMap();
 		}
 
-		Map<Integer, T> result = new LinkedHashMap<Integer, T>();
-		for (int i = 0; i < size(); i++) {
-			result.put(keys[i], values.get(i));
-		}
+		synchronized (this) {
+			Map<Integer, T> result = new LinkedHashMap<Integer, T>();
+			for (int i = 0; i < size(); i++) {
+				result.put(keys[i], values.get(i));
+			}
 
-		return result;
+			return result;
+		}
 	}
 
 	@Override
 	public Set<Map.Entry<Integer, T>> entrySet() {
-		return new EntrySet();
-	}
-
-	private class EntrySet extends AbstractEntrySet {
-		@Override
-		public Iterator<Map.Entry<Integer, T>> iterator() {
-			return new EntryIterator();
-		}
-	}
-
-	private class EntryIterator implements Iterator<Map.Entry<Integer, T>> {
-		private int current;
-
-		@Override
-		public boolean hasNext() {
-			return current < size();
-		}
-
-		@Override
-		public Map.Entry<Integer, T> next() {
-			if (current >= size()) {
-				throw new NoSuchElementException();
-			}
-
-			current++;
-			return new Map.Entry<Integer, T>() {
-				private final int key = keys[current - 1];
-				private final T value = values.get(current - 1);
-
-				@Override
-				public Integer getKey() {
-					return key;
-				}
-
-				@Override
-				public T getValue() {
-					return value;
-				}
-
-				@Override
-				public T setValue(T value) {
-					throw new UnsupportedOperationException("Not supported yet.");
-				}
-			};
-		}
-
-		@Override
-		public void remove() {
-			throw new UnsupportedOperationException("Not supported yet.");
-		}
+		return toMap().entrySet();
 	}
 }