Add test + some comments addressed

Author: ognyanstoimenov

cpp/arcticdb/storage/failure_simulation.hpp

@@ -31,6 +31,7 @@ enum class FailureType : int {
 static const char* failure_names[] = {
         "WRITE",
         "READ",
+        "WRITE_LOCK",
         "ITERATE",
         "DELETE",
 };

cpp/arcticdb/util/storage_lock.hpp

@@ -159,7 +159,7 @@ class StorageLock {
 
     bool try_acquire_lock(const std::shared_ptr<Store>& store) {
         auto start = ClockType::coarse_nanos_since_epoch();
-        if(!ref_key_exists(store) || !ttl_not_expired(store)) {
+        if(!ttl_not_expired(store)) {
             ts_= create_ref_key(store);
             auto lock_sleep_ms = ConfigsMap::instance()->get_int("StorageLock.WaitMs", DEFAULT_WAIT_MS);
             ARCTICDB_DEBUG(log::lock(), "Waiting for {} ms, thread id: {}", lock_sleep_ms, std::this_thread::get_id());
@@ -208,12 +208,6 @@ class StorageLock {
         return get_ref_key(name_);
     }
 
-    bool ref_key_exists(const std::shared_ptr<Store>& store) {
-        auto exists = store->key_exists_sync(ref_key());
-        ARCTICDB_DEBUG(log::lock(), "Ref key exists: {}", exists ? "true" : "false");
-        return exists;
-    }
-
     static void do_remove_ref_key(const std::shared_ptr<Store>& store, const StreamId& name) {
         ARCTICDB_DEBUG(log::lock(), "Removing ref key");
         try {

cpp/arcticdb/util/test/test_storage_lock.cpp

@@ -48,23 +48,16 @@ TEST(StorageLock, Timeout) {
 }
 
 struct LockData {
-    std::string lock_name_;
-    std::shared_ptr<InMemoryStore> store_;
-    volatile uint64_t vol_;
-    std::atomic<uint64_t> atomic_;
-    std::atomic<bool> contended_;
+    std::string lock_name_ = "stress_test_lock";
+    std::shared_ptr<InMemoryStore> store_ = std::make_shared<InMemoryStore>();
+    volatile uint64_t vol_ { 0 };
+    std::atomic<uint64_t> atomic_ = { 0 };
+    std::atomic<bool> contended_ { false };
+    std::atomic<bool> timedout_ { false };
     const size_t num_tests_;
-    std::atomic<bool> timedout_;
-
-    LockData(size_t num_tests) :
-    lock_name_("stress_test_lock"),
-    store_(std::make_shared<InMemoryStore>()),
-    vol_(0),
-    atomic_(0),
-    contended_(false),
-    num_tests_(num_tests),
-    timedout_(false){
-    }
+
+    explicit LockData(size_t num_tests) :
+    num_tests_(num_tests){}
 
 };
 
@@ -77,6 +70,7 @@ struct OptimisticLockTask {
 
     folly::Future<folly::Unit> operator()() {
         StorageLock<> lock{data_->lock_name_};
+        using namespace std::chrono_literals;
 
         for (auto i = size_t(0); i < data_->num_tests_; ++i) {
             if (!lock.try_lock(data_->store_)) {
@@ -85,6 +79,7 @@ struct OptimisticLockTask {
             else {
                 // As of C++20, '++' expression of 'volatile'-qualified type is deprecated.
                 const uint64_t vol_ = data_->vol_ + 1;
+                std::this_thread::sleep_for(10ms);
                 data_->vol_ = vol_;
                 ++data_->atomic_;
                 lock.unlock(data_->store_);
@@ -370,7 +365,7 @@ class StorageLockWithSlowWrites : public ::testing::TestWithParam<std::tuple<int
 protected:
     void SetUp() override {
         log::lock().set_level(spdlog::level::debug);
-        StorageFailureSimulator::instance()->reset();
+        StorageFailureSimulator::reset();
     }
 };
 
@@ -426,6 +421,29 @@ TEST_P(StorageLockWithSlowWrites, ConcurrentWritesWithRetrying) {
     ASSERT_EQ(lock_data->atomic_, 1);
 }
 
+TEST(StorageLock, StressManyWriters) {
+
+    const StorageFailureSimulator::ParamActionSequence SLOW_ACTIONS = {
+        action_factories::slow_action(0.3, 600, 1100),
+    };
+
+    constexpr size_t num_writers = 50;
+    FutureExecutor<CPUThreadPoolExecutor> exec{num_writers};
+
+    StorageFailureSimulator::instance()->configure({{FailureType::WRITE_LOCK, SLOW_ACTIONS}});
+
+    std::vector<Future<Unit>> futures;
+    auto lock_data = std::make_shared<LockData>(num_writers);
+    lock_data->store_ = std::make_shared<InMemoryStore>();
+
+    for (size_t i = 0; i < num_writers; ++i) {
+        futures.emplace_back(exec.addFuture(OptimisticLockTask(lock_data)));
+    }
+    collect(futures).get();
+
+    ASSERT_EQ(lock_data->atomic_, lock_data->vol_);
+}
+
 INSTANTIATE_TEST_SUITE_P(
     DelayPairs,
     StorageLockWithSlowWrites,
@@ -450,9 +468,6 @@ TEST(StorageLock, SlowWrites) {
 
 TEST(StorageLock, DISABLED_LockSameTimestamp) { // Not yet implemented
     log::lock().set_level(spdlog::level::debug);
-    const StorageFailureSimulator::ParamActionSequence SLOW_ON_2ND_CALL = {
-        action_factories::slow_action(1, 10, 10), action_factories::slow_action(1, 10, 10)
-    };
     constexpr size_t num_writers = 2;
 
     using StorageLockType = StorageLock<util::ManualClock>;
@@ -462,7 +477,6 @@ TEST(StorageLock, DISABLED_LockSameTimestamp) { // Not yet implemented
     while (locks.size() < num_writers) { locks.emplace_back(std::make_unique<StorageLockType>("test")); };
     auto store = std::make_shared<InMemoryStore>();
     folly::FutureExecutor<folly::CPUThreadPoolExecutor> exec{num_writers};
-    StorageFailureSimulator::instance()->configure({{FailureType::WRITE_LOCK, SLOW_ON_2ND_CALL}});
     std::vector<Future<Unit>> futures;
     std::vector<bool> writer_has_lock;
     writer_has_lock.resize(num_writers);