Skip to content

Commit

Permalink
iox-eclipse-iceoryx#1640 Correct activation during self exchange
Browse files Browse the repository at this point in the history 
Signed-off-by: Matthias Killat <[email protected]>
  • Loading branch information
@MatthiasKillat
MatthiasKillat committed Jan 24, 2023
1 parent 78f9443 commit cf6dc33
Show file tree
Hide file tree
Showing 3 changed files with 127 additions and 28 deletions.
21 changes: 17 additions & 4 deletions iceoryx_hoofs/include/iceoryx_hoofs/design_pattern/polymorphic_handler.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,16 @@ namespace design_pattern
class Activatable
{
public:
Activatable() = default;
Activatable() noexcept = default;

Activatable(const Activatable&) noexcept;
Activatable& operator=(const Activatable&) noexcept;

// there is no useful move operation
Activatable(Activatable&&) noexcept = delete;
Activatable& operator=(Activatable&&) noexcept = delete;

~Activatable() noexcept = default;

/// @brief Switch on.
void activate() noexcept;
Expand Down Expand Up @@ -95,13 +104,17 @@ class PolymorphicHandler
friend class StaticLifetimeGuard<Self>;

public:
/// @brief get the current singleton instance
/// @brief set the current singleton instance
/// @return the current instance
/// @note we cannot be sure to use the current handler unless we call get,
/// i.e. a reference obtained from get may reference a previous handler
/// (that is still functional but inactive)
static Interface& get() noexcept;

/// @brief set the current singleton instance
/// @param handlerGuard a guard to the handler instance to be set
/// @return pointer to the previous instance
/// @return pointer to the previous instance or nullptr if the handler could not be replaced
/// @note the handler cannot be replaced if it was finalized
/// @note using a guard in the interface prevents the handler to be destroyed while it is used,
/// passing the guard by value is necessary (it has no state anyway)
template <typename Handler>
Expand Down Expand Up @@ -145,4 +158,4 @@ class PolymorphicHandler

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

#endif // IOX_HOOFS_DESIGN_PATTERN_POLYMORPHIC_HANDLER_HPP
#endif // IOX_HOOFS_DESIGN_PATTERN_POLYMORPHIC_HANDLER_HPP
45 changes: 30 additions & 15 deletions iceoryx_hoofs/include/iceoryx_hoofs/internal/design_pattern/polymorphic_handler.inl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,20 @@ void DefaultHooks<Interface>::onSetAfterFinalize(Interface&, Interface&) noexcep

} // namespace detail

Activatable::Activatable(const Activatable& other) noexcept
: m_active(other.m_active.load(std::memory_order_relaxed))
{
}

Activatable& Activatable::operator=(const Activatable& other) noexcept
{
if (this != &other)
{
m_active = other.m_active.load(std::memory_order_relaxed);
}
return *this;
}

