iox-#1640 Remove mutex

Signed-off-by: Matthias Killat <[email protected]>

iceoryx_hoofs/include/iceoryx_hoofs/design_pattern/polymorphic_handler.hpp

@@ -19,7 +19,6 @@
 
 #include <atomic>
 #include <iostream>
-#include <mutex>
 #include <type_traits>
 
 #include "iceoryx_hoofs/design_pattern/static_lifetime_guard.hpp"
@@ -93,6 +92,9 @@ class PolymorphicHandler
     // but they need to behave correctly and inheritance enforces this
     static_assert(std::is_base_of<Activatable, Interface>::value, "Interface must inherit from Activatable");
 
+    using Self = PolymorphicHandler<Interface, Default, Hooks>;
+    friend class StaticLifetimeGuard<Self>;
+
   public:
     /// @brief get the current singleton instance
     /// @return the current instance
@@ -109,16 +111,20 @@ class PolymorphicHandler
 
     /// @brief finalizes the instance, afterwards Hooks::onSetAfterFinalize
     ///        will be called during the remaining program lifetime
+    ///        when attempting to set or reset the handler
     static void finalize() noexcept;
 
-    static auto guard()
-    {
-        return StaticLifetimeGuard<Default>();
-    }
+    /// @brief returns a lifetime guard whose existence guarantees
+    /// the created PolymorphicHandler singleton instance will exist at least as long as the guard.
+    /// @return opaque lifetime guard object for the (implicit) PolymorphicHandler instance
+    /// @note the PolymorphicHandler will exist once any of the static methods (get, set etc.)
+    ///  are called
+    static auto guard() noexcept;
 
   private:
+    // should a defaultHandler be created, this delays its destruction
+    StaticLifetimeGuard<Default> m_defaultGuard;
     std::atomic_bool m_isFinal{false};
-    std::mutex m_mutex;
     std::atomic<Interface*> m_current;
 
     PolymorphicHandler() noexcept;

iceoryx_hoofs/include/iceoryx_hoofs/design_pattern/static_lifetime_guard.hpp

@@ -17,6 +17,7 @@
 #pragma once
 
 #include <atomic>
+#include <iostream>
 #include <type_traits>
 
 namespace iox
@@ -34,9 +35,7 @@ namespace design_pattern
 /// of the instance at least until G is destroyed
 /// @tparam T the type of the instance to be guarded
 ///
-/// @note dtor, ctor and copy ctor are thread-safe but instance()
-/// intentionally is not as it is supposed to be called in
-/// the static initialization phase or in a thread-safe context (e.g. with a mutex).
+/// @note dtor, ctor and copy ctor and instance are thread-safe
 ///
 /// @code
 /// // instance will be destroyed after guard
@@ -51,6 +50,7 @@ class StaticLifetimeGuard
   public:
     StaticLifetimeGuard() noexcept
     {
+        std::cerr << "GUARD " << typeid(T).hash_code() << std::endl;
         ++s_count;
     }
 
@@ -59,17 +59,17 @@ class StaticLifetimeGuard
         ++s_count;
     }
 
-    // move and assignment have no purpose as the objects themselves
-    // have no state and no side effects are required
-    // (copy exists to support passing/returning a value object)
+    // move and assignment have no purpose,
+    // copy exists to support passing/returning a value object
     StaticLifetimeGuard(StaticLifetimeGuard&&) = delete;
     StaticLifetimeGuard& operator=(const StaticLifetimeGuard&) = delete;
     StaticLifetimeGuard& operator=(StaticLifetimeGuard&&) = delete;
 
     ~StaticLifetimeGuard() noexcept
     {
-        if (--s_count == 0)
+        if (s_count.fetch_sub(1) == 1)
         {
+            std::cerr << "UNGUARD " << typeid(T).hash_code() << std::endl;
             destroy();
         }
     }
@@ -80,23 +80,39 @@ class StaticLifetimeGuard
     /// if it already exists
     /// @note creates an implicit static StaticLifetimeGuard to ensure
     /// the instance is destroyed in the static destruction phase
