Skip to content

Commit

Permalink
iox-eclipse-iceoryx#1640 Remove Activatable restriction
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 cf6dc33 commit 65e2ec6
Show file tree
Hide file tree
Showing 4 changed files with 34 additions and 199 deletions.
34 changes: 13 additions & 21 deletions doc/design/polymorphic_handler.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -123,19 +123,10 @@ guards) are destroyed, but this happens only during program termination.

## Using the Polymorphic Handler

### The Activatable interface

To allow atomic activation and deactivation of the handler without the use of mutexes, we require the
interface `I` to implement the `Activatable` interface (basically a concept). This just uses an atomic
flag and provides two functions to set and unset the flag.

Concretely, this requires `I` to derive from `Activatable` (could be solved without inheritance as
well but then we cannot easily enforce the interface).

### Basic Usage

```cpp
struct Interface : public Activatable {
struct Interface {
public:
virtual void foo() = 0;
};
Expand Down Expand Up @@ -204,9 +195,7 @@ handler.
If externally set handlers are required to live even longer, explicit guards of them must be kept by
other static objects.
### Why implementing Activatable is required
Requiring any handler to derive from `Activatable` (via the interface `I`) is a mild restriction.
### Switching between multiple handlers
When a new handler is set by
Expand All @@ -218,9 +207,7 @@ Handler::set(guard);

1. Create a guard for the new handler
1. Obtain the new handler instance from the guard
1. Activate the new handler (via `Activatable::activate`)
1. Exchange the old handler with the new handler
1. Deactiavte the old handler (via `Activatable::deactivate`)

Afterwards both handlers still exist (they have static lifetime), and using either works if a
reference to it is known. Any threads calling `Handler::get` will use the new handler, but there may
Expand All @@ -231,14 +218,19 @@ Any thread keeps track of its latest known local handler using a `thread_local`
handler is initialized the first time the thread uses `Handler`. This is guaranteed to provide the
latest handler instance, but the latest instance can change in the meantime.

The thread can then check whether the local handler is still considered active
(`Activatable::isActive`). If it is active the thread will proceed to use the local handler (which
is active at this very moment **but may change any time after the check**). Otherwise it will obtain
the new handler.
The thread can then check whether the local handler is still considered by comparing it to the
global handler (which can be loaded with a relaxed atomic). If both are equal then it is considered
unchanged and the thread will proceed to use the local handler.
Otherwise it will obtain the new handler with a stronger memory synchronization (more costly).

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.

This does not require blocking and only relies on fairly cheap atomic operations.
Without using a mutex, it is unavoidable that threads will always use the latest handler (as it may
change at any time). However, this is not required, it only is required that a handler that is
Without using a mutex while using the handler, it is impossible that
threads will always use the latest handler (as it maychange at any time).
However, this is not required, it only is required that a handler that is
obtained can be safely accessed. The latter is ensured by using `StaticLifetimeGuard`.

### Thread safety
Expand Down
37 changes: 0 additions & 37 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 @@ -27,39 +27,6 @@ namespace iox
{
namespace design_pattern
{
/// @brief Implements the Activatable concept to be used in the PolymorphicHandler
/// The concept implements a binary switch. By default is switched on (active).
/// Anyone defining another custom handler interface is supposed to derive from Activatable.
/// @note While this is public, it is also partially implementation detail and partially convenience
/// to use the PolymorphicHandler.
class Activatable
{
public:
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;

/// @brief Switch off.
void deactivate() noexcept;

/// @brief Query switch state.
/// @return true if active (on), false otherwise (off).
bool isActive() const noexcept;

private:
std::atomic<bool> m_active{true};
};

namespace detail
{

Expand Down Expand Up @@ -96,10 +63,6 @@ class PolymorphicHandler
{
static_assert(std::is_base_of<Interface, Default>::value, "Default must inherit from Interface");

// actually it suffices to provide the methods activate, deactivate, isActive
// but they need to behave correctly and inheritance enforces this
static_assert(std::is_base_of<Activatable, Interface>::value, "Interface must inherit from Activatable");

using Self = PolymorphicHandler<Interface, Default, Hooks>;
friend class StaticLifetimeGuard<Self>;

Expand Down
65 changes: 19 additions & 46 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,66 +40,47 @@ 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);
}

void Activatable::deactivate() noexcept
{
m_active.store(false, std::memory_order_relaxed);
}

bool Activatable::isActive() const noexcept
{
return m_active.load(std::memory_order_relaxed);
}

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

// 2. If any thread changes the active handler with set or reset, it will:
// - set the new handler to active
// 2. If any thread changes the handler with set or reset, it will:
// - be detected by
// - 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
// 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 but we do not loop in order to prevent blocking by misuse.
// 1. We detect the change by comparison with the localHandler (can assume that addresses are unique by
// design as we use singletons in static memory)
// 2. If it was changed we perform a synchronizing load and obtain the current handler
// Note that it may change again but this is unavoidable if it can change concurrently
// without locks and not a problem.
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

if (!localHandler->isActive())
// weak load, required since it could be set in another thread concurrently
auto* currentHandler = instance().m_current.load(std::memory_order_relaxed);

// is the known localHandler outdated?
// NB: this is a variation of double checked locking but with atomics:
// we avoid the more costly operation on the hot path unless there was a change (rare case)
if (localHandler != currentHandler)
{
// stronger sync of memory
localHandler = getCurrent();
// note that it may concurrently change again but we do not loop to check as there is no
// point in such a lock-free code (it may also change while returning)
}

return *localHandler;
}

Expand Down Expand Up @@ -128,16 +109,8 @@ Interface* PolymorphicHandler<Interface, Default, Hooks>::setHandler(Interface&
return nullptr;
}

handler.activate(); // it may have been deactivated before, so always reactivate it
auto prev = ins.m_current.exchange(&handler, std::memory_order_acq_rel);

// 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 Down
97 changes: 2 additions & 95 deletions iceoryx_hoofs/test/moduletests/test_polymorphic_handler.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ namespace
{
using namespace ::testing;

struct Interface : public iox::design_pattern::Activatable
struct Interface
{
virtual ~Interface() = default;

Expand Down Expand Up @@ -100,7 +100,6 @@ TEST_F(PolymorphicHandler_test, handlerIsInitializedWithDefault)
auto& handler = Handler::get();

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

TEST_F(PolymorphicHandler_test, settingAlternateWorks)
Expand All @@ -110,11 +109,9 @@ TEST_F(PolymorphicHandler_test, settingAlternateWorks)
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 Down Expand Up @@ -151,7 +148,6 @@ TEST_F(PolymorphicHandler_test, returnValueOfSetPointsToPreviousInstance)

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

TEST_F(PolymorphicHandler_test, resetToDefaultWorks)
Expand All @@ -166,9 +162,6 @@ TEST_F(PolymorphicHandler_test, resetToDefaultWorks)

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

EXPECT_FALSE(prevHandler.isActive());
}

TEST_F(PolymorphicHandler_test, setToCurrentHandlerWorks)
Expand All @@ -177,16 +170,14 @@ TEST_F(PolymorphicHandler_test, setToCurrentHandlerWorks)

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

// set to alternateHandler again, should stay active
// set to alternateHandler again
// 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)
Expand Down Expand Up @@ -233,88 +224,4 @@ TEST_F(PolymorphicHandler_test, obtainingGuardWorks)
EXPECT_EQ(Guard<Handler>::count(), 2);
}

class Activatable_test : public Test
{
public:
void SetUp() override
{
}

void TearDown() override
{
}

iox::design_pattern::Activatable sut;
};

TEST_F(Activatable_test, isActiveAfterConstruction)
{
::testing::Test::RecordProperty("TEST_ID", "874b600a-7976-4c97-a800-55bac11c4eaa");
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");
sut.deactivate();
EXPECT_FALSE(sut.isActive());
}

TEST_F(Activatable_test, isNotActiveAfterMultiDeactivate)
{
::testing::Test::RecordProperty("TEST_ID", "8ab19dd3-83a4-4e95-a4d2-3c9d973ab28b");
sut.deactivate();
sut.deactivate();
EXPECT_FALSE(sut.isActive());
}

TEST_F(Activatable_test, isActiveAfterReactivation)
{
::testing::Test::RecordProperty("TEST_ID", "ec26ea62-d979-4f28-89a2-59d4639b52b2");
sut.deactivate();
sut.activate();
EXPECT_TRUE(sut.isActive());
}

TEST_F(Activatable_test, isActiveAfterMultiActivation)
{
::testing::Test::RecordProperty("TEST_ID", "5593d002-394b-4e30-908c-d56d9b56c58e");
sut.activate();
EXPECT_TRUE(sut.isActive());

sut.deactivate();
sut.activate();
sut.activate();
EXPECT_TRUE(sut.isActive());
}

} // namespace

0 comments on commit 65e2ec6

Please sign in to comment.