diff --git a/iceoryx_hoofs/include/iceoryx_hoofs/design_pattern/polymorphic_handler.hpp b/iceoryx_hoofs/include/iceoryx_hoofs/design_pattern/polymorphic_handler.hpp index 3f725804534..4381d55999c 100644 --- a/iceoryx_hoofs/include/iceoryx_hoofs/design_pattern/polymorphic_handler.hpp +++ b/iceoryx_hoofs/include/iceoryx_hoofs/design_pattern/polymorphic_handler.hpp @@ -19,7 +19,6 @@ #include #include -#include #include #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::value, "Interface must inherit from Activatable"); + using Self = PolymorphicHandler; + friend class StaticLifetimeGuard; + 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(); - } + /// @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 m_defaultGuard; std::atomic_bool m_isFinal{false}; - std::mutex m_mutex; std::atomic m_current; PolymorphicHandler() noexcept; diff --git a/iceoryx_hoofs/include/iceoryx_hoofs/design_pattern/static_lifetime_guard.hpp b/iceoryx_hoofs/include/iceoryx_hoofs/design_pattern/static_lifetime_guard.hpp index a321ab59278..cb07b7d03c9 100644 --- a/iceoryx_hoofs/include/iceoryx_hoofs/design_pattern/static_lifetime_guard.hpp +++ b/iceoryx_hoofs/include/iceoryx_hoofs/design_pattern/static_lifetime_guard.hpp @@ -17,6 +17,7 @@ #pragma once #include +#include #include 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 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)...); // 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; + 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 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 s_instanceState; static T* s_instance; + std::atomic m_value{1}; + static void destroy() { if (s_instance) { s_instance->~T(); s_instance = nullptr; + s_instanceState = 0; } } }; @@ -145,6 +166,8 @@ typename StaticLifetimeGuard::storage_t StaticLifetimeGuard::s_storage; template std::atomic StaticLifetimeGuard::s_count{0}; template +std::atomic StaticLifetimeGuard::s_instanceState{UNINITIALIZED}; +template T* StaticLifetimeGuard::s_instance{nullptr}; // NOLINTEND (cppcoreguidelines-avoid-non-const-global-variables) diff --git a/iceoryx_hoofs/include/iceoryx_hoofs/internal/design_pattern/polymorphic_handler.inl b/iceoryx_hoofs/include/iceoryx_hoofs/internal/design_pattern/polymorphic_handler.inl index 14205ddeb62..0daf57eb645 100644 --- a/iceoryx_hoofs/include/iceoryx_hoofs/internal/design_pattern/polymorphic_handler.inl +++ b/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 #include namespace iox @@ -86,30 +87,29 @@ Interface& PolymorphicHandler::get() noexcept if (!localHandler->isActive()) { - std::lock_guard lock(instance().m_mutex); localHandler = getCurrent(); } return *localHandler; } -// can throw template Interface* PolymorphicHandler::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 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 void PolymorphicHandler::finalize() noexcept { auto& ins = instance(); - std::lock_guard lock(ins.m_mutex); ins.m_isFinal.store(true); } template PolymorphicHandler::PolymorphicHandler() noexcept { - std::lock_guard lock(m_mutex); - // this is set the first time we call instance() m_current.store(&getDefault(), std::memory_order_release); } template PolymorphicHandler& PolymorphicHandler::instance() noexcept { - static PolymorphicHandler handler; - return handler; + return StaticLifetimeGuard::instance(); } template @@ -158,6 +154,12 @@ Interface* PolymorphicHandler::getCurrent() noexcept std::memory_order_acquire); // must be strong enough to sync memory of the object pointed to } +template +auto PolymorphicHandler::guard() noexcept +{ + return StaticLifetimeGuard(); +} + } // namespace design_pattern } // namespace iox diff --git a/iceoryx_hoofs/test/moduletests/test_static_lifetime_guard.cpp b/iceoryx_hoofs/test/moduletests/test_static_lifetime_guard.cpp index 24b3d5d6fd4..11cb8a679c5 100644 --- a/iceoryx_hoofs/test/moduletests/test_static_lifetime_guard.cpp +++ b/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");