iox-eclipse-iceoryx#1196 Introduce make_consistent to make the mutex …

…fully usable for robust mutex, address review findings

Signed-off-by: Christian Eltzschig <[email protected]>

iceoryx_hoofs/include/iceoryx_hoofs/internal/posix_wrapper/mutex.hpp

@@ -21,6 +21,7 @@
 #include "iceoryx_hoofs/cxx/optional.hpp"
 #include "iceoryx_hoofs/design_pattern/builder.hpp"
 #include "iceoryx_hoofs/platform/pthread.hpp"
+#include <pthread.h>
 
 namespace iox
 {
@@ -39,6 +40,7 @@ enum class MutexError
     DEADLOCK_CONDITION,
     NOT_OWNED_BY_THREAD,
     INVALID_PRIORITY_CEILING_VALUE,
+    HAS_INCONSISTENT_STATE_SINCE_OWNER_DIED,
     UNDEFINED
 };
 
@@ -80,8 +82,9 @@ class Mutex
     /// @todo iox-#1036 remove this, introduced to keep current API temporarily
     explicit Mutex(const bool f_isRecursive) noexcept;
 
-    /// @brief Destroyes the mutex. When the is still locked this will fail and the
-    ///        mutex is leaked!
+    /// @brief Destroys the mutex. When the mutex is still locked this will fail and the
+    ///        mutex is leaked! If the MutexThreadTerminationBehavior is set to RELEASE_WHEN_LOCKED
+    ///        a locked mutex is unlocked and the handle is cleaned up correctly.
     ~Mutex() noexcept;
 
     /// @brief all copy and move assignment methods need to be deleted otherwise
@@ -99,7 +102,7 @@ class Mutex
     ///          * will be non blocking when the lock call comes from the same thread and the
     ///              mutex type is MutexType::RECURSIVE
     ///          * will be blocking when for all non MutexType::RECURSIVE
-    ///          * will be a MutexError::DEADLOCK_CONDITION when it is a
+    ///          * will return a MutexError::DEADLOCK_CONDITION when it is a
     ///              MutexType::WITH_DEADLOCK_DETECTION
 
     cxx::expected<MutexError> lock() noexcept;
@@ -119,15 +122,22 @@ class Mutex
     ///         On failure it returns MutexError describing the failure.
     cxx::expected<MutexTryLock, MutexError> try_lock() noexcept;
 
+    /// @brief When a mutex owning thread/process with MutexThreadTerminationBehavior::RELEASE_WHEN_LOCKED dies then the
+    ///        next instance which would like to acquire the lock will get an
+    ///        MutexError::HAS_INCONSISTENT_STATE_SINCE_OWNER_DIED error. This method puts the mutex again into a
+    ///        consistent state. If the mutex is already in a consistent state it will do nothing.
+    void make_consistent() noexcept;
+
   private:
     Mutex() noexcept = default;
 
   private:
     friend class MutexBuilder;
     friend class cxx::optional<Mutex>;
 
-    pthread_mutex_t m_handle;
+    pthread_mutex_t m_handle = PTHREAD_MUTEX_INITIALIZER;
     bool m_isDescructable = true;
+    bool m_hasInconsistentState = false;
 };
 
 /// @todo iox-#1036 remove this, introduced to keep current API temporarily
@@ -151,7 +161,8 @@ enum class MutexType : int32_t
     WITH_DEADLOCK_DETECTION = PTHREAD_MUTEX_ERRORCHECK,
 };
 
-/// @brief Describes the priority setting of the mutex.
+/// @brief Describes how the priority of a mutex owning thread changes when another thread
+///        with an higher priority would like to acquire the mutex.
 enum class MutexPriorityInheritance : int32_t
 {
     /// @brief No priority setting.
@@ -170,11 +181,14 @@ enum class MutexPriorityInheritance : int32_t
 /// @brief Defines the behavior when a mutex owning thread is terminated
 enum class MutexThreadTerminationBehavior : int32_t
 {
-    /// @brief The mutex stays locked is unlockable and no longer usable.
+    /// @brief The mutex stays locked, is unlockable and no longer usable.
     ///        This can also lead to a mutex leak in the destructor.
     STALL_WHEN_LOCKED = PTHREAD_MUTEX_STALLED,
 
-    /// @brief The mutex is unlocked when the thread is terminated.
+    /// @brief It implies the same behavior as MutexType::WITH_DEADLOCK_DETECTION. Additionally, when a mutex owning
+    ///        thread/process dies the mutex is put into an inconsistent state which can be recovered with
+    ///        Mutex::make_consistent(). The inconsistent state is detected by the next instance which calls
+    ///        Mutex::lock() or Mutex::try_lock() by the error value MutexError::HAS_INCONSISTENT_STATE_SINCE_OWNER_DIED
     RELEASE_WHEN_LOCKED = PTHREAD_MUTEX_ROBUST,
 };
 

iceoryx_hoofs/source/posix_wrapper/mutex.cpp

@@ -1,5 +1,5 @@
 // Copyright (c) 2019 by Robert Bosch GmbH. All rights reserved.
-// Copyright (c) 2021 by Apex.AI Inc. All rights reserved.
+// Copyright (c) 2021 - 2022 by Apex.AI Inc. All rights reserved.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -19,7 +19,7 @@
 #include "iceoryx_hoofs/cxx/generic_raii.hpp"
 #include "iceoryx_hoofs/internal/log/hoofs_logging.hpp"
 #include "iceoryx_hoofs/posix_wrapper/posix_call.hpp"
-#include "iceoryx_platform/platform_correction.hpp"
+#include "iceoryx_hoofs/posix_wrapper/scheduler.hpp"
 
 #include "iceoryx_hoofs/platform/platform_correction.hpp"
 namespace iox
@@ -217,7 +217,7 @@ Mutex::~Mutex() noexcept
             switch (destroyCall.get_error().errnum)
             {
             case EBUSY:
-                LogError() << "Tried to remove a locked mutex which failed. The mutex resource is now leaked and "
+                LogError() << "Tried to remove a locked mutex which failed. The mutex handle is now leaked and "
                               "cannot be removed anymore!";
                 break;
             default:
@@ -228,6 +228,16 @@ Mutex::~Mutex() noexcept
     }
 }
 