void Activatable::activate() noexcept
{
m_active.store(true, std::memory_order_relaxed);
Expand All @@ -57,33 +71,32 @@ bool Activatable::isActive() const noexcept

// The get method is considered to be on the hot path and works as follows:
//
// on first call (in a thread):
// On first call (in a thread):
// 1. localHandler is initialized
// - getCurrent is called
// - instantiates singleton instance()
// - instantiates default
// - sets m_current of instance to default instance (release)
// - default is active

// if any thread changes the active handler with set (or reset)
// under mutex protection, it will:
// 2. If any thread changes the active handler with set or reset, it will:
// - set the new handler to active
// - set current handler to the new handler
// - set the current handler to the new handler
// - deactivate the old handler (can still be used as it still needs to exist)
//

// on any call after the handler was changed in another thread
// On any call after the handler was changed in another thread
// 1. we check whether the handler is active
// (can be outdated information but will eventually be false once the atomic value is updated)
// 2. if it was changed it is now inactive and we update the local handler
// Note that it may change again hence we loop (usually it acts like an if, i.e. checks once)
// Note that it may change again but we do not loop in order to prevent blocking by misuse.
template <typename Interface, typename Default, typename Hooks>
Interface& PolymorphicHandler<Interface, Default, Hooks>::get() noexcept
{
// NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables) false positive, thread_local
thread_local Interface* localHandler = getCurrent(); // initialized once per thread on first call

while (!localHandler->isActive())
if (!localHandler->isActive())
{
localHandler = getCurrent();
}
Expand Down Expand Up @@ -118,9 +131,13 @@ Interface* PolymorphicHandler<Interface, Default, Hooks>::setHandler(Interface&
handler.activate(); // it may have been deactivated before, so always reactivate it
auto prev = ins.m_current.exchange(&handler, std::memory_order_acq_rel);

// anyone still using the old handler will eventually see that it is inactive
// and switch to the new handler
prev->deactivate();
// self exchange? if so, do not deactivate
if (&handler != prev)
{
// anyone still using the old handler will eventually see that it is inactive
// and switch to the new handler
prev->deactivate();
}
return prev;
}

Expand All @@ -133,8 +150,7 @@ Interface* PolymorphicHandler<Interface, Default, Hooks>::reset() noexcept
template <typename Interface, typename Default, typename Hooks>
void PolymorphicHandler<Interface, Default, Hooks>::finalize() noexcept
{
auto& ins = instance();
ins.m_isFinal.store(true, std::memory_order_release);
instance().m_isFinal.store(true, std::memory_order_release);
}

template <typename Interface, typename Default, typename Hooks>
Expand All @@ -158,9 +174,8 @@ Default& PolymorphicHandler<Interface, Default, Hooks>::getDefault() noexcept
template <typename Interface, typename Default, typename Hooks>
Interface* PolymorphicHandler<Interface, Default, Hooks>::getCurrent() noexcept
{
auto& ins = instance();
return ins.m_current.load(
std::memory_order_acquire); // must be strong enough to sync memory of the object pointed to
// must be strong enough to sync memory of the object pointed to
return instance().m_current.load(std::memory_order_acquire);
}

template <typename Interface, typename Default, typename Hooks>
Expand Down
89 changes: 80 additions & 9 deletions iceoryx_hoofs/test/moduletests/test_polymorphic_handler.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,15 +100,21 @@ TEST_F(PolymorphicHandler_test, handlerIsInitializedWithDefault)
auto& handler = Handler::get();

EXPECT_EQ(handler.id(), DEFAULT_ID);
EXPECT_TRUE(handler.isActive());
}

TEST_F(PolymorphicHandler_test, settingAlternateWorks)
{
::testing::Test::RecordProperty("TEST_ID", "8b2f0cfe-f13c-4fa0-aa93-5ddd4f0904d1");
Handler::set(alternateGuard);
auto* prevHandler = Handler::set(alternateGuard);
auto& handler = Handler::get();

EXPECT_EQ(handler.id(), ALTERNATE_ID);
EXPECT_TRUE(handler.isActive());

ASSERT_NE(prevHandler, nullptr);
EXPECT_EQ(prevHandler->id(), DEFAULT_ID);
EXPECT_FALSE(prevHandler->isActive());
}

TEST_F(PolymorphicHandler_test, alternatePointsToExternalMemory)
Expand All @@ -126,36 +132,78 @@ TEST_F(PolymorphicHandler_test, explicitlySettingToDefaultWorks)
{
::testing::Test::RecordProperty("TEST_ID", "32e4d808-c848-4bf9-b878-e163ca825539");
Handler::set(alternateGuard);
Handler::set(defaultGuard);
auto* prevHandler = Handler::set(defaultGuard);

auto& handler = Handler::get();
auto* ptr = static_cast<Interface*>(&defaultHandler);

EXPECT_EQ(&handler, ptr);
ASSERT_NE(prevHandler, nullptr);
EXPECT_EQ(prevHandler->id(), ALTERNATE_ID);
}

TEST_F(PolymorphicHandler_test, returnValueOfSetPointsToPreviousInstance)
{
::testing::Test::RecordProperty("TEST_ID", "96447d94-ea27-4d51-8959-12e7752728ae");
auto& expectedHandler = Handler::get();

auto* prevHandler = Handler::set(alternateGuard);

ASSERT_NE(prevHandler, nullptr);
EXPECT_EQ(&expectedHandler, prevHandler);
EXPECT_FALSE(prevHandler->isActive());
}

TEST_F(PolymorphicHandler_test, resetToDefaultWorks)
{
::testing::Test::RecordProperty("TEST_ID", "ef8a99da-22a6-497e-b2ec-bf72cc3ae943");
Handler::set(alternateGuard);
auto& prevHandler = Handler::get();
EXPECT_EQ(prevHandler.id(), ALTERNATE_ID);

// note that we have to use reset to set it back to the internal default
Handler::reset();

auto& handler = Handler::get();
EXPECT_EQ(handler.id(), DEFAULT_ID);
EXPECT_TRUE(handler.isActive());

EXPECT_FALSE(prevHandler.isActive());
}

TEST_F(PolymorphicHandler_test, setToCurrentHandlerWorks)
{
::testing::Test::RecordProperty("TEST_ID", "54e22290-a7b4-4552-a18f-953571381d38");

// change to alternateHandler
Handler::set(alternateGuard);
EXPECT_TRUE(alternateHandler.isActive());

// set to alternateHandler again, should stay active
// while this is a useless operation, we cannot forbid it via interface
auto* prevHandler = Handler::set(alternateGuard);
auto& handler = Handler::get();

EXPECT_EQ(&handler, prevHandler);
EXPECT_EQ(prevHandler, &alternateHandler);
EXPECT_TRUE(handler.isActive());
}

TEST_F(PolymorphicHandler_test, settingAfterFinalizeCallsHook)
{
::testing::Test::RecordProperty("TEST_ID", "171ac802-01b9-4e08-80a6-6f2defecaf6d");

auto& handler = Handler::get();

// reset the handler value to non-zero and check later whether they are set to non-zero as expecteded
handler.reset();
alternateHandler.reset();

// note that all following tests will also call the after finalize
// hook but we only check if we care whether it was called
Handler::finalize();
Handler::set(alternateGuard);
auto* prevHandler = Handler::set(alternateGuard);
EXPECT_EQ(prevHandler, nullptr);

// does the hook set the values to the corresponding arguments?
EXPECT_EQ(handler.value, DEFAULT_ID);
Expand Down Expand Up @@ -190,16 +238,10 @@ class Activatable_test : public Test
public:
void SetUp() override
{
internal::CaptureStderr();
}

void TearDown() override
{
std::string output = internal::GetCapturedStderr();
if (Test::HasFailure())
{
std::cout << output << std::endl;
}
}

iox::design_pattern::Activatable sut;
Expand All @@ -211,6 +253,35 @@ TEST_F(Activatable_test, isActiveAfterConstruction)
EXPECT_TRUE(sut.isActive());
}

TEST_F(Activatable_test, copyCtorWorks)
{
::testing::Test::RecordProperty("TEST_ID", "f8e6b2c7-a8bf-441d-8066-66096329b21f");
{
auto copy(sut);
EXPECT_TRUE(copy.isActive());
}

{
sut.deactivate();
auto copy(sut);
EXPECT_FALSE(copy.isActive());
}
}

TEST_F(Activatable_test, copyAssignmentWorks)
{
::testing::Test::RecordProperty("TEST_ID", "241ad501-2295-4da7-accd-50872264997d");
iox::design_pattern::Activatable other;

sut.deactivate();
EXPECT_FALSE(sut.isActive());
sut = other;
EXPECT_TRUE(sut.isActive());
other.deactivate();
sut = other;
EXPECT_FALSE(sut.isActive());
}

TEST_F(Activatable_test, isNotActiveAfterDeactivate)
{
::testing::Test::RecordProperty("TEST_ID", "b9f052b1-33dd-4e71-9887-26581d219492");
Expand Down

0 comments on commit cf6dc33

Please sign in to comment.