RFC: Assert on probable deadlocks if the second lock isnt try_lock #5515

Just for travis

Since travis passes, I figured we might actually consider this...essentially, it asserts that we dont have deadlocks assuming: (a) no locking-conditions are based on data other than the current set of locked locks and (b) if we do a TRY_LOCK, and it fails, we will immediately bail out and just skip that section of code. Since DEBUG_LOCKORDER is pretty much travis-only, if it might crash in cases that regular bitcoind might not, I dont really mind.

Mostly, the DEBUG_LOCKORDER warnings arent even shown by travis anymore (they're just hidden behind the debug.log, so the only thing that flag does is check the lock assertions), so I figure we should have some semi-automated screeching if common-path code has lock inversions.

Concept ACK, but this makes the deadlock detection code kind of complex. I can't immediately see whether it's correct or wrong.

Hmmm....

Would this work instead? Modify sync.h CMutexLock::TryEnter to only call EnterCritical() if the lock is successfully acquired?

    bool TryEnter(const char* pszName, const char* pszFile, int nLine)
    {
        lock.try_lock();
        if (lock.owns_lock()
              EnterCritical(pszName, pszFile, nLine, (void*)(lock.mutex()), true);
        return lock.owns_lock();
    }

(or, in other words: just ignore TRY_LOCKS that don't acquire the lock)

@gavinandresen No, because often you see warnings because you have a lock(B) followed by a try_lock(A) and a lock(A) followed by a lock(B) at different times, you'll get a warning, but your suggestion would ignore it because you didnt actually see a deadlock.

Also, you cant just ignore try_locks entirely because it is very easy to create a deadlock with a try_lock.

@TheBlueMatt : maybe I'm being dense... but:

If we get: lock(B) then a successful (lock-acquired) try_lock(A), then the lockorders map will contain "B then A". If at any point in the future lock(A) is followed by lock(B), a warning will be triggered -- my suggestion doesn't require that there be an actual deadlock.

I'm not suggesting that try_lock be ignored entirely....

@gavinandresen Yes, and if a warning were triggered in that (beningn) case, then you'd see things crash on the new assert?

Haven't spent too much time thinking about this, but having (A, B) in one place and (B, try A) in another place can never cause a deadlock. I believe that the correct checking behaviour is to add any acquired lock to the table, but never do a deadlock check when entering a try_lock.

@sipa: Consider the case A, tryB, C and C, tryB, A. This is actually not a deadlock, but if you simply skip the deadlock check for B, you'll get an error.

IIRC, there was a similar case that I saw that prompted this pull.

@TheBlueMatt : sorry, I WAS just being dense.

I'm with @laanwj -- ACK on the concept, but I'm having trouble convincing myself the code is correct (I may just need more caffeine before thinking about it some more). It feels like there should be a simpler solution.

Interesting. I agree (a,try b,c),(c,try b,a)) is not a deadlock, but it is not something any code should be doing - it looks extremely fragile (remove a lock, and turn it into a deadlock). So I'd rather trigger deadlock warnings for it too.

IIRC we (used to) do something like that. We may not anymore, but some of the network processing stuff is a mess.

On April 8, 2015 3:46:03 PM GMT+01:00, Pieter Wuille notifications@github.com wrote:

Interesting. I agree (a,try b,c),(c,try b,a)) is not a deadlock, but it is not something any code should be doing - it looks extremely fragile (remove a lock, and turn it into a deadlock). So I'd rather trigger deadlock warnings for it too.

---

Reply to this email directly or view it on GitHub: #5515 (comment)

@sipa checked, turns out the deadlock detection code never called potential_deadlock_detected for TRY_LOCKs so we definitely still do require the code in this pull before we can assert on deadlocks. Also rebased and added a comment so that this is more readable.