From b35cb02b2eaf05e5478524c1b59ccde1f7de6aef Mon Sep 17 00:00:00 2001 From: Matthias Killat Date: Mon, 30 Jan 2023 23:05:31 +0100 Subject: [PATCH] iox-#1640 Correct concurrent StaticLifetimeGuard destruction Signed-off-by: Matthias Killat --- .../design_pattern/polymorphic_handler.hpp | 2 +- .../design_pattern/static_lifetime_guard.hpp | 9 ++++- .../design_pattern/static_lifetime_guard.inl | 35 +++++++++++++++++-- .../test_static_lifetime_guard.cpp | 2 +- 4 files changed, 42 insertions(+), 6 deletions(-) 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 19a3231521..b2dea21b35 100644 --- a/iceoryx_hoofs/include/iceoryx_hoofs/design_pattern/polymorphic_handler.hpp +++ b/iceoryx_hoofs/include/iceoryx_hoofs/design_pattern/polymorphic_handler.hpp @@ -63,7 +63,7 @@ struct DefaultHooks template > class PolymorphicHandler { - static_assert(std::is_base_of::value, "Default must inherit from Interface"); + static_assert(std::is_base_of::value, "Interface must be a base class of Default"); using Self = PolymorphicHandler; friend class StaticLifetimeGuard; 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 c20d6278b2..11864ff07e 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 @@ -29,8 +29,15 @@ namespace design_pattern /// existing StaticLifetimeGuard prevents the destruction of /// the instance. /// 1. instance() creates a static guard itself and hence has static lifetime -/// 2. any static StaticLifetimeGuard G created before that prolongs the lifetime +/// 2. Any static StaticLifetimeGuard G created before that prolongs the lifetime /// of the instance at least until G is destroyed +/// 3. The instance is lazily constructed, i.e. only when first used. +/// Hence there can be guards without any instance existing. +/// These guards still protect the instance from destruction if it is ever constructed. +/// 4. If and once the instance is constructed, it will be destructed only after main exits (static +/// destruction time). +/// Existing guards used variables must be used to control destruction order +/// of static variables if a specific order is required. /// @tparam T the type of the instance to be guarded /// /// @note all public functions are thread-safe diff --git a/iceoryx_hoofs/include/iceoryx_hoofs/internal/design_pattern/static_lifetime_guard.inl b/iceoryx_hoofs/include/iceoryx_hoofs/internal/design_pattern/static_lifetime_guard.inl index 65a76538c0..e2aef79226 100644 --- a/iceoryx_hoofs/include/iceoryx_hoofs/internal/design_pattern/static_lifetime_guard.inl +++ b/iceoryx_hoofs/include/iceoryx_hoofs/internal/design_pattern/static_lifetime_guard.inl @@ -19,6 +19,7 @@ #include "iceoryx_hoofs/design_pattern/static_lifetime_guard.hpp" +#include #include namespace iox @@ -71,6 +72,7 @@ template template T& StaticLifetimeGuard::instance(Args&&... args) noexcept { + // primary guard static StaticLifetimeGuard guard; // we determine wether this call has to initialize the instance @@ -117,11 +119,38 @@ uint64_t StaticLifetimeGuard::count() template void StaticLifetimeGuard::destroy() { + // instance either exists, i.e. instance() was called and returned + // or is being called for the first time and has already set the instance + // 1) was called is no problem, because this means the primary guard must have + // gone out of scope at program end as otherwise destroy would not be called + // 2) is called for first time and has not yet returned + // - the state is INITIALIZED or INITIALIZING + // - s_count is > 0 due to the primary guard + // - s_count went up after it went to zero due to the guard that triggered this destroy + // + // NB: instance can be called for the first time in a destructor + // of a static object that is destroyed after main; + // unusual but OK if guards are used correctly to control the destruction order of all statics if (s_instance) { - s_instance->~T(); - s_instance = nullptr; - s_instanceState = UNINITIALIZED; + // there is an instance, so there MUST be a primary guard (that may have + // triggered this destroy) + // + // check the counter again, if it is zero the primary guard and all others that existed + // are already destroyed or being destroyed (one of them triggered this destroy) + uint64_t exp{0}; + // destroy is a rare operation and the memory order is intentional to ensure + // memory synchronization of s_instance and limit reordering. + if (s_count.compare_exchange_strong(exp, 0, std::memory_order_acq_rel)) + { + // s_count is 0, so we know the primary guard was destroyed before this OR + // has triggered this destroy + // this will only happen at program end when the primary guard goes out of scope + s_instance->~T(); + s_instance = nullptr; + // NB: reinitialization is normally only possible at program end (static destruction) + s_instanceState = UNINITIALIZED; + } } } diff --git a/iceoryx_hoofs/test/moduletests/test_static_lifetime_guard.cpp b/iceoryx_hoofs/test/moduletests/test_static_lifetime_guard.cpp index ca1d462f42..519a8f529a 100644 --- a/iceoryx_hoofs/test/moduletests/test_static_lifetime_guard.cpp +++ b/iceoryx_hoofs/test/moduletests/test_static_lifetime_guard.cpp @@ -306,7 +306,7 @@ TEST_F(StaticLifetimeGuard_test, constructionAfterDestructionWorks) EXPECT_EQ(instance.id, FIRST_INSTANCE_ID); } - // first instance destroyed (should usually only happen at the the program + // first instance destroyed (should usually only happen at the the program end // during static destruction) T::Foo::reset();