+void Mutex::make_consistent() noexcept
+{
+    if (this->m_hasInconsistentState)
+    {
+        posixCall(pthread_mutex_consistent)(&m_handle).returnValueMatchesErrno().evaluate().or_else(
+            [](auto) { LogError() << "This should never happen. Unable to put robust mutex in a consistent state!"; });
+        this->m_hasInconsistentState = false;
+    }
+}
+
 cxx::expected<MutexError> Mutex::lock() noexcept
 {
     auto result =
@@ -246,6 +256,11 @@ cxx::expected<MutexError> Mutex::lock() noexcept
         case EDEADLK:
             LogError() << "Deadlock in mutex detected.";
             return cxx::error<MutexError>(MutexError::DEADLOCK_CONDITION);
+        case EOWNERDEAD:
+            LogError() << "The thread/process which owned the mutex died. The mutex is now in an inconsistent state "
+                          "and must be put into a consistent state again with Mutex::make_consistent()";
+            this->m_hasInconsistentState = true;
+            return cxx::error<MutexError>(MutexError::HAS_INCONSISTENT_STATE_SINCE_OWNER_DIED);
         default:
             LogError() << "This should never happen. An unknown error occurred while locking the mutex. "
                           "This can indicate a either corrupted or non-posix compliant system.";
@@ -291,6 +306,11 @@ cxx::expected<MutexTryLock, MutexError> Mutex::try_lock() noexcept
             LogError() << "The mutex has the attribute MutexPriorityInheritance::PROTECT set and the calling threads "
                           "priority is greater than the mutex priority.";
             return cxx::error<MutexError>(MutexError::PRIORITY_MISMATCH);
+        case EOWNERDEAD:
+            LogError() << "The thread/process which owned the mutex died. The mutex is now in an inconsistent state "
+                          "and must be put into a consistent state again with Mutex::make_consistent()";
+            this->m_hasInconsistentState = true;
+            return cxx::error<MutexError>(MutexError::HAS_INCONSISTENT_STATE_SINCE_OWNER_DIED);
         default:
             LogError() << "This should never happen. An unknown error occurred while try locking the mutex. "
                           "This can indicate a either corrupted or non-posix compliant system.";

iceoryx_hoofs/source/posix_wrapper/scheduler.cpp

@@ -31,13 +31,15 @@ int32_t getSchedulerPriorityMinimum(const Scheduler scheduler) noexcept
         LogError() << "The \"sched_get_priority_min\" should never fail. This can only be caused by an internal logic "
                       "error or a non posix compliant system.";
 
+        // NOLINTJUSTIFICATION Required to provide an error message to the user
+        // NOLINTNEXTLINE(hicpp-no-array-decay,cppcoreguidelines-pro-bounds-array-to-pointer-decay)
         iox::cxx::Ensures(
             false
             && "This should never happen! Either the system is not posix compliant or and invalid integer was "
                "casted to the enum class Scheduler.");
         return -1;
     }
-    return static_cast<int32_t>(result.value().value);
+    return result.value().value;
 }
 
 int32_t getSchedulerPriorityMaximum(const Scheduler scheduler) noexcept
@@ -48,13 +50,15 @@ int32_t getSchedulerPriorityMaximum(const Scheduler scheduler) noexcept
         LogError() << "The \"sched_get_priority_max\" should never fail. This can only be caused by an internal logic "
                       "error or a non posix compliant system.";
 
+        // NOLINTJUSTIFICATION Required to provide an error message to the user
+        // NOLINTNEXTLINE(hicpp-no-array-decay,cppcoreguidelines-pro-bounds-array-to-pointer-decay)
         iox::cxx::Ensures(
             false
             && "This should never happen! Either the system is not posix compliant or and invalid integer was "
                "casted to the enum class Scheduler.");
         return -1;
     }
-    return static_cast<int32_t>(result.value().value);
+    return result.value().value;
 }
 } // namespace posix
 } // namespace iox