ConcurrentHashMap::try_emplace returns an iterator which does not keep the corresponding value alive

[benhannel]

The documentation suggests that the iterators contain hazard points to the elements, and the elements will remain accessible while the iterator is alive, even if it is erased from the map: https://github.com/facebook/folly/blob/main/folly/concurrency/ConcurrentHashMap.h#L44

Here is a very simple repro of the bug, in which an element is destroyed while an iterator to it is still held:

    static std::atomic<bool> hasBeenDestroyed = false;
    struct Value {
        Value(bool logDestruction) : logDestruction_(logDestruction) {}
        ~Value() {
            if (logDestruction_) {
                hasBeenDestroyed = true;
            }
        }
        bool logDestruction_;
    };
    folly::ConcurrentHashMapSIMD<uint64_t, unique_ptr<Value>> map;
    auto [it, inserted] = map.try_emplace(1, make_unique<Value>(true));
    std::thread t([&]() {
        auto it1 = map.find(1);
        map.erase(it1);
        // Add enough entries to the cache to trigger a GC of the erased
        // iterators.
        for (int i = 2; i < 100000; ++i) {
            map.try_emplace(i, make_unique<Value>(false));
        }
    });
    t.join();
    std::cout << "Has element 1 been destroyed? " << hasBeenDestroyed << std::endl;

Logs:

Has element 1 been destroyed? 1

[benhannel]

cc @magedm in case this is related to your work on hazptr

[instw]

I was curious about the issue, and tried to repro but couldn't. Added this to ConcurrentHashMapTest.cpp-

TEST(TempTest, Temp) {
  static std::atomic<bool> hasBeenDestroyed = false;
    struct Value {
        Value(bool logDestruction) : logDestruction_(logDestruction) {}
        ~Value() {
            if (logDestruction_) {
                hasBeenDestroyed = true;
            }
        }
        bool logDestruction_;
    };
    folly::ConcurrentHashMapSIMD<uint64_t, unique_ptr<Value>> map;
    auto [it, inserted] = map.try_emplace(1, make_unique<Value>(true));
    std::thread t([&]() {
        auto it1 = map.find(1);
        map.erase(it1);
        // Add enough entries to the cache to trigger a GC of the erased
        // iterators.
        for (int i = 2; i < 100000; ++i) {
            map.try_emplace(i, make_unique<Value>(false));
        }
    });
    join;
    std::cout << "Has element 1 been destroyed? " << hasBeenDestroyed << std::endl;
}

Running it-

buck2 run @mode/opt folly/concurrency/test:concurrent_hash_map_test -- --gtest_filter=Temp* --folly_hazptr_use_executor=false
...
[ RUN      ] TempTest.Temp
Has element 1 been destroyed? 0
[       OK ] TempTest.Temp (535 ms)
[----------] 1 test from TempTest (535 ms total)

I wasn't able to reproduce the issue in either debug or opt modes