more ConcurrentHashMap deletion tests

Summary:
Fixes for hazptr deletion on map destruction.

Several iteration cases don't properly release their hazard pointers.
This isn't disastrous, but means objects may stick around longer than necessary.

This diff adds tests and fixes for iteration to ensure we eagerly reset all hazard pointers.

Reviewed By: magedm

Differential Revision: D6993857

fbshipit-source-id: bee63b3f597d1ed16cae5ed22a971fd4be2e1a77

folly/concurrency/ConcurrentHashMap.h

@@ -444,13 +444,17 @@ class ConcurrentHashMap {
     void next() {
       while (it_ == parent_->ensureSegment(segment_)->cend() &&
              segment_ < parent_->NumShards) {
-        segment_++;
-        auto seg = parent_->segments_[segment_].load(std::memory_order_acquire);
-        if (segment_ < parent_->NumShards) {
-          if (!seg) {
-            continue;
+        SegmentT* seg{nullptr};
+        while (!seg) {
+          segment_++;
+          seg = parent_->segments_[segment_].load(std::memory_order_acquire);
+          if (segment_ < parent_->NumShards) {
+            if (!seg) {
+              continue;
+            }
+            it_ = seg->cbegin();
           }
-          it_ = seg->cbegin();
+          break;
         }
       }
     }

folly/concurrency/detail/ConcurrentHashMap-detail.h

@@ -589,6 +589,8 @@ class alignas(64) ConcurrentHashMapSegment {
   // throw if hash or key_eq functions throw.
   void erase(Iterator& res, Iterator& pos) {
     erase_internal(pos->first, &res);
+    // Invalidate the iterator.
+    pos = cend();
   }
 
   void clear() {

folly/concurrency/test/ConcurrentHashMapTest.cpp

@@ -594,25 +594,107 @@ TEST(ConcurrentHashMap, RefcountTest) {
 }
 
 struct Wrapper {
-  Wrapper() = default;
+  explicit Wrapper(bool& del_) : del(del_) {}
   ~Wrapper() {
     del = true;
   }
 
-  static bool del;
+  bool& del;
 };
 
-bool Wrapper::del = false;
-
 TEST(ConcurrentHashMap, Deletion) {
-  EXPECT_FALSE(Wrapper::del);
+  bool del{false};
 
   {
     ConcurrentHashMap<int, std::shared_ptr<Wrapper>> map;
 
-    map.insert(0, std::make_shared<Wrapper>());
+    map.insert(0, std::make_shared<Wrapper>(del));
+  }
+
+  EXPECT_TRUE(del);
+}
+
+TEST(ConcurrentHashMap, DeletionWithErase) {
+  bool del{false};
+
+  {
+    ConcurrentHashMap<int, std::shared_ptr<Wrapper>> map;
+
+    map.insert(0, std::make_shared<Wrapper>(del));
     map.erase(0);
   }
 
-  EXPECT_TRUE(Wrapper::del);
+  EXPECT_TRUE(del);
+}
+
+TEST(ConcurrentHashMap, DeletionWithIterator) {
+  bool del{false};
+
+  {
+    ConcurrentHashMap<int, std::shared_ptr<Wrapper>> map;
+
+    map.insert(0, std::make_shared<Wrapper>(del));
+    auto it = map.find(0);
+    map.erase(it);
+  }
+
+  EXPECT_TRUE(del);
+}
+
+TEST(ConcurrentHashMap, DeletionWithForLoop) {
+  bool del{false};
+
+  {
+    ConcurrentHashMap<int, std::shared_ptr<Wrapper>> map;
+
+    map.insert(0, std::make_shared<Wrapper>(del));
+    for (auto it = map.cbegin(); it != map.cend(); ++it) {
+      EXPECT_EQ(it->first, 0);
+    }
+  }
+
+  EXPECT_TRUE(del);
+}
+
+TEST(ConcurrentHashMap, DeletionMultiple) {
+  bool del1{false}, del2{false};
+
+  {
+    ConcurrentHashMap<int, std::shared_ptr<Wrapper>> map;
+
+    map.insert(0, std::make_shared<Wrapper>(del1));
+    map.insert(1, std::make_shared<Wrapper>(del2));
+  }
+
+  EXPECT_TRUE(del1);
+  EXPECT_TRUE(del2);
+}
+
+TEST(ConcurrentHashMap, DeletionAssigned) {
+  bool del1{false}, del2{false};
+
+  {
+    ConcurrentHashMap<int, std::shared_ptr<Wrapper>> map;
+
+    map.insert(0, std::make_shared<Wrapper>(del1));
+    map.insert_or_assign(0, std::make_shared<Wrapper>(del2));
+  }
+
+  EXPECT_TRUE(del1);
+  EXPECT_TRUE(del2);
+}
+
+TEST(ConcurrentHashMap, DeletionMultipleMaps) {
+  bool del1{false}, del2{false};
+
+  {
+    ConcurrentHashMap<int, std::shared_ptr<Wrapper>> map1;
+    ConcurrentHashMap<int, std::shared_ptr<Wrapper>> map2;
+
+    map1.insert(0, std::make_shared<Wrapper>(del1));
+    map2.insert(0, std::make_shared<Wrapper>(del2));
+  }
+
+  EXPECT_TRUE(del1);
+  EXPECT_TRUE(del2);
 }

folly/experimental/hazptr/hazptr-impl.h

@@ -702,6 +702,7 @@ FOLLY_ALWAYS_INLINE hazptr_array<M>::~hazptr_array() {
       auto count = tc.count();
       if ((M <= HAZPTR_TC_SIZE) && (count + M <= HAZPTR_TC_SIZE)) {
         for (size_t i = 0; i < M; ++i) {
+          h[i].reset();
           tc[count + i].hprec_ = h[i].hazptr_;
           HAZPTR_DEBUG_PRINT(i << " " << &h[i]);
           new (&h[i]) hazptr_holder(nullptr);