Redesign stoppable object pattern

src/ripple/core/impl/JobQueue.cpp

@@ -20,22 +20,21 @@
 #include <ripple/basics/PerfLog.h>
 #include <ripple/basics/contract.h>
 #include <ripple/core/JobQueue.h>
+#include <mutex>
 
 namespace ripple {
 
 JobQueue::JobQueue(
     beast::insight::Collector::ptr const& collector,
-    Stoppable& parent,
     beast::Journal journal,
     Logs& logs,
     perf::PerfLog& perfLog)
-    : Stoppable("JobQueue", parent)
-    , m_journal(journal)
+    : m_journal(journal)
     , m_lastJob(0)
     , m_invalidJobData(JobTypes::instance().getInvalid(), collector, logs)
     , m_processCount(0)
     , m_workers(*this, &perfLog, "JobQueue", 0)
-    , m_cancelCallback(std::bind(&Stoppable::isStopping, this))
+    , m_cancelCallback(std::bind(&JobQueue::isStopping, this))
     , perfLog_(perfLog)
     , m_collector(collector)
 {
@@ -96,24 +95,8 @@ JobQueue::addRefCountedJob(
 
     {
         std::lock_guard lock(m_mutex);
-
-        // If this goes off it means that a child didn't follow
-        // the Stoppable API rules. A job may only be added if:
-        //
-        //  - The JobQueue has NOT stopped
-        //          AND
-        //      * We are currently processing jobs
-        //          OR
-        //      * We have have pending jobs
-        //          OR
-        //      * Not all children are stopped
-        //
-        assert(
-            !isStopped() &&
-            (m_processCount > 0 || !m_jobSet.empty() || !areChildrenStopped()));
-
-        std::pair<std::set<Job>::iterator, bool> result(m_jobSet.insert(
-            Job(type, name, ++m_lastJob, data.load(), func, m_cancelCallback)));
+        auto result = m_jobSet.emplace(
+            type, name, ++m_lastJob, data.load(), func, m_cancelCallback);
         queueJob(*result.first, lock);
     }
     return true;
@@ -278,7 +261,7 @@ void
 JobQueue::rendezvous()
 {
     std::unique_lock<std::mutex> lock(m_mutex);
-    cv_.wait(lock, [&] { return m_processCount == 0 && m_jobSet.empty(); });
+    cv_.wait(lock, [this] { return m_processCount == 0 && m_jobSet.empty(); });
 }
 
 JobTypeData&
@@ -296,28 +279,31 @@ JobQueue::getJobTypeData(JobType type)
 }
 
 void
-JobQueue::onStop()
+JobQueue::stop()
 {
-    // onStop must be defined and empty here,
-    // otherwise the base class will do the wrong thing.
+    stopping_ = true;
+    using namespace std::chrono_literals;
+    jobCounter_.join("JobQueue", 1s, m_journal);
+    {
+        // After the JobCounter is joined, all jobs have finished executing
+        // (i.e. returned from `Job::doJob`) and no more are being accepted,
+        // but there may still be some threads between the return of
+        // `Job::doJob` and the return of `JobQueue::processTask`. That is why
+        // we must wait on the condition variable to make these assertions.
+        std::unique_lock<std::mutex> lock(m_mutex);
+        cv_.wait(
+            lock, [this] { return m_processCount == 0 && m_jobSet.empty(); });
+        assert(m_processCount == 0);
+        assert(m_jobSet.empty());
+        assert(nSuspend_ == 0);
+        stopped_ = true;
+    }
 }
 
-void
-JobQueue::checkStopped(std::lock_guard<std::mutex> const& lock)
+bool
+JobQueue::isStopped() const
 {
-    // We are stopped when all of the following are true:
-    //
-    //  1. A stop notification was received
-    //  2. All Stoppable children have stopped
-    //  3. There are no executing calls to processTask
-    //  4. There are no remaining Jobs in the job set
-    //  5. There are no suspended coroutines
-    //
-    if (isStopping() && areChildrenStopped() && (m_processCount == 0) &&
-        m_jobSet.empty() && nSuspend_ == 0)
-    {
-        stopped();
-    }
+    return stopped_;
 }
 
 void
@@ -437,13 +423,12 @@ JobQueue::processTask(int instance)
 
     {
         std::lock_guard lock(m_mutex);
-        // Job should be destroyed before calling checkStopped
+        // Job should be destroyed before stopping
         // otherwise destructors with side effects can access
         // parent objects that are already destroyed.
         finishJob(type);
         if (--m_processCount == 0 && m_jobSet.empty())
             cv_.notify_all();
-        checkStopped(lock);
     }
 
     // Note that when Job::~Job is called, the last reference
@@ -459,11 +444,4 @@ JobQueue::getJobLimit(JobType type)
     return j.limit();
 }
 
-void
-JobQueue::onChildrenStopped()
-{
-    std::lock_guard lock(m_mutex);
-    checkStopped(lock);
-}
-
 }  // namespace ripple