kvs: changed approach to locking

docs/features/persistency/safety_analysis/dfa.rst

@@ -138,7 +138,7 @@ Unintended impacts to function due to various failures.
   * - UI_01_04
     - Deadlocks
     - no
-    - One mutex per instance (Rust additionally one for the instance pool in the builder); never more than one lock is held at a time. Rust waits for the lock; C++ does not wait and returns MutexLockFailed.
+    - One mutex per instance (Rust additionally one for the instance pool in the builder). Both implementations wait for the lock. A call holds at most one lock and does not call back into the application while holding it. Exception: the C++ move assignment takes the locks of both instances at once with ``std::scoped_lock``, which avoids deadlock.
   * - UI_01_05
     - Livelocks
     - no
@@ -185,10 +185,14 @@ DFA
    :mitigated_by: aou_req__persistency__error_handling
    :sufficient: yes
    :status: valid
-   :version: 2
+   :version: 3
 
-   Persistency runs in the calling context; blocking (by the application or OS scheduling) leads to no or a
-   too late response, handled by :need:`aou_req__persistency__error_handling`.
+   Persistency runs in the calling context. Blocking leads to no or a too late response, handled by
+   :need:`aou_req__persistency__error_handling`.
+
+   Causes: the application or OS scheduling, and waiting for the instance lock while another thread of the
+   process uses the same instance. The wait lasts until that call returns, including its file access (e.g.
+   flush or snapshot restore).
 
 .. feat_saf_dfa:: Corruption of persisted data
    :violates: feat_arc_sta__persistency__static

score/kvs/docs/safety_analysis/dfa.rst

@@ -144,11 +144,11 @@ Unintended impacts to function due to various failures.
   * - UI_01_05
     - Livelocks
     - no
-    - No retry loops; a failed lock is reported (C++ ``MutexLockFailed``).
+    - No retry loops. Calls wait for the instance lock and do not retry.
   * - UI_01_06
     - Blocking of execution
     - no
-    - No component specific effect, see :need:`feat_saf_dfa__persistency__execution_blocking`.
+    - Calls wait for the instance lock held by another thread of the process. The effect is covered by :need:`feat_saf_dfa__persistency__execution_blocking`.
   * - UI_01_07
     - Incorrect allocation of execution time
     - no

score/kvs/error.cpp

