From: Mike Kolupaev <kolmike@fb.com>
Date: Fri, 19 Dec 2014 20:17:41 +0000 (-0800)
Subject: Fixed folly::AtomicHashMap::iterator not advancing past empty submaps
X-Git-Tag: v0.22.0~81
X-Git-Url: http://demsky.eecs.uci.edu/git/?a=commitdiff_plain;h=d8d3e2676a073e415ecbfeb3144948ddcd6449df;p=folly.git

Fixed folly::AtomicHashMap::iterator not advancing past empty submaps

Summary: A potential "real life" scenario that maybe can hit this bug is if we erase almost all elements and then iterate over the whole map.

Test Plan: Added a test for it.

Reviewed By: mwilliams@fb.com

Subscribers: folly-diffs@, lovro

FB internal diff: D1751455

Tasks: 5866368

Signature: t1:1751455:1419016611:b44c41d348f54397844460cb38002ad0d9704972
---

diff --git a/folly/AtomicHashMap-inl.h b/folly/AtomicHashMap-inl.h
index 264064b7..11167b8d 100644
--- a/folly/AtomicHashMap-inl.h
+++ b/folly/AtomicHashMap-inl.h
@@ -408,7 +408,7 @@ struct AtomicHashMap<KeyT, ValueT, HashFcn, EqualFcn, Allocator>::ahm_iterator
 
     SubMap* thisMap = ahm_->subMaps_[subMap_].
       load(std::memory_order_relaxed);
-    if (subIt_ == thisMap->end()) {
+    while (subIt_ == thisMap->end()) {
       // This sub iterator is done, advance to next one
       if (subMap_ + 1 <
           ahm_->numMapsAllocated_.load(std::memory_order_acquire)) {
@@ -417,6 +417,7 @@ struct AtomicHashMap<KeyT, ValueT, HashFcn, EqualFcn, Allocator>::ahm_iterator
         subIt_ = thisMap->begin();
       } else {
         ahm_ = nullptr;
+        return;
       }
     }
   }
diff --git a/folly/test/AtomicHashMapTest.cpp b/folly/test/AtomicHashMapTest.cpp
index 9d57d621..16e17e14 100644
--- a/folly/test/AtomicHashMapTest.cpp
+++ b/folly/test/AtomicHashMapTest.cpp
@@ -618,6 +618,35 @@ TEST(Ahm, atomic_hash_array_insert_race) {
   }
 }
 
+// Repro for a bug when iterator didn't skip empty submaps.
+TEST(Ahm, iterator_skips_empty_submaps) {
+  AtomicHashMap<uint64_t, uint64_t>::Config config;
+  config.growthFactor = 1;
+
+  AtomicHashMap<uint64_t, uint64_t> map(1, config);
+
+  map.insert(1, 1);
+  map.insert(2, 2);
+  map.insert(3, 3);
+
+  map.erase(2);
+
+  auto it = map.find(1);
+
+  ASSERT_NE(map.end(), it);
+  ASSERT_EQ(1, it->first);
+  ASSERT_EQ(1, it->second);
+
+  ++it;
+
+  ASSERT_NE(map.end(), it);
+  ASSERT_EQ(3, it->first);
+  ASSERT_EQ(3, it->second);
+
+  ++it;
+  ASSERT_EQ(map.end(), it);
+}
+
 namespace {
 
 void loadGlobalAha() {