race condition in AtomicHashMap

[storypku]

Describe the bug Impl of AtomicHashMap isn't thread-safe.

To Reproduce Steps to reproduce the behavior: Given the following example:

#include "cyber/base/atomic_hash_map.h"
#include <chrono>
#include <string>
#include <thread>

using namespace apollo::cyber::base;

int main(int argc, char *argv[]) {
    AtomicHashMap<int, std::string, 128> amap;
    amap.Set(5, "old");

    std::thread a([&]() {
        std::string *value = nullptr;
        if (amap.Get(5, &value)) {
            std::this_thread::sleep_for(std::chrono::seconds(1));
            printf("Got: %s\n", value->c_str());
        }
    });

    std::this_thread::sleep_for(std::chrono::milliseconds(500));
    amap.Set(5, "new");
    a.join();

    return 0;
}

Output: Got: (null)

Additional context Although we can hardly observe above senerio using AtomicHashMap::Get(K, V*) rather than Get(K, V**), the bug is intrinsic to AtomicHashMap implementation.

[fengqikai1414]

@storypku Yes, you are right. We provide two Get funcions, one for faster access and the other for thread safe. You can choose a more appropriate interface based on your own usage scenario. I'm sorry to confuse you, we'll add more detailed comments later.

[storypku]

@fengqikai1414 In fact, neither of the two Get functions is thread-safe: *v = *target->value_ptr.load(std::memory_order_acquire); is not an atomic operation.

[fengqikai1414]

@storypku Thank you for your reminder. We'll see how to fix it.

[fengqikai1414]

@storypku After careful analysis, the scenario used in cyber RT does not trigger this problem, we will completely solve this problem after upgrading to C++17.

[storypku]

@fengqikai1414 Any ideas on how to resolve this as we are targeting C++17 now?