Synchronize all appropriate methods in ConsistentHash.

Only add(T, int) was synchronized (and its javadoc claimed it was the
only one modifying .items), but all the bulk add methods were going
straight to addInternal() and then refreshTable() on their own. Then
countAllPoints() was iterating over items.values() which has undefined
behavior if the underlying Map changed.

Technically I didn't need to synchronize the current implementations of
remove(T) and add(T) since they delegated fully to add(T, int), but
that's leaking implementation details and inviting trouble down the road
when someone doesn't realize that.

Likewise, addInternal() and refreshTable() need not be synchronized
since they're only ever called from already synchronized methods, but
I'm inclined to go for belt & suspenders in the wake of these other
issues.
This commit is contained in:
David Hoover committed 2015-04-19 14:39:01 -07:00
1 parent d7f83ec264
commit aea48a9d7b
2 files changed
+29 -17

No files matched your search

@@ -122,11 +122,24 @@ public class ConsistentHashTest {
@Test
public void emptyBehavior() {
ConsistentHash<String> hash = new ConsistentHash<String>();
assertEquals(0, hash.countAllPoints());
assertFalse(hash.list(0).iterator().hasNext());
assertNull(hash.lookup(0));
assertNull(hash.lookup(999));
}
@Test
public void countAllPoints() {
ConsistentHash<String> hash = new ConsistentHash<String>();
assertEquals(0, hash.countAllPoints());
hash.add("foo", 10);
assertEquals(10, hash.countAllPoints());
hash.add("bar", 5);
assertEquals(15, hash.countAllPoints());
hash.remove("foo");
assertEquals(5, hash.countAllPoints());
}
/**
* This test doesn't fail but it's written to measure the performance of the consistent hash function with large data set.
*/