concurrency issues solved

Author: vlntb

include/xrpl/ledger/LedgerIndexMap.h

@@ -2,7 +2,9 @@
 
 #include <algorithm>
 #include <mutex>
+#include <optional>
 #include <unordered_map>
+#include <utility>
 
 namespace xrpl {
 
@@ -23,45 +25,19 @@ class LedgerIndexMap
     LedgerIndexMap&
     operator=(LedgerIndexMap&&) = delete;
 
-    Mapped&
-    operator[](Key const& k)
-    {
-        std::lock_guard lock(mutex_);
-        return data_[k];
-    }
-
-    Mapped&
-    operator[](Key&& k)
-    {
-        std::lock_guard lock(mutex_);
-        return data_[std::move(k)];
-    }
-
-    [[nodiscard]] Mapped*
-    get(Key const& k)
-    {
-        std::lock_guard lock(mutex_);
-        auto it = data_.find(k);
-        return it == data_.end() ? nullptr : &it->second;
-    }
-
-    [[nodiscard]] Mapped const*
+    [[nodiscard]] std::optional<Mapped>
     get(Key const& k) const
     {
         std::lock_guard lock(mutex_);
         auto it = data_.find(k);
-        return it == data_.end() ? nullptr : &it->second;
+        return it == data_.end() ? std::nullopt : std::optional<Mapped>{it->second};
     }
 
-    template <class... Args>
-    Mapped&
-    put(Key const& k, Args&&... args)
+    bool
+    put(Key const& k, Mapped value)
     {
         std::lock_guard lock(mutex_);
-        auto [it, inserted] = data_.try_emplace(k, std::forward<Args>(args)...);
-        if (!inserted)
-            it->second = Mapped(std::forward<Args>(args)...);
-        return it->second;
+        return data_.insert_or_assign(k, std::move(value)).second;
     }
 
     bool
@@ -72,14 +48,14 @@ class LedgerIndexMap
     }
 
     std::size_t
-    size() const noexcept
+    size() const
     {
         std::lock_guard lock(mutex_);
         return data_.size();
     }
 
     bool
-    empty() const noexcept
+    empty() const
     {
         std::lock_guard lock(mutex_);
         return data_.empty();

src/tests/libxrpl/ledger/LedgerIndexMap.cpp

@@ -15,60 +15,64 @@ TEST(LedgerIndexMap, DefaultEmpty)
     TestMap m;
     EXPECT_EQ(m.size(), 0);
     EXPECT_TRUE(m.empty());
-    EXPECT_EQ(m.get(42), nullptr);
+    EXPECT_FALSE(m.get(42).has_value());
     EXPECT_FALSE(m.contains(42));
 }
 
-TEST(LedgerIndexMap, OperatorBracketInsertsDefault)
+TEST(LedgerIndexMap, PutInsertsValue)
 {
     TestMap m;
-    auto& v = m[10];
+    bool inserted = m.put(10, "");
+    EXPECT_TRUE(inserted);
     EXPECT_EQ(m.size(), 1);
     EXPECT_TRUE(m.contains(10));
-    EXPECT_TRUE(v.empty());
+    auto got = m.get(10);
+    ASSERT_TRUE(got.has_value());
+    EXPECT_TRUE(got->empty());
 }
 
-TEST(LedgerIndexMap, OperatorBracketRvalueKey)
+TEST(LedgerIndexMap, PutWithValue)
 {
     TestMap m;
-    int k = 7;
-    auto& v1 = m[std::move(k)];
-    v1 = "seven";
+    bool inserted = m.put(7, "seven");
+    EXPECT_TRUE(inserted);
     EXPECT_EQ(m.size(), 1);
-    auto* got = m.get(7);
-    ASSERT_NE(got, nullptr);
+    auto got = m.get(7);
+    ASSERT_TRUE(got.has_value());
     EXPECT_EQ(*got, "seven");
 }
 
 TEST(LedgerIndexMap, InsertThroughPut)
 {
     TestMap m;
-    auto& v = m.put(20, "twenty");
-    EXPECT_EQ(v, "twenty");
-    auto* got = m.get(20);
-    ASSERT_NE(got, nullptr);
+    bool inserted = m.put(20, "twenty");
+    EXPECT_TRUE(inserted);
+    auto got = m.get(20);
+    ASSERT_TRUE(got.has_value());
     EXPECT_EQ(*got, "twenty");
     EXPECT_EQ(m.size(), 1);
 }
 
 TEST(LedgerIndexMap, OverwriteExistingKeyWithPut)
 {
     TestMap m;
-    m.put(5, "five");
+    bool inserted1 = m.put(5, "five");
+    EXPECT_TRUE(inserted1);
     EXPECT_EQ(m.size(), 1);
-    m.put(5, "FIVE");
+    bool inserted2 = m.put(5, "FIVE");
+    EXPECT_FALSE(inserted2);  // Not a new insertion, it's an update
     EXPECT_EQ(m.size(), 1);
-    auto* got = m.get(5);
-    ASSERT_NE(got, nullptr);
+    auto got = m.get(5);
+    ASSERT_TRUE(got.has_value());
     EXPECT_EQ(*got, "FIVE");
 }
 
 TEST(LedgerIndexMap, OnceFoundOneNotFound)
 {
     TestMap m;
     m.put(1, "one");
-    EXPECT_NE(m.get(1), nullptr);
-    EXPECT_EQ(m.get(2), nullptr);
+    EXPECT_TRUE(m.get(1).has_value());
+    EXPECT_FALSE(m.get(2).has_value());
 }
 
 TEST(LedgerIndexMap, TryEraseBeforeNothingToDo)
@@ -174,8 +178,8 @@ TEST(LedgerIndexMap, RehashDoesNotLoseEntries)
 
     for (int k = 0; k < 16; ++k)
     {
-        auto* v = m.get(k);
-        ASSERT_NE(v, nullptr);
+        auto v = m.get(k);
+        ASSERT_TRUE(v.has_value());
         EXPECT_EQ(*v, "v" + std::to_string(k));
     }
 

src/xrpld/app/ledger/LedgerHistory.cpp

@@ -34,7 +34,7 @@ LedgerHistory::insert(std::shared_ptr<Ledger const> const& ledger, bool validate
 
     bool const alreadyHad = m_ledgers_by_hash.canonicalize_replace_cache(ledger->header().hash, ledger);
     if (validated)
-        mLedgersByIndex[ledger->header().seq] = ledger->header().hash;
+        mLedgersByIndex.put(ledger->header().seq, ledger->header().hash);
 
     return alreadyHad;
 }
@@ -67,7 +67,7 @@ LedgerHistory::getLedgerBySeq(LedgerIndex index)
         // Add this ledger to the local tracking by index
         XRPL_ASSERT(ret->isImmutable(), "xrpl::LedgerHistory::getLedgerBySeq : immutable result ledger");
         m_ledgers_by_hash.canonicalize_replace_client(ret->header().hash, ret);
-        mLedgersByIndex[ret->header().seq] = ret->header().hash;
+        mLedgersByIndex.put(ret->header().seq, ret->header().hash);
         return (ret->header().seq == index) ? ret : nullptr;
     }
 }