Merge #9674: Always enforce strict lock ordering (try or not)

618ee92 Further-enforce lockordering by enforcing directly after TRY_LOCKs (Matt Corallo)
2a962d4 Fixup style a bit by moving { to the same line as if statements (Matt Corallo)
8465631 Always enforce lock strict lock ordering (try or not) (Matt Corallo)
fd13eca Lock cs_vSend and cs_inventory in a consistent order even in TRY (Matt Corallo)

Author: laanwj

src/net.cpp

@@ -1093,20 +1093,18 @@ void CConnman::ThreadSocketHandler()
             BOOST_FOREACH(CNode* pnode, vNodesDisconnectedCopy)
             {
                 // wait until threads are done using it
-                if (pnode->GetRefCount() <= 0)
-                {
+                if (pnode->GetRefCount() <= 0) {
                     bool fDelete = false;
                     {
-                        TRY_LOCK(pnode->cs_vSend, lockSend);
-                        if (lockSend)
-                        {
-                                TRY_LOCK(pnode->cs_inventory, lockInv);
-                                if (lockInv)
-                                    fDelete = true;
+                        TRY_LOCK(pnode->cs_inventory, lockInv);
+                        if (lockInv) {
+                            TRY_LOCK(pnode->cs_vSend, lockSend);
+                            if (lockSend) {
+                                fDelete = true;
+                            }
                         }
                     }
-                    if (fDelete)
-                    {
+                    if (fDelete) {
                         vNodesDisconnected.remove(pnode);
                         DeleteNode(pnode);
                     }

src/sync.cpp

@@ -77,52 +77,28 @@ boost::thread_specific_ptr<LockStack> lockstack;
 
 static void potential_deadlock_detected(const std::pair<void*, void*>& mismatch, const LockStack& s1, const LockStack& s2)
 {
-    // We attempt to not assert on probably-not deadlocks by assuming that
-    // a try lock will immediately have otherwise bailed if it had
-    // failed to get the lock
-    // We do this by, for the locks which triggered the potential deadlock,
-    // in either lockorder, checking that the second of the two which is locked
-    // is only a TRY_LOCK, ignoring locks if they are reentrant.
-    bool firstLocked = false;
-    bool secondLocked = false;
-    bool onlyMaybeDeadlock = false;
-
     LogPrintf("POTENTIAL DEADLOCK DETECTED\n");
     LogPrintf("Previous lock order was:\n");
     BOOST_FOREACH (const PAIRTYPE(void*, CLockLocation) & i, s2) {
         if (i.first == mismatch.first) {
             LogPrintf(" (1)");
-            if (!firstLocked && secondLocked && i.second.fTry)
-                onlyMaybeDeadlock = true;
-            firstLocked = true;
         }
         if (i.first == mismatch.second) {
             LogPrintf(" (2)");
-            if (!secondLocked && firstLocked && i.second.fTry)
-                onlyMaybeDeadlock = true;
-            secondLocked = true;
         }
         LogPrintf(" %s\n", i.second.ToString());
     }
-    firstLocked = false;
-    secondLocked = false;
     LogPrintf("Current lock order is:\n");
     BOOST_FOREACH (const PAIRTYPE(void*, CLockLocation) & i, s1) {
         if (i.first == mismatch.first) {
             LogPrintf(" (1)");
-            if (!firstLocked && secondLocked && i.second.fTry)
-                onlyMaybeDeadlock = true;
-            firstLocked = true;
         }
         if (i.first == mismatch.second) {
             LogPrintf(" (2)");
-            if (!secondLocked && firstLocked && i.second.fTry)
-                onlyMaybeDeadlock = true;
-            secondLocked = true;
         }
         LogPrintf(" %s\n", i.second.ToString());
     }
-    assert(onlyMaybeDeadlock);
+    assert(false);
 }
 
 static void push_lock(void* c, const CLockLocation& locklocation, bool fTry)
@@ -134,21 +110,19 @@ static void push_lock(void* c, const CLockLocation& locklocation, bool fTry)
 
     (*lockstack).push_back(std::make_pair(c, locklocation));
 
-    if (!fTry) {
-        BOOST_FOREACH (const PAIRTYPE(void*, CLockLocation) & i, (*lockstack)) {
-            if (i.first == c)
-                break;
+    BOOST_FOREACH (const PAIRTYPE(void*, CLockLocation) & i, (*lockstack)) {
+        if (i.first == c)
+            break;
 
-            std::pair<void*, void*> p1 = std::make_pair(i.first, c);
-            if (lockdata.lockorders.count(p1))
-                continue;
-            lockdata.lockorders[p1] = (*lockstack);
+        std::pair<void*, void*> p1 = std::make_pair(i.first, c);
+        if (lockdata.lockorders.count(p1))
+            continue;
+        lockdata.lockorders[p1] = (*lockstack);
 
-            std::pair<void*, void*> p2 = std::make_pair(c, i.first);
-            lockdata.invlockorders.insert(p2);
-            if (lockdata.lockorders.count(p2))
-                potential_deadlock_detected(p1, lockdata.lockorders[p2], lockdata.lockorders[p1]);
-        }
+        std::pair<void*, void*> p2 = std::make_pair(c, i.first);
+        lockdata.invlockorders.insert(p2);
+        if (lockdata.lockorders.count(p2))
+            potential_deadlock_detected(p1, lockdata.lockorders[p2], lockdata.lockorders[p1]);
     }
 }