-    /// @note NOT thread-safe on its own on purpose as the first call should be used
-    /// for a static or in a context that handles thread-safety on its own
     template <typename... Args>
     static T& instance(Args&&... args) noexcept
     {
         static StaticLifetimeGuard guard;
-        if (s_count == 1)
+
+        // we determine wether this call has to initialize the instance
+        // via CAS (without mutex!)
+        // NB: this shows how CAS acts as consensus primitive
+        // will only be initialized once
+        uint32_t exp{UNINITIALIZED};
+        if (s_instanceState.compare_exchange_strong(
+                exp, INITALIZING, std::memory_order_acq_rel, std::memory_order_acquire))
         {
+            std::cerr << "CREATE INSTANCE " << typeid(T).hash_code() << std::endl;
             // NOLINTJUSTIFICATION s_instance is managed by reference counting
-            // NOLINTNEXTLINE (cppcoreguidelines-owning-memory)
+            // NOLINTNEXTLINE(cppcoreguidelines-owning-memory)
             s_instance = new (&s_storage) T(std::forward<Args>(args)...);
 
             // synchronize s_instance
-            // (store only is bad as concurrent construction of guards is allowed)
-            ++s_count;
-            --s_count;
+            s_instanceState.store(INITALIZED, std::memory_order_release);
+            return *s_instance;
+        }
+        // design constraint: no mutex - makes it more intricate
+        // (otherwise we could simply use double checked locking)
+
+        // either this call initialized the instance and we already returned
+        // or a concurrent call is doing so and we have to wait until it completes ...
+
+        while (s_instanceState.load(std::memory_order_acquire) != INITALIZED)
+        {
+            // wait
         }
+        // guaranteed to be non-null after initialization
         return *s_instance;
     }
 
@@ -121,21 +137,26 @@ class StaticLifetimeGuard
   private:
     using storage_t = typename std::aligned_storage_t<sizeof(T), alignof(T)>;
 
+    static constexpr uint32_t UNINITIALIZED{0};
+    static constexpr uint32_t INITALIZING{1};
+    static constexpr uint32_t INITALIZED{2};
+
     // NOLINTJUSTIFICATION static variables are private and mutability is required
     // NOLINTBEGIN (cppcoreguidelines-avoid-non-const-global-variables)
-
     static storage_t s_storage;
     static std::atomic<uint64_t> s_count;
-    // actually the pointer would not be needed (we can use the counter as indicator),
-    // but simpler and more clear this way
+    static std::atomic<uint32_t> s_instanceState;
     static T* s_instance;
 
+    std::atomic<uint64_t> m_value{1};
+
     static void destroy()
     {
         if (s_instance)
         {
             s_instance->~T();
             s_instance = nullptr;
+            s_instanceState = 0;
         }
     }
 };
@@ -145,6 +166,8 @@ typename StaticLifetimeGuard<T>::storage_t StaticLifetimeGuard<T>::s_storage;
 template <typename T>
 std::atomic<uint64_t> StaticLifetimeGuard<T>::s_count{0};
 template <typename T>
+std::atomic<uint32_t> StaticLifetimeGuard<T>::s_instanceState{UNINITIALIZED};
+template <typename T>
 T* StaticLifetimeGuard<T>::s_instance{nullptr};
 
 // NOLINTEND (cppcoreguidelines-avoid-non-const-global-variables)

iceoryx_hoofs/include/iceoryx_hoofs/internal/design_pattern/polymorphic_handler.inl

@@ -19,6 +19,7 @@
 
 #include "iceoryx_hoofs/design_pattern/polymorphic_handler.hpp"
 #include "iceoryx_hoofs/design_pattern/static_lifetime_guard.hpp"
+#include <atomic>
 #include <type_traits>
 
 namespace iox
