Cleanup usage of ScopedUnlock

src/ripple/app/consensus/RCLValidations.h

@@ -21,7 +21,6 @@
 #define RIPPLE_APP_CONSENSUSS_VALIDATIONS_H_INCLUDED
 
 #include <ripple/app/ledger/Ledger.h>
-#include <ripple/basics/ScopedLock.h>
 #include <ripple/consensus/Validations.h>
 #include <ripple/protocol/Protocol.h>
 #include <ripple/protocol/RippleLedgerHash.h>
@@ -222,8 +221,6 @@ public:
     }
 
 private:
-    using ScopedUnlockType = GenericScopedUnlock<Mutex>;
-
     Application& app_;
     beast::Journal j_;
 };

src/ripple/app/ledger/LedgerMaster.h

@@ -31,17 +31,16 @@
 #include <ripple/app/misc/CanonicalTXSet.h>
 #include <ripple/basics/chrono.h>
 #include <ripple/basics/RangeSet.h>
-#include <ripple/basics/ScopedLock.h>
 #include <ripple/basics/StringUtilities.h>
+#include <ripple/beast/insight/Collector.h>
+#include <ripple/beast/utility/PropertyStream.h>
+#include <ripple/core/Stoppable.h>
+#include <ripple/protocol/messages.h>
+#include <ripple/protocol/Protocol.h>
 #include <ripple/protocol/RippleLedgerHash.h>
 #include <ripple/protocol/STValidation.h>
-#include <ripple/beast/insight/Collector.h>
-#include <ripple/core/Stoppable.h>
-#include <ripple/protocol/Protocol.h>
-#include <ripple/beast/utility/PropertyStream.h>
-#include <mutex>
 
-#include <ripple/protocol/messages.h>
+#include <mutex>
 
 namespace ripple {
 
@@ -256,8 +255,6 @@ public:
     }
 
 private:
-    using ScopedUnlockType = GenericScopedUnlock <std::recursive_mutex>;
-
     void setValidLedger(
         std::shared_ptr<Ledger const> const& l);
     void setPubLedger(
@@ -278,10 +275,11 @@ private:
     void fetchForHistory(
         std::uint32_t missing,
         bool& progress,
-        InboundLedger::Reason reason);
+        InboundLedger::Reason reason,
+        std::unique_lock<std::recursive_mutex>&);
     // Try to publish ledgers, acquire missing ledgers.  Always called with
     // m_mutex locked.  The passed lock is a reminder to callers.
-    void doAdvance(std::lock_guard<std::recursive_mutex>&);
+    void doAdvance(std::unique_lock<std::recursive_mutex>&);
     bool shouldAcquire(
         std::uint32_t const currentLedger,
         std::uint32_t const ledgerHistory,
@@ -289,13 +287,13 @@ private:
         std::uint32_t const candidateLedger) const;
 
     std::vector<std::shared_ptr<Ledger const>>
-    findNewLedgersToPublish();
+    findNewLedgersToPublish(std::unique_lock<std::recursive_mutex>&);
 
     void updatePaths(Job& job);
 
     // Returns true if work started.  Always called with m_mutex locked.
     // The passed lock is a reminder to callers.
-    bool newPFWork(const char *name, std::lock_guard<std::recursive_mutex>&);
+    bool newPFWork(const char *name, std::unique_lock<std::recursive_mutex>&);
 
 private:
     Application& app_;

src/ripple/app/ledger/impl/LedgerMaster.cpp

