From a4f42dccbca8ca5552c98f577915c82a78b2f662 Mon Sep 17 00:00:00 2001 From: Matthias Killat Date: Tue, 31 Jan 2023 14:40:13 +0100 Subject: [PATCH] iox-#1640 Update comments and documentation Signed-off-by: Matthias Killat --- doc/design/polymorphic_handler.md | 16 ++++++++++++---- .../design_pattern/polymorphic_handler.hpp | 5 ++--- .../design_pattern/static_lifetime_guard.hpp | 4 ++-- .../design_pattern/polymorphic_handler.inl | 2 +- .../design_pattern/static_lifetime_guard.inl | 3 +-- .../moduletests/test_polymorphic_handler.cpp | 2 +- .../moduletests/test_static_lifetime_guard.cpp | 4 ++-- 7 files changed, 21 insertions(+), 15 deletions(-) diff --git a/doc/design/polymorphic_handler.md b/doc/design/polymorphic_handler.md index 6094184a3a7..3fd58849b19 100644 --- a/doc/design/polymorphic_handler.md +++ b/doc/design/polymorphic_handler.md @@ -11,10 +11,10 @@ that also inherits from `I` variables 1. Obtaining the current handler must be thread-safe 1. It must be possible to finalize the handler, i.e. prohibit any changes after it is finalized. -1. Any attempt to change the handler after it is finalized, shall call an function that has access +1. Any attempt to change the handler after it is finalized, shall call a function that has access to the current and new handler (for e.g. logging). -To achieve this, we define another support class `StaticLifeTimeGuard` that solves the singleton lifetime problem. +To achieve this, we define another support class `StaticLifetimeGuard` that solves the singleton lifetime problem. This class can be used on its own and is based on the nifty counter reference counting. While obtaining the instance is thread-safe, the instance managed by the handler may not be @@ -172,7 +172,15 @@ StaticLifetimeGuard guard; // set the handler to the instance guarded by guard, // this will create another guard to ensure the lifetime -Handler::set(guard); +bool success = Handler::set(guard); +if(!success) +{ + // setting may only fail after finalize + // + // do something in this case + // ... + // even if set was not successful, get() will return the previous handler +} // OtherHandler instance exists now, // any other thread will eventually use the new handler @@ -226,7 +234,7 @@ Otherwise it will obtain the new handler with a stronger memory synchronization Note that the current handler can change any time but there is no problem as all handlers remain usbale during the entire program lifetime. Due to this, there are no issues like the ABA problem, -the worst thing that can happen is working with a outdated handler. +the worst thing that can happen is working with an outdated handler. This does not require blocking and only relies on fairly cheap atomic operations. Without using a mutex while using the handler, it is impossible that 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 b2dea21b359..fd87afe4a15 100644 --- a/iceoryx_hoofs/include/iceoryx_hoofs/design_pattern/polymorphic_handler.hpp +++ b/iceoryx_hoofs/include/iceoryx_hoofs/design_pattern/polymorphic_handler.hpp @@ -18,7 +18,6 @@ #define IOX_HOOFS_DESIGN_PATTERN_POLYMORPHIC_HANDLER_HPP #include -#include #include #include "iceoryx_hoofs/design_pattern/static_lifetime_guard.hpp" @@ -39,7 +38,8 @@ struct DefaultHooks /// @brief called if the polymorphic handler is set or reset after finalize /// @param currentInstance the current instance of the handler singleton /// @param newInstance the instance of the handler singleton to be set - static void onSetAfterFinalize(Interface& currentInstance, Interface& newInstance) noexcept; + /// @note calls terminate and does not return + [[noreturn]] static void onSetAfterFinalize(Interface& currentInstance, Interface& newInstance) noexcept; }; } // namespace detail @@ -53,7 +53,6 @@ struct DefaultHooks /// attempting to set or reset the handler after finalize was called. /// /// @note In the special case where Default equals Interface, no polymorphism is required. -/// It is then possible to e.g. switch between multiple instances of Default type. /// @note The lifetime of external non-default instances must exceed the lifetime of the PolymorphicHandler. /// @note The PolymorphicHandler is guaranteed to provide a valid handler during the whole program lifetime (static). /// It is hence not advisable to have other static variables depend on the PolymorphicHandler. 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 11864ff07e4..d3f6f048088 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 @@ -36,8 +36,8 @@ namespace design_pattern /// 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. +/// 5. Guards can be used in static variables to control destruction order of static (singleton) +/// instances if a specific order of destruction 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/polymorphic_handler.inl b/iceoryx_hoofs/include/iceoryx_hoofs/internal/design_pattern/polymorphic_handler.inl index 348a7e713d5..88941864a68 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 @@ -31,7 +31,7 @@ namespace detail { template -void DefaultHooks::onSetAfterFinalize(Interface&, Interface&) noexcept +[[noreturn]] void DefaultHooks::onSetAfterFinalize(Interface&, Interface&) noexcept { // we should not use an error handling construct (e.g. some IOX_ASSERT) here for dependency reasons // we could in principle do nothing by default as well, but the misuse failure should have visible consequences 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 e2aef79226c..3e0b1cca48b 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 @@ -72,8 +72,7 @@ template template T& StaticLifetimeGuard::instance(Args&&... args) noexcept { - // primary guard - static StaticLifetimeGuard guard; + static StaticLifetimeGuard primaryGuard; // we determine wether this call has to initialize the instance // via CAS (without mutex!) diff --git a/iceoryx_hoofs/test/moduletests/test_polymorphic_handler.cpp b/iceoryx_hoofs/test/moduletests/test_polymorphic_handler.cpp index d26844e7ae4..b72ea4a475c 100644 --- a/iceoryx_hoofs/test/moduletests/test_polymorphic_handler.cpp +++ b/iceoryx_hoofs/test/moduletests/test_polymorphic_handler.cpp @@ -223,7 +223,7 @@ TEST_F(PolymorphicHandler_test, settingAfterFinalizeCallsHook) // we always finalize it to be alternateHandler Handler::set(alternateGuard); - // reset the handler value to non-zero and check later whether they are set to non-zero as expecteded + // reset the handler value to zero and check later whether they are set to non-zero as expected defaultHandler.reset(); alternateHandler.reset(); diff --git a/iceoryx_hoofs/test/moduletests/test_static_lifetime_guard.cpp b/iceoryx_hoofs/test/moduletests/test_static_lifetime_guard.cpp index 519a8f529ad..ee6d31b5f0a 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 end + // first instance destroyed (should usually only happen at the program end // during static destruction) T::Foo::reset(); @@ -349,7 +349,7 @@ TEST_F(StaticLifetimeGuard_test, instanceCtorIsConcurrentlyCalledExactlyOnce) // all threads have notfied (but may pass wait in any order ...) // cannot wait too long otherwise we slow down the tests too much, - // cannot be optimized away, as it has has side effects (counting) + // cannot be optimized away, as it has side effects (counting) Sut::instance(std::chrono::milliseconds(1)); };