diff --git a/src/ripple/core/Stoppable.h b/src/ripple/core/Stoppable.h index 14b5cc1fc0..9748f9ff3f 100644 --- a/src/ripple/core/Stoppable.h +++ b/src/ripple/core/Stoppable.h @@ -44,7 +44,7 @@ class RootStoppable; provides a set of behaviors for ensuring that the start and stop of a composite application-style object is well defined. - Upon the initialization of the composite object these steps are peformed: + Upon the initialization of the composite object these steps are performed: 1. Construct sub-components. @@ -339,14 +339,15 @@ private: /* Notify a root stoppable and children to stop, without waiting. Has no effect if the stoppable was already notified. + Returns true on the first call to stopAsync(), false otherwise. + Thread safety: Safe to call from any thread at any time. */ - void stopAsync(beast::Journal j); + bool stopAsync(beast::Journal j); std::atomic m_prepared; - bool m_calledStop; - std::atomic m_calledStopAsync; + std::atomic m_calledStop; std::mutex m_; std::condition_variable c_; }; @@ -362,7 +363,7 @@ RootStoppable::alertable_sleep_for( std::unique_lock lock(m_); if (m_calledStop) return true; - return c_.wait_for(lock, d, [this]{return m_calledStop;}); + return c_.wait_for(lock, d, [this]{return m_calledStop.load();}); } template diff --git a/src/ripple/core/impl/Stoppable.cpp b/src/ripple/core/impl/Stoppable.cpp index f15378e314..744417e279 100644 --- a/src/ripple/core/impl/Stoppable.cpp +++ b/src/ripple/core/impl/Stoppable.cpp @@ -161,13 +161,12 @@ RootStoppable::RootStoppable (char const* name) : Stoppable (name, *this) , m_prepared (false) , m_calledStop (false) - , m_calledStopAsync (false) { } bool RootStoppable::isStopping() const { - return m_calledStopAsync; + return m_calledStop; } void RootStoppable::prepare () @@ -191,25 +190,30 @@ void RootStoppable::stop (beast::Journal j) // Must have a prior call to start() assert (m_started); - { - std::lock_guard lock(m_); - if (m_calledStop) - { - if (auto stream = j.warn()) - stream << "Stoppable::stop called again"; - return; - } - m_calledStop = true; - c_.notify_all(); - } - stopAsync (j); - stopRecursive (j); + if (stopAsync (j)) + stopRecursive (j); } -void RootStoppable::stopAsync(beast::Journal j) +bool RootStoppable::stopAsync(beast::Journal j) { - if (m_calledStopAsync.exchange (true) == false) - stopAsyncRecursive(j); + bool alreadyCalled; + { + // Even though m_calledStop is atomic, we change its value under a + // lock. This removes a small timing window that occurs if the + // waiting thread is handling a spurious wakeup while m_calledStop + // changes state. + std::unique_lock lock (m_); + alreadyCalled = m_calledStop.exchange (true); + } + if (alreadyCalled) + { + if (auto stream = j.warn()) + stream << "Stoppable::stop called again"; + return false; + } + c_.notify_all(); + stopAsyncRecursive(j); + return true; } } diff --git a/src/test/core/Stoppable_test.cpp b/src/test/core/Stoppable_test.cpp index 5a4fe6989f..43f4d685c4 100644 --- a/src/test/core/Stoppable_test.cpp +++ b/src/test/core/Stoppable_test.cpp @@ -436,6 +436,13 @@ class Stoppable_test Stoppable::stopped(); test_.expect(--test_.count == 0, "Root::onChildrenStopped called out of order"); } + + void secondStop() + { + // Calling stop() a second time should have no negative + // consequences. + stop({}); + } }; public: @@ -444,6 +451,7 @@ public: { Root rt(*this); rt.run(); + rt.secondStop(); } pass(); }