@@ -86,30 +87,29 @@ Interface& PolymorphicHandler<Interface, Default, Hooks>::get() noexcept
 
     if (!localHandler->isActive())
     {
-        std::lock_guard<std::mutex> lock(instance().m_mutex);
         localHandler = getCurrent();
     }
     return *localHandler;
 }
 
-// can throw
 template <typename Interface, typename Default, typename Hooks>
 Interface* PolymorphicHandler<Interface, Default, Hooks>::set(Interface& handler) noexcept
 {
     auto& ins = instance();
+    // m_current is now guaranteed to be set
 
-    // setting is rare and we lock to synchronize and update the active flags properly
-    std::lock_guard<std::mutex> lock(ins.m_mutex);
-    if (ins.m_isFinal)
+    if (ins.m_isFinal.load())
     {
+        // it must be ensured that the handlers still exist and are thread-safe,
+        // this is ensured for the default handler
         Hooks::onSetAfterFinalize(*getCurrent(), handler);
         return nullptr;
     }
 
     handler.activate(); // it may have been deactivated before, so always reactivate it
-    auto prev = ins.m_current.exchange(&handler, std::memory_order_relaxed);
+    auto prev = ins.m_current.exchange(&handler, std::memory_order_acq_rel);
 
-    // anyone still using it will eventually see that it is inactive
+    // anyone still using the old handler will eventually see that it is inactive
     // and switch to the new handler
     prev->deactivate();
     return prev;
@@ -125,23 +125,19 @@ template <typename Interface, typename Default, typename Hooks>
 void PolymorphicHandler<Interface, Default, Hooks>::finalize() noexcept
 {
     auto& ins = instance();
-    std::lock_guard<std::mutex> lock(ins.m_mutex);
     ins.m_isFinal.store(true);
 }
 
 template <typename Interface, typename Default, typename Hooks>
 PolymorphicHandler<Interface, Default, Hooks>::PolymorphicHandler() noexcept
 {
-    std::lock_guard<std::mutex> lock(m_mutex);
-    //  this is set the first time we call instance()
     m_current.store(&getDefault(), std::memory_order_release);
 }
 
 template <typename Interface, typename Default, typename Hooks>
 PolymorphicHandler<Interface, Default, Hooks>& PolymorphicHandler<Interface, Default, Hooks>::instance() noexcept
 {
-    static PolymorphicHandler handler;
-    return handler;
+    return StaticLifetimeGuard<Self>::instance();
 }
 
 template <typename Interface, typename Default, typename Hooks>
@@ -158,6 +154,12 @@ Interface* PolymorphicHandler<Interface, Default, Hooks>::getCurrent() noexcept
         std::memory_order_acquire); // must be strong enough to sync memory of the object pointed to
 }
 
+template <typename Interface, typename Default, typename Hooks>
+auto PolymorphicHandler<Interface, Default, Hooks>::guard() noexcept
+{
+    return StaticLifetimeGuard<Self>();
+}
+
 } // namespace design_pattern
 } // namespace iox
 

iceoryx_hoofs/test/moduletests/test_static_lifetime_guard.cpp

@@ -130,7 +130,7 @@ TEST_F(StaticLifetimeGuard_test, copyIncreasesLifetimeCount)
     {
         EXPECT_EQ(Guard::count(), 2);
         // NOLINTJUSTIFICATION ctor and dtor side effects are tested
-        // NOLINTNEXTLINE (performance-unnecessary-copy-initialization)
+        // NOLINTNEXTLINE(performance-unnecessary-copy-initialization)
         Guard copy(guard);
         EXPECT_EQ(Guard::count(), 3);
     }
@@ -164,6 +164,8 @@ TEST_F(StaticLifetimeGuard_test, destructionAtZeroCountWorks)
     EXPECT_EQ(Foo::dtorCalled, 1);
 }
 
+// TODO: this may not be completely thread-safe in general
+// and is not really required anyway
 TEST_F(StaticLifetimeGuard_test, constructionAfterDestructionWorks)
 {
     ::testing::Test::RecordProperty("TEST_ID", "0077e73d-ddf5-47e7-a7c6-93819f376175");