Skip to content

Commit

Permalink
-- fix potential deadlock in TimeProvider (#729)
Browse files Browse the repository at this point in the history
  • Loading branch information
@VDanielEdwards
VDanielEdwards authored and GitHub Enterprise committed Nov 28, 2023
1 parent 337bec4 commit ad87b85
Show file tree
Hide file tree
Showing 3 changed files with 183 additions and 140 deletions.
187 changes: 90 additions & 97 deletions SilKit/source/services/orchestration/TimeProvider.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,92 +34,69 @@ constexpr auto DEFAULT_NOW_TIMESTAMP_WITHOUT_SYNC = std::chrono::nanoseconds::mi


using namespace std::chrono_literals;


namespace SilKit {
namespace Services {
namespace Orchestration {

// Actual Provider Implementations
namespace detail {
namespace {

class ProviderBase : public ITimeProviderInternal

class ProviderBase : public ITimeProviderImpl
{
public:
ProviderBase(std::string name)
ProviderBase(std::string name, ITimeProviderImplListener& listener)
: _name{std::move(name)}
, _listener{&listener}
{
}

auto Now() const -> std::chrono::nanoseconds override
{
return _now;
}
// ITimeProvierImpl (partial)

auto TimeProviderName() const -> const std::string& override
auto TimeProviderName() const -> const std::string& final
{
return _name;
}

HandlerId AddNextSimStepHandler(NextSimStepHandler nextSimStepHandler) override
{
return _handlers.Add(std::move(nextSimStepHandler));
}

void RemoveNextSimStepHandler(HandlerId handlerId) override
void SetActive(bool value) final
{
_handlers.Remove(handlerId);
_active = value;
}

void SetTime(std::chrono::nanoseconds now, std::chrono::nanoseconds ) override
{
_now = now;
}

void ConfigureTimeProvider(Orchestration::TimeProviderKind ) override
{
// No Op
}

void SetSynchronizeVirtualTime(bool isSynchronizingVirtualTime) override
{
_isSynchronizingVirtualTime = isSynchronizingVirtualTime;
}

bool IsSynchronizingVirtualTime() const override
protected:
void NotifyListenerAboutTick(std::chrono::nanoseconds now, std::chrono::nanoseconds duration)
{
return _isSynchronizingVirtualTime;
}
if (!_active)
{
return;
}

public:
auto MutableNextSimStepHandlers() -> Util::SynchronizedHandlers<NextSimStepHandler> & override
{
return _handlers;
_listener->OnTick(now, duration);
}

protected:
std::chrono::nanoseconds _now{DEFAULT_NOW_TIMESTAMP_WITHOUT_SYNC};
private:
std::string _name;
bool _isSynchronizingVirtualTime{false};
Util::SynchronizedHandlers<NextSimStepHandler> _handlers;
ITimeProviderImplListener* _listener;
std::atomic<bool> _active{false};
};

class WallclockProvider : public ProviderBase

class WallclockProvider final : public ProviderBase
{
public:
WallclockProvider(std::chrono::nanoseconds tickPeriod)
:ProviderBase("WallclockProvider")
explicit WallclockProvider(ITimeProviderImplListener& consumer, std::chrono::nanoseconds tickPeriod)
: ProviderBase("WallclockProvider", consumer)
, _tickPeriod{tickPeriod}
{
}

// Wall clock provider
HandlerId AddNextSimStepHandler(NextSimStepHandler simStepHandler) override
void OnHandlerAdded() final
{
const auto handlerId = ProviderBase::AddNextSimStepHandler(std::move(simStepHandler));

_timer.WithPeriod(_tickPeriod, [this](const auto& now) {
_handlers.InvokeAll(now, _tickPeriod);
NotifyListenerAboutTick(now, _tickPeriod);
});

return handlerId;
}

auto Now() const -> std::chrono::nanoseconds override
Expand All @@ -128,109 +105,125 @@ class WallclockProvider : public ProviderBase
return now;
}

void SetTime(std::chrono::nanoseconds, std::chrono::nanoseconds) override {}

private:
std::chrono::nanoseconds _tickPeriod{0};
Util::Timer _timer;
};


class NoSyncProvider : public ProviderBase
class NoSyncProvider final : public ProviderBase
{
public:
NoSyncProvider()
: ProviderBase("NoSyncProvider")
explicit NoSyncProvider(ITimeProviderImplListener& consumer)
: ProviderBase("NoSyncProvider", consumer)
{
}

// From Wall clock provider
HandlerId AddNextSimStepHandler(NextSimStepHandler simStepHandler) override
void OnHandlerAdded() override
{
const auto handlerId = ProviderBase::AddNextSimStepHandler(std::move(simStepHandler));

_timer.WithPeriod(_tickPeriod, [this](const auto& now) {
_handlers.InvokeAll(now, _tickPeriod);
NotifyListenerAboutTick(now, _tickPeriod);
});

return handlerId;
}

auto Now() const -> std::chrono::nanoseconds override
{
return DEFAULT_NOW_TIMESTAMP_WITHOUT_SYNC;
}

void SetTime(std::chrono::nanoseconds, std::chrono::nanoseconds) override {}

private:
std::chrono::nanoseconds _tickPeriod{100000};
Util::Timer _timer;
};



//! \brief A caching time provider: we update its internal state whenever the controller's
// simulation time changes.
// This ensures that the our time provider is available even after
// the TimeSyncService gets destructed.
class SynchronizedVirtualTimeProvider : public ProviderBase
/// A caching time provider: we update its internal state whenever the controller's simulation time changes.
/// This ensures that the our time provider is available even after the TimeSyncService gets destructed.
class SynchronizedVirtualTimeProvider final : public ProviderBase
{
public:
SynchronizedVirtualTimeProvider()
: ProviderBase("SynchronizedVirtualTimeProvider")
explicit SynchronizedVirtualTimeProvider(ITimeProviderImplListener& listener)
: ProviderBase("SynchronizedVirtualTimeProvider", listener)
{
}

void OnHandlerAdded() override {}

auto Now() const -> std::chrono::nanoseconds override
{
return _now;
}

void SetTime(std::chrono::nanoseconds now, std::chrono::nanoseconds duration) override
{
_now = now;
// tell our users about the next simulation step
_handlers.InvokeAll(now, duration);
NotifyListenerAboutTick(now, duration);
}

private:
std::chrono::nanoseconds _now{DEFAULT_NOW_TIMESTAMP_WITHOUT_SYNC};
};

} // namespace detail

//TimeProvider
} // namespace


TimeProvider::TimeProvider()
:_currentProvider{std::make_unique<detail::NoSyncProvider>()}
: _currentProvider{std::make_unique<NoSyncProvider>(static_cast<ITimeProviderImplListener&>(*this))}
{
}

void TimeProvider::ConfigureTimeProvider(Orchestration::TimeProviderKind timeProviderKind)
{
const auto isSynchronizingVirtualTime = _currentProvider->IsSynchronizingVirtualTime();
// NB: The destructor of the 'old' time provider implementation must be called _without_ the lock being held.
// Otherwise, the timer-thread (used in the WallclockProvider / NoSyncProvider) will be joined under lock, and
// cause a deadlock when changing implementations.
std::unique_ptr<ITimeProviderImpl> providerPtr;

std::unique_lock<decltype(_mutex)> lock{_mutex};
{
std::unique_lock<decltype(_mutex)> lock{_mutex};

std::unique_ptr<detail::ITimeProviderInternal> providerPtr;
switch (timeProviderKind)
{
case Orchestration::TimeProviderKind::NoSync:
providerPtr = std::make_unique<NoSyncProvider>(static_cast<ITimeProviderImplListener&>(*this));
break;
case Orchestration::TimeProviderKind::WallClock:
providerPtr = std::make_unique<WallclockProvider>(static_cast<ITimeProviderImplListener&>(*this), 1ms);
break;
case Orchestration::TimeProviderKind::SyncTime:
providerPtr = std::make_unique<SynchronizedVirtualTimeProvider>(static_cast<ITimeProviderImplListener&>(*this));
break;
default:
break;
}

switch (timeProviderKind)
{
case Orchestration::TimeProviderKind::NoSync:
providerPtr = std::make_unique<detail::NoSyncProvider>();
break;
case Orchestration::TimeProviderKind::WallClock:
providerPtr = std::make_unique<detail::WallclockProvider>(1ms);
break;
case Orchestration::TimeProviderKind::SyncTime:
providerPtr = std::make_unique<detail::SynchronizedVirtualTimeProvider>();
break;
default: break;
}
if (providerPtr)
{
using std::swap;

if (providerPtr)
{
using std::swap;
_currentProvider->SetActive(false);

// swap the newly created provider with the current provider
swap(_currentProvider, providerPtr);
// swap the newly created provider with the current provider
swap(_currentProvider, providerPtr);

// swap the NextSimStepHandler's of the current provider and the last provider
swap(_currentProvider->MutableNextSimStepHandlers(), providerPtr->MutableNextSimStepHandlers());
_currentProvider->SetActive(true);
}
}
}

_currentProvider->SetSynchronizeVirtualTime(isSynchronizingVirtualTime);
void TimeProvider::OnTick(std::chrono::nanoseconds now, std::chrono::nanoseconds duration)
{
std::unique_lock<decltype(_mutex)> lock{_mutex};
_handlers.InvokeAll(now, duration);
}


} // namespace Orchestration
} // namespace Services
} // namespace SilKit
44 changes: 33 additions & 11 deletions SilKit/source/services/orchestration/TimeProvider.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,15 +34,31 @@ namespace SilKit {
namespace Services {
namespace Orchestration {

namespace detail {
class ITimeProviderInternal : public ITimeProvider
struct ITimeProviderImplListener;

struct ITimeProviderImpl
{
public:
virtual auto MutableNextSimStepHandlers() -> Util::SynchronizedHandlers<NextSimStepHandler> & = 0;
virtual ~ITimeProviderImpl() = default;

virtual auto TimeProviderName() const -> const std::string& = 0;

virtual void SetActive(bool value) = 0;

virtual void OnHandlerAdded() = 0;

virtual auto Now() const -> std::chrono::nanoseconds = 0;

virtual void SetTime(std::chrono::nanoseconds now, std::chrono::nanoseconds duration) = 0;
};

struct ITimeProviderImplListener
{
virtual ~ITimeProviderImplListener() = default;

virtual void OnTick(std::chrono::nanoseconds now, std::chrono::nanoseconds duration) = 0;
};
} // namespace detail

class TimeProvider : public ITimeProvider
class TimeProvider : public ITimeProvider, private ITimeProviderImplListener
{
public:
TimeProvider();
Expand All @@ -60,9 +76,14 @@ class TimeProvider : public ITimeProvider

void ConfigureTimeProvider(Orchestration::TimeProviderKind timeProviderKind) override;

private:
void OnTick(std::chrono::nanoseconds now, std::chrono::nanoseconds duration) final;

private: //Members
mutable std::recursive_mutex _mutex;
std::unique_ptr<detail::ITimeProviderInternal> _currentProvider;
Util::Handlers<NextSimStepHandler> _handlers;
bool _isSynchronizingVirtualTime{false};
std::unique_ptr<ITimeProviderImpl> _currentProvider;
};

//////////////////////////////////////////////////////////////////////
Expand All @@ -85,13 +106,14 @@ auto TimeProvider::TimeProviderName() const -> const std::string&
HandlerId TimeProvider::AddNextSimStepHandler(NextSimStepHandler handler)
{
std::unique_lock<decltype(_mutex)> lock{_mutex};
return _currentProvider->AddNextSimStepHandler(std::move(handler));
_currentProvider->OnHandlerAdded();
return _handlers.Add(std::move(handler));
}

void TimeProvider::RemoveNextSimStepHandler(HandlerId handlerId)
{
std::unique_lock<decltype(_mutex)> lock{_mutex};
_currentProvider->RemoveNextSimStepHandler(handlerId);
_handlers.Remove(handlerId);
}

void TimeProvider::SetTime(std::chrono::nanoseconds now, std::chrono::nanoseconds duration)
Expand All @@ -103,13 +125,13 @@ void TimeProvider::SetTime(std::chrono::nanoseconds now, std::chrono::nanosecond
void TimeProvider::SetSynchronizeVirtualTime(bool isSynchronizingVirtualTime)
{
std::unique_lock<decltype(_mutex)> lock{_mutex};
_currentProvider->SetSynchronizeVirtualTime(isSynchronizingVirtualTime);
_isSynchronizingVirtualTime = isSynchronizingVirtualTime;
}

bool TimeProvider::IsSynchronizingVirtualTime() const
{
std::unique_lock<decltype(_mutex)> lock{_mutex};
return _currentProvider->IsSynchronizingVirtualTime();
return _isSynchronizingVirtualTime;
}

} // namespace Orchestration
Expand Down
Loading

0 comments on commit ad87b85

Please sign in to comment.