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 264064b7c19e5461e70f72bae1a4d6e25793966f..11167b8db3f6bb0cdaba087178a043f6fa3d4c89 100644 (file)
--- 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 9d57d62100c633d727570b9f6aa456ba392668f7..16e17e1401f3c6d2a0a288ea41989660e05c203e 100644 (file)
--- 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() {