@@ -51,16 +51,93 @@
 
 namespace ripple {
 
-using namespace std::chrono_literals;
+namespace {
+
+//==============================================================================
+/**
+    Automatically unlocks and re-locks a unique_lock object.
+
+    This is the reverse of a std::unique_lock object - instead of locking the mutex
+    for the lifetime of this object, it unlocks it.
+
+    Make sure you don't try to unlock mutexes that aren't actually locked!
+
+    This is essentially a less-versatile boost::reverse_lock.
+
+    e.g. @code
+
+    std::mutex mut;
+
+    for (;;)
+    {
+        std::unique_lock myScopedLock{mut};
+        // mut is now locked
+
+        ... do some stuff with it locked ..
+
+        while (xyz)
+        {
+            ... do some stuff with it locked ..
+
+            ScopedUnlock unlocker{myScopedLock};
+
+            // mut is now unlocked for the remainder of this block,
+            // and re-locked at the end.
+
+            ...do some stuff with it unlocked ...
+        }  // mut gets locked here.
+
+    }  // mut gets unlocked here
+    @endcode
+*/
+template <class MutexType>
+class ScopedUnlock
+{
+    std::unique_lock<MutexType>& lock_;
+public:
+    /** Creates a ScopedUnlock.
+
+        As soon as it is created, this will unlock the unique_lock, and
+        when the ScopedLock object is deleted, the unique_lock will
+        be re-locked.
+
+        Make sure this object is created and deleted by the same thread,
+        otherwise there are no guarantees what will happen! Best just to use it
+        as a local stack object, rather than creating on the heap.
+    */
+    explicit ScopedUnlock (std::unique_lock<MutexType>& lock)
+        : lock_ (lock)
+    {
+        assert(lock_.owns_lock());
+        lock_.unlock();
+    }
+
+    ScopedUnlock (ScopedUnlock const&) = delete;
+    ScopedUnlock& operator= (ScopedUnlock const&) = delete;
+
+    /** Destructor.
+
+        The unique_lock will be locked after the destructor is called.
+
+        Make sure this object is created and deleted by the same thread,
+        otherwise there are no guarantees what will happen!
+    */
+    ~ScopedUnlock() noexcept(false)
+    {
+        lock_.lock();
+    }
+};
+
+}
 
 // Don't catch up more than 100 ledgers (cannot exceed 256)
-#define MAX_LEDGER_GAP          100
+static constexpr int MAX_LEDGER_GAP {100};
 
 // Don't acquire history if ledger is too old
-auto constexpr MAX_LEDGER_AGE_ACQUIRE = 1min;
+static constexpr std::chrono::minutes MAX_LEDGER_AGE_ACQUIRE {1};
 
 // Don't acquire history if write load is too high
-auto constexpr MAX_WRITE_LOAD_ACQUIRE = 8192;
+static constexpr int MAX_WRITE_LOAD_ACQUIRE {8192};
 
 LedgerMaster::LedgerMaster (Application& app, Stopwatch& stopwatch,
     Stoppable& parent,
@@ -76,7 +153,7 @@ LedgerMaster::LedgerMaster (Application& app, Stopwatch& stopwatch,
         app_.config().FETCH_DEPTH))
     , ledger_history_ (app_.config().LEDGER_HISTORY)
     , ledger_fetch_size_ (app_.config().getSize (siLedgerFetch))
-    , fetch_packs_ ("FetchPack", 65536, 45s, stopwatch,
+    , fetch_packs_ ("FetchPack", 65536, std::chrono::seconds {45}, stopwatch,
         app_.journal("TaggedCache"))
 {
 }
@@ -124,6 +201,7 @@ LedgerMaster::isCompatible (
 std::chrono::seconds
 LedgerMaster::getPublishedLedgerAge()
 {
+    using namespace std::chrono_literals;
     std::chrono::seconds pubClose{mPubLedgerClose.load()};
     if (pubClose == 0s)
     {
@@ -142,6 +220,7 @@ LedgerMaster::getPublishedLedgerAge()
 std::chrono::seconds
 LedgerMaster::getValidatedLedgerAge()
 {
+    using namespace std::chrono_literals;
     std::chrono::seconds valClose{mValidLedgerSign.load()};
     if (valClose == 0s)
     {
@@ -160,6 +239,7 @@ LedgerMaster::getValidatedLedgerAge()
 bool
 LedgerMaster::isCaughtUp(std::string& reason)
 {
+    using namespace std::chrono_literals;
     if (getPublishedLedgerAge() > 3min)
     {
         reason = "No recently-published ledger";
@@ -281,6 +361,7 @@ LedgerMaster::canBeCurrent (std::shared_ptr<Ledger const> const& ledger)
     auto closeTime = app_.timeKeeper().closeTime();
     auto ledgerClose = ledger->info().parentCloseTime;
 
+    using namespace std::chrono_literals;
     if ((validLedger || (ledger->info().seq > 10)) &&
         ((std::max (closeTime, ledgerClose) - std::min (closeTime, ledgerClose))
             > 5min))
@@ -1004,7 +1085,7 @@ LedgerMaster::consensusBuilt(
 void
 LedgerMaster::advanceThread()
 {
-    std::lock_guard sl (m_mutex);
+    std::unique_lock sl (m_mutex);
     assert (!mValidLedger.empty () && mAdvanceThread);
 
     JLOG (m_journal.trace()) << "advanceThread<";
@@ -1045,7 +1126,7 @@ LedgerMaster::getLedgerHashForHistory(
 }
 
 std::vector<std::shared_ptr<Ledger const>>
-LedgerMaster::findNewLedgersToPublish ()
+LedgerMaster::findNewLedgersToPublish(std::unique_lock<std::recursive_mutex>& sl)
 {
     std::vector<std::shared_ptr<Ledger const>> ret;
 
@@ -1093,7 +1174,7 @@ LedgerMaster::findNewLedgersToPublish ()
     auto valLedger = mValidLedger.get ();
     std::uint32_t valSeq = valLedger->info().seq;
 
-    ScopedUnlockType sul(m_mutex);
+    ScopedUnlock sul{sl};
     try
     {
         for (std::uint32_t seq = pubSeq; seq <= valSeq; ++seq)
@@ -1286,7 +1367,7 @@ LedgerMaster::updatePaths (Job& job)
 bool
 LedgerMaster::newPathRequest ()
 {
-    std::lock_guard ml (m_mutex);
+    std::unique_lock ml (m_mutex);
     mPathFindNewRequest = newPFWork("pf:newRequest", ml);
     return mPathFindNewRequest;
 }
@@ -1305,7 +1386,7 @@ LedgerMaster::isNewPathRequest ()
 bool
 LedgerMaster::newOrderBookDB ()
 {
-    std::lock_guard ml (m_mutex);
+    std::unique_lock ml (m_mutex);
     mPathLedger.reset();
 
     return newPFWork("pf:newOBDB", ml);
@@ -1314,7 +1395,7 @@ LedgerMaster::newOrderBookDB ()
 /** A thread needs to be dispatched to handle pathfinding work of some kind.
 */
 bool
-LedgerMaster::newPFWork (const char *name, std::lock_guard<std::recursive_mutex>&)
+LedgerMaster::newPFWork (const char *name, std::unique_lock<std::recursive_mutex>&)
 {
     if (mPathFindThread < 2)
     {
@@ -1617,9 +1698,10 @@ void
 LedgerMaster::fetchForHistory(
     std::uint32_t missing,
     bool& progress,
-    InboundLedger::Reason reason)
+    InboundLedger::Reason reason,
+    std::unique_lock<std::recursive_mutex>& sl)
 {
-    ScopedUnlockType sl(m_mutex);
+    ScopedUnlock sul{sl};
     if (auto hash = getLedgerHashForHistory(missing, reason))
     {
         assert(hash->isNonZero());
@@ -1734,14 +1816,14 @@ LedgerMaster::fetchForHistory(
 }
 
 // Try to publish ledgers, acquire missing ledgers
-void LedgerMaster::doAdvance (std::lock_guard<std::recursive_mutex>& sl)
+void LedgerMaster::doAdvance (std::unique_lock<std::recursive_mutex>& sl)
 {
     do
     {
         mAdvanceWork = false; // If there's work to do, we'll make progress
         bool progress = false;
 
-        auto const pubLedgers = findNewLedgersToPublish ();
+        auto const pubLedgers = findNewLedgersToPublish (sl);
         if (pubLedgers.empty())
         {
             if (!standalone_ && !app_.getFeeTrack().isLoadedLocal() &&
@@ -1784,7 +1866,7 @@ void LedgerMaster::doAdvance (std::lock_guard<std::recursive_mutex>& sl)
                 }
                 if(missing)
                 {
-                    fetchForHistory(*missing, progress, reason);
+                    fetchForHistory(*missing, progress, reason, sl);
                     if (mValidLedgerSeq != mPubLedgerSeq)
                     {
                         JLOG (m_journal.debug()) <<
@@ -1809,7 +1891,7 @@ void LedgerMaster::doAdvance (std::lock_guard<std::recursive_mutex>& sl)
             for(auto ledger : pubLedgers)
             {
                 {
-                    ScopedUnlockType sul (m_mutex);
+                    ScopedUnlock sul{sl};
                     JLOG (m_journal.debug()) <<
                         "tryAdvance publishing seq " << ledger->info().seq;
                     setFullLedger(ledger, true, true);
@@ -1818,7 +1900,7 @@ void LedgerMaster::doAdvance (std::lock_guard<std::recursive_mutex>& sl)
                 setPubLedger(ledger);
 
                 {
-                    ScopedUnlockType sul(m_mutex);
+                    ScopedUnlock sul{sl};
                     app_.getOPs().pubLedger(ledger);
                 }
             }
@@ -1877,6 +1959,7 @@ LedgerMaster::makeFetchPack (
     uint256 haveLedgerHash,
      UptimeClock::time_point uptime)
 {
+    using namespace std::chrono_literals;
     if (UptimeClock::now() > uptime + 1s)
     {
         JLOG(m_journal.info()) << "Fetch pack request got stale";

src/ripple/basics/ScopedLock.h

@@ -1,105 +0,0 @@
-//------------------------------------------------------------------------------
-/*
-    This file is part of rippled: https://github.com/ripple/rippled
-    Copyright (c) 2012, 2013 Ripple Labs Inc.
-
-    Portions of this file are from JUCE.
-    Copyright (c) 2013 - Raw Material Software Ltd.
-    Please visit http://www.juce.com
-
-    Permission to use, copy, modify, and/or distribute this software for any
-    purpose  with  or without fee is hereby granted, provided that the above
-    copyright notice and this permission notice appear in all copies.
-
-    THE  SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
-    WITH  REGARD  TO  THIS  SOFTWARE  INCLUDING  ALL  IMPLIED  WARRANTIES  OF
-    MERCHANTABILITY  AND  FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
-    ANY  SPECIAL ,  DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
-    WHATSOEVER  RESULTING  FROM  LOSS  OF USE, DATA OR PROFITS, WHETHER IN AN
-    ACTION  OF  CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
-    OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
-*/
-//==============================================================================
-
-#ifndef RIPPLE_BASICS_SCOPEDLOCK_H_INCLUDED
-#define RIPPLE_BASICS_SCOPEDLOCK_H_INCLUDED
-
-namespace ripple
-{
-
-//==============================================================================
-/**
-    Automatically unlocks and re-locks a mutex object.
-
-    This is the reverse of a std::lock_guard object - instead of locking the mutex
-    for the lifetime of this object, it unlocks it.
-
-    Make sure you don't try to unlock mutexes that aren't actually locked!
-
-    e.g. @code
-
-    std::mutex mut;
-
-    for (;;)
-    {
-        std::lock_guard myScopedLock{mut};
-        // mut is now locked
-
-        ... do some stuff with it locked ..
-
-        while (xyz)
-        {
-            ... do some stuff with it locked ..
-
-            GenericScopedUnlock<std::mutex> unlocker{mut};
-
-            // mut is now unlocked for the remainder of this block,
-            // and re-locked at the end.
-
-            ...do some stuff with it unlocked ...
-        }  // mut gets locked here.
-
-    }  // mut gets unlocked here
-    @endcode
-
-*/
-template <class MutexType>
-class GenericScopedUnlock
-{
-    MutexType& lock_;
-public:
-    /** Creates a GenericScopedUnlock.
-
-        As soon as it is created, this will unlock the CriticalSection, and
-        when the ScopedLock object is deleted, the CriticalSection will
-        be re-locked.
-
-        Make sure this object is created and deleted by the same thread,
-        otherwise there are no guarantees what will happen! Best just to use it
-        as a local stack object, rather than creating one with the new() operator.
-    */
-    explicit GenericScopedUnlock (MutexType& lock) noexcept
-        : lock_ (lock)
-    {
-        lock.unlock();
-    }
-
-    GenericScopedUnlock (GenericScopedUnlock const&) = delete;
-    GenericScopedUnlock& operator= (GenericScopedUnlock const&) = delete;
-
-    /** Destructor.
-
-        The CriticalSection will be unlocked when the destructor is called.
-
-        Make sure this object is created and deleted by the same thread,
-        otherwise there are no guarantees what will happen!
-    */
-    ~GenericScopedUnlock() noexcept(false)
-    {
-        lock_.lock();
-    }
-};
-
-} // ripple
-#endif
-