@@ -61,8 +61,6 @@ std::string_view KvsErrorDomain::MessageFor(const score::result::ErrorCode& code
             return "Invalid snapshot ID";
         case ErrorCode::kConversionFailed:
             return "Conversion failed";
-        case ErrorCode::kMutexLockFailed:
-            return "Mutex failed";
         case ErrorCode::kInvalidValueType:
             return "Invalid value type";
         case ErrorCode::kInvalidKey:

score/kvs/error.h

@@ -77,9 +77,6 @@ enum class ErrorCode : score::result::ErrorCode
     /// Conversion failed.
     kConversionFailed = 18,
 
-    /// Mutex failed.
-    kMutexLockFailed = 19,
-
     /// Invalid value type.
     kInvalidValueType = 20,
 
@@ -124,8 +121,6 @@ enum class ErrorCode : score::result::ErrorCode
         kInvalidSnapshotId,
     ConversionFailed [[deprecated("Use kConversionFailed instead. This name is removed in a future release.")]] =
         kConversionFailed,
-    MutexLockFailed [[deprecated("Use kMutexLockFailed instead. This name is removed in a future release.")]] =
-        kMutexLockFailed,
     InvalidValueType [[deprecated("Use kInvalidValueType instead. This name is removed in a future release.")]] =
         kInvalidValueType,
     InvalidKey [[deprecated("Use kInvalidKey instead. This name is removed in a future release.")]] = kInvalidKey,

score/kvs/kvs.cpp

@@ -329,25 +329,15 @@ score::Result<Kvs> Kvs::Open(const InstanceId& instance_id,
 
 score::Result<void> Kvs::Reset() noexcept
 {
-    std::unique_lock<std::mutex> lock{kvs_mutex_, std::try_to_lock};
-    if (!lock.owns_lock())
-    {
-        GetLogger().LogWarn() << "Failed to lock KVS mutex";
-        return score::MakeUnexpected(ErrorCode::kMutexLockFailed);
-    }
+    std::lock_guard<std::mutex> lock{kvs_mutex_};
 
     kvs_.clear();
     return {};
 }
 
 score::Result<std::vector<std::string>> Kvs::GetAllKeys() const noexcept
 {
-    std::unique_lock<std::mutex> lock{kvs_mutex_, std::try_to_lock};
-    if (!lock.owns_lock())
-    {
-        GetLogger().LogWarn() << "Failed to lock KVS mutex";
-        return score::MakeUnexpected(ErrorCode::kMutexLockFailed);
-    }
+    std::lock_guard<std::mutex> lock{kvs_mutex_};
 
     std::vector<std::string> keys;
     keys.reserve(kvs_.size());
@@ -360,12 +350,7 @@ score::Result<std::vector<std::string>> Kvs::GetAllKeys() const noexcept
 
 score::Result<bool> Kvs::KeyExists(const std::string_view key) const noexcept
 {
-    std::unique_lock<std::mutex> lock{kvs_mutex_, std::try_to_lock};
-    if (!lock.owns_lock())
-    {
-        GetLogger().LogWarn() << "Failed to lock KVS mutex";
-        return score::MakeUnexpected(ErrorCode::kMutexLockFailed);
-    }
+    std::lock_guard<std::mutex> lock{kvs_mutex_};
 
     // unordered_map find() needs string and doesn't work with string_view.
     // C++20 heterogeneous lookup would avoid the copy (applies to more functions).
@@ -375,12 +360,7 @@ score::Result<bool> Kvs::KeyExists(const std::string_view key) const noexcept
 
 score::Result<KvsValue> Kvs::GetValue(const std::string_view key) const noexcept
 {
-    std::unique_lock<std::mutex> lock_kvs{kvs_mutex_, std::try_to_lock};
-    if (!lock_kvs.owns_lock())
-    {
-        GetLogger().LogWarn() << "Failed to lock KVS mutex";
-        return score::MakeUnexpected(ErrorCode::kMutexLockFailed);
-    }
+    std::lock_guard<std::mutex> lock{kvs_mutex_};
 
     const auto search_kvs{kvs_.find(std::string(key))};
     if (search_kvs == kvs_.end())
@@ -419,25 +399,15 @@ score::Result<void> Kvs::SetValueToDefault(const std::string_view key) noexcept
     }
 
     // Set value.
-    std::unique_lock<std::mutex> lock{kvs_mutex_, std::try_to_lock};
-    if (!lock.owns_lock())
-    {
-        GetLogger().LogWarn() << "Failed to lock KVS mutex";
-        return score::MakeUnexpected(ErrorCode::kMutexLockFailed);
-    }
+    std::lock_guard<std::mutex> lock{kvs_mutex_};
 
     score::cpp::ignore = kvs_.insert_or_assign(std::string(key), std::move(*default_value_result));
     return {};
 }
 
 score::Result<bool> Kvs::IsValueDefault(const std::string_view key) const noexcept
 {
-    std::unique_lock<std::mutex> lock(kvs_mutex_, std::try_to_lock);
-    if (!lock.owns_lock())
-    {
-        GetLogger().LogWarn() << "Failed to lock KVS mutex";
-        return score::MakeUnexpected(ErrorCode::kMutexLockFailed);
-    }
+    std::lock_guard<std::mutex> lock(kvs_mutex_);
 
     const std::string key_str{key};
     if (kvs_.find(key_str) != kvs_.end())
@@ -473,25 +443,15 @@ score::Result<void> Kvs::SetValue(const std::string_view key, const KvsValue& va
         return score::MakeUnexpected(ErrorCode::kInvalidValueType);
     }
 
-    std::unique_lock<std::mutex> lock{kvs_mutex_, std::try_to_lock};
-    if (!lock.owns_lock())
-    {
-        GetLogger().LogWarn() << "Failed to lock KVS mutex";
-        return score::MakeUnexpected(ErrorCode::kMutexLockFailed);
-    }
+    std::lock_guard<std::mutex> lock{kvs_mutex_};
 
     score::cpp::ignore = kvs_.insert_or_assign(std::string(key), value);
     return {};
 }
 
 score::Result<void> Kvs::RemoveKey(const std::string_view key) noexcept
 {
-    std::unique_lock<std::mutex> lock{kvs_mutex_, std::try_to_lock};
-    if (!lock.owns_lock())
-    {
-        GetLogger().LogWarn() << "Failed to lock KVS mutex";
-        return score::MakeUnexpected(ErrorCode::kMutexLockFailed);
-    }
+    std::lock_guard<std::mutex> lock{kvs_mutex_};
 
     if (kvs_.erase(std::string(key)) == 0U)
     {
@@ -503,25 +463,15 @@ score::Result<void> Kvs::RemoveKey(const std::string_view key) noexcept
 
 score::Result<void> Kvs::RemoveAllKeys() noexcept
 {
-    std::unique_lock<std::mutex> lock{kvs_mutex_, std::try_to_lock};
-    if (!lock.owns_lock())
-    {
-        GetLogger().LogWarn() << "Failed to lock KVS mutex";
-        return score::MakeUnexpected(ErrorCode::kMutexLockFailed);
-    }
+    std::lock_guard<std::mutex> lock{kvs_mutex_};
 
     kvs_.clear();
     return {};
 }
 
 score::Result<void> Kvs::DiscardPendingChanges() noexcept
 {
-    std::unique_lock<std::mutex> lock{kvs_mutex_, std::try_to_lock};
-    if (!lock.owns_lock())
-    {
-        GetLogger().LogWarn() << "Failed to lock KVS mutex";
-        return score::MakeUnexpected(ErrorCode::kMutexLockFailed);
-    }
+    std::lock_guard<std::mutex> lock{kvs_mutex_};
 
     // Snapshot 0 is the current persisted state: written by Flush(), read by Open().
     const score::filesystem::Path kvs_path{filename_prefix_.Native() + "_0"};
@@ -539,6 +489,8 @@ score::Result<void> Kvs::DiscardPendingChanges() noexcept
 
 score::Result<void> Kvs::Flush() noexcept
 {
+    std::lock_guard<std::mutex> lock{kvs_mutex_};
+
     auto result{SerializeAndCheck()};
     if (!result)
     {
@@ -585,12 +537,7 @@ size_t Kvs::SnapshotMaxCount() const noexcept
 
 score::Result<void> Kvs::SnapshotRestore(const SnapshotId& snapshot_id) noexcept
 {
-    std::unique_lock<std::mutex> lock{kvs_mutex_, std::try_to_lock};
-    if (!lock.owns_lock())
-    {
-        GetLogger().LogWarn() << "Failed to lock KVS mutex";
-        return score::MakeUnexpected(ErrorCode::kMutexLockFailed);
-    }
+    std::lock_guard<std::mutex> lock{kvs_mutex_};
 
     const auto snapshot_count_result{SnapshotCount()};
     if (!snapshot_count_result)
@@ -687,6 +634,8 @@ score::Result<size_t> Kvs::GetStorageFileSize() const noexcept
 
 score::Result<size_t> Kvs::CalculatePotentialSize() const noexcept
 {
+    std::lock_guard<std::mutex> lock{kvs_mutex_};
+
     auto result{SerializeAndCheck()};
     if (!result)
     {
@@ -715,13 +664,6 @@ Kvs::Kvs() noexcept
 
 score::Result<void> Kvs::SnapshotRotate() const noexcept
 {
-    std::unique_lock<std::mutex> lock{kvs_mutex_, std::try_to_lock};
-    if (!lock.owns_lock())
-    {
-        GetLogger().LogWarn() << "Failed to lock KVS mutex";
-        return score::MakeUnexpected(ErrorCode::kMutexLockFailed);
-    }
-
     for (size_t index{max_snapshots_}; index > 0; --index)
     {
         const score::filesystem::Path hash_old{filename_prefix_.Native() + "_" + std::to_string(index - 1) +
@@ -913,25 +855,14 @@ score::Result<void> Kvs::WriteJsonData(const std::string& buffer) const noexcept
 
 score::Result<std::string> Kvs::SerializeAndCheck() const noexcept
 {
-    score::json::Any root_obj;
-
     // 1. Serialize the current KVS map.
+    auto converted{KvsValueToAny(kvs_)};
+    if (!converted)
     {
-        std::unique_lock<std::mutex> lock{kvs_mutex_, std::try_to_lock};
-        if (!lock.owns_lock())
-        {
-            GetLogger().LogWarn() << "Failed to lock KVS mutex";
-            return score::MakeUnexpected(ErrorCode::kMutexLockFailed);
-        }
-
-        auto converted{KvsValueToAny(kvs_)};
-        if (!converted)
-        {
-            GetLogger().LogError() << "Failed to serialize KVS data";
-            return score::MakeUnexpected<std::string>(converted.error());
-        }
-        root_obj = std::move(*converted);
+        GetLogger().LogError() << "Failed to serialize KVS data";
+        return score::MakeUnexpected<std::string>(converted.error());
     }
+    const score::json::Any root_obj{std::move(*converted)};
 
     auto buffer_result{writer_->ToBuffer(root_obj)};
     if (!buffer_result)

score/kvs/kvs_deprecated_api_test.cpp

@@ -49,7 +49,6 @@ static_assert(static_cast<score::result::ErrorCode>(ErrorCode::kKeyNotFound) ==
 static_assert(static_cast<score::result::ErrorCode>(ErrorCode::kSerializationFailed) == 16, "");
 static_assert(static_cast<score::result::ErrorCode>(ErrorCode::kInvalidSnapshotId) == 17, "");
 static_assert(static_cast<score::result::ErrorCode>(ErrorCode::kConversionFailed) == 18, "");
-static_assert(static_cast<score::result::ErrorCode>(ErrorCode::kMutexLockFailed) == 19, "");
 static_assert(static_cast<score::result::ErrorCode>(ErrorCode::kInvalidValueType) == 20, "");
 static_assert(static_cast<score::result::ErrorCode>(ErrorCode::kInvalidKey) == 21, "");
 
@@ -99,7 +98,6 @@ static_assert(ErrorCode::KeyNotFound == ErrorCode::kKeyNotFound, "");
 static_assert(ErrorCode::SerializationFailed == ErrorCode::kSerializationFailed, "");
 static_assert(ErrorCode::InvalidSnapshotId == ErrorCode::kInvalidSnapshotId, "");
 static_assert(ErrorCode::ConversionFailed == ErrorCode::kConversionFailed, "");
-static_assert(ErrorCode::MutexLockFailed == ErrorCode::kMutexLockFailed, "");
 static_assert(ErrorCode::InvalidValueType == ErrorCode::kInvalidValueType, "");
 static_assert(ErrorCode::InvalidKey == ErrorCode::kInvalidKey, "");
 DISABLE_WARNING_POP

score/kvs/kvs_error_test.cpp

@@ -43,7 +43,6 @@ TEST(kvs_MessageFor, MessageFor)
         {ErrorCode::kSerializationFailed, "Serialization failed"},
         {ErrorCode::kInvalidSnapshotId, "Invalid snapshot ID"},
         {ErrorCode::kConversionFailed, "Conversion failed"},
-        {ErrorCode::kMutexLockFailed, "Mutex failed"},
         {ErrorCode::kInvalidValueType, "Invalid value type"},
         {ErrorCode::kInvalidKey, "Key is not valid"},
     };