From d50ff5ab300773b982ae74c9418f876e8dc0e417 Mon Sep 17 00:00:00 2001 From: chenheng Date: Tue, 25 Aug 2015 16:50:14 +0800 Subject: [PATCH] HBASE-14279 Race condition in ConcurrentIndex --- .../apache/hadoop/hbase/util/ConcurrentIndex.java | 60 ++++++++++++++-------- 1 file changed, 38 insertions(+), 22 deletions(-) diff --git a/hbase-common/src/main/java/org/apache/hadoop/hbase/util/ConcurrentIndex.java b/hbase-common/src/main/java/org/apache/hadoop/hbase/util/ConcurrentIndex.java index 3b4a1f1..48abf1e 100644 --- a/hbase-common/src/main/java/org/apache/hadoop/hbase/util/ConcurrentIndex.java +++ b/hbase-common/src/main/java/org/apache/hadoop/hbase/util/ConcurrentIndex.java @@ -49,8 +49,12 @@ import com.google.common.base.Supplier; @InterfaceStability.Evolving public class ConcurrentIndex { - /** Container for the sets, indexed by key */ - private final ConcurrentMap> container; + /** Container for the sets, indexed by key + * value is Pair>, + * value.getFirst() is lock for the key + * value.getSecond() is the set. + * */ + private final ConcurrentMap>> container; /** * A factory that constructs new instances of the sets if no set is @@ -65,7 +69,7 @@ public class ConcurrentIndex { */ public ConcurrentIndex(Supplier> valueSetFactory) { this.valueSetFactory = valueSetFactory; - this.container = new ConcurrentHashMap>(); + this.container = new ConcurrentHashMap>>(); } /** @@ -87,23 +91,27 @@ public class ConcurrentIndex { * @param value An additional unique value we want to associate with a key */ public void put(K key, V value) { - Set set = container.get(key); - if (set != null) { - set.add(value); - } else { - set = valueSetFactory.get(); - set.add(value); - Set existing = container.putIfAbsent(key, set); - if (existing != null) { - // If a set is already associated with a key, that means another - // writer has already come in and created the set for the given key. - // Pursuant to an optimistic concurrency policy, in this case we will - // simply add the value to the existing set associated with the key. - existing.add(value); + Pair> lockAndSet = createLockAndSet(); + lockAndSet.getSecond().add(value); + Pair> existing = container.putIfAbsent(key, lockAndSet); + if (existing != null) { + synchronized (existing.getFirst()) { + existing = container.get(key); //Re check if the key is associate with the set. + if (existing == null) { + container.put(key, lockAndSet); + } else { + existing.getSecond().add(value); + } } } + + } + + private Pair> createLockAndSet() { + return new Pair>(new Object(), valueSetFactory.get()); } + /** * Get all values associated with a specified key or null if no values are * associated. Note: if the caller wishes to add or removes values @@ -115,7 +123,11 @@ public class ConcurrentIndex { * are associated with the key. */ public Set values(K key) { - return container.get(key); + Pair> lockAndSet = container.get(key); + if (lockAndSet != null) { + return lockAndSet.getSecond(); + } + return null; } /** @@ -126,12 +138,16 @@ public class ConcurrentIndex { * @param value The value to disassociate with the key */ public boolean remove(K key, V value) { - Set set = container.get(key); + Pair> lockAndSet = container.get(key); boolean success = false; - if (set != null) { - success = set.remove(value); - if (set.isEmpty()) { - container.remove(key); + if (lockAndSet != null) { + success = lockAndSet.getSecond().remove(value); + if (lockAndSet.getSecond().isEmpty()) { + synchronized (lockAndSet.getFirst()) { + if (lockAndSet.getSecond().isEmpty()) { //Re check if the set is empty + container.remove(key); + } + } } } return success; -- 1.9.3 (Apple Git-50)