iox-#1640 Correct concurrent StaticLifetimeGuard destruction

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

iceoryx_hoofs/include/iceoryx_hoofs/design_pattern/polymorphic_handler.hpp

@@ -63,7 +63,7 @@ struct DefaultHooks
 template <typename Interface, typename Default, typename Hooks = detail::DefaultHooks<Interface>>
 class PolymorphicHandler
 {
-    static_assert(std::is_base_of<Interface, Default>::value, "Default must inherit from Interface");
+    static_assert(std::is_base_of<Interface, Default>::value, "Interface must be a base class of Default");
 
     using Self = PolymorphicHandler<Interface, Default, Hooks>;
     friend class StaticLifetimeGuard<Self>;

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

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 <atomic>
 #include <thread>
 
 namespace iox
@@ -71,6 +72,7 @@ template <typename T>
 template <typename... Args>
 T& StaticLifetimeGuard<T>::instance(Args&&... args) noexcept
 {
+    // primary guard
     static StaticLifetimeGuard<T> guard;
 
     // we determine wether this call has to initialize the instance
@@ -117,11 +119,38 @@ uint64_t StaticLifetimeGuard<T>::count()
 template <typename T>
 void StaticLifetimeGuard<T>::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;
+        }
     }
 }
 

iceoryx_hoofs/test/moduletests/test_static_lifetime_guard.cpp

@@ -292,6 +292,7 @@ TEST_F(StaticLifetimeGuard_test, destructionAtZeroCountWorks)
     EXPECT_EQ(T::Foo::dtorCalled, 1);
 }
 
+// TODO: decide whether we want this
 TEST_F(StaticLifetimeGuard_test, constructionAfterDestructionWorks)
 {
     ::testing::Test::RecordProperty("TEST_ID", "0077e73d-ddf5-47e7-a7c6-93819f376175");
@@ -306,7 +307,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();