From a807bde9b3f486c77996e717f5154f60f50fe95f Mon Sep 17 00:00:00 2001 From: Dave Watson Date: Tue, 19 Dec 2017 14:30:19 -0800 Subject: [PATCH] Fix erase in Iterate (2) Summary: Previously D6579707. Correctly advance to next item if we erase the current element. Corner cases were slightly off if we were at the end of a hash chain. Reviewed By: yfeldblum Differential Revision: D6603518 fbshipit-source-id: acb959e5bcd5da1c3df642b75985d464fdd3b23d --- folly/concurrency/ConcurrentHashMap.h | 1 - .../detail/ConcurrentHashMap-detail.h | 1 + .../test/ConcurrentHashMapTest.cpp | 19 +++++++++++++++++++ 3 files changed, 20 insertions(+), 1 deletion(-) diff --git a/folly/concurrency/ConcurrentHashMap.h b/folly/concurrency/ConcurrentHashMap.h index b4d2fc3a..9a332809 100644 --- a/folly/concurrency/ConcurrentHashMap.h +++ b/folly/concurrency/ConcurrentHashMap.h @@ -326,7 +326,6 @@ class ConcurrentHashMap { ConstIterator erase(ConstIterator& pos) { auto segment = pickSegment(pos->first); ConstIterator res(this, segment); - res.next(); ensureSegment(segment)->erase(res.it_, pos.it_); res.next(); // May point to segment end, and need to advance. return res; diff --git a/folly/concurrency/detail/ConcurrentHashMap-detail.h b/folly/concurrency/detail/ConcurrentHashMap-detail.h index 60688726..51c1375b 100644 --- a/folly/concurrency/detail/ConcurrentHashMap-detail.h +++ b/folly/concurrency/detail/ConcurrentHashMap-detail.h @@ -563,6 +563,7 @@ class FOLLY_ALIGNED(64) ConcurrentHashMapSegment { iter->hazptrs_[0].reset(buckets); iter->setNode( node->next_.load(std::memory_order_acquire), buckets, idx); + iter->next(); } size_--; break; diff --git a/folly/concurrency/test/ConcurrentHashMapTest.cpp b/folly/concurrency/test/ConcurrentHashMapTest.cpp index b1b18e42..9ac3e0a1 100644 --- a/folly/concurrency/test/ConcurrentHashMapTest.cpp +++ b/folly/concurrency/test/ConcurrentHashMapTest.cpp @@ -257,6 +257,25 @@ TEST(ConcurrentHashMap, EraseTest) { foomap.erase(f1); } +TEST(ConcurrentHashMap, EraseInIterateTest) { + ConcurrentHashMap foomap(3); + for (uint64_t k = 0; k < 10; ++k) { + foomap.insert(k, k); + } + EXPECT_EQ(10, foomap.size()); + for (auto it = foomap.cbegin(); it != foomap.cend();) { + if (it->second > 3) { + it = foomap.erase(it); + } else { + ++it; + } + } + EXPECT_EQ(4, foomap.size()); + for (auto it = foomap.cbegin(); it != foomap.cend(); ++it) { + EXPECT_GE(3, it->second); + } +} + // TODO: hazptrs must support DeterministicSchedule #define Atom std::atomic // DeterministicAtomic -- 2.34.1