D22182: Remove anchorClicked lock which causes deadlock

[Aaron Puchert]

aaronpuchert added a comment.


  In D22182#491716 <https://phabricator.kde.org/D22182#491716>, @mswan wrote:
  
  > I don't suspect our `..Locker` constructors will ever (i.e. not anytime prior to the heat death of the universe) take a different lock. For that reason I am partial to making the lock implicit wherever possible.
  
  
  That's a bold statement. ;)
  
  > Would we be able to change the constructor to default to `DUChain::lock()`? I'm not familiar with C++'s rules w.r.t. default function/constructor values. If we can't make it a function call, would we be able to make it a plain-old global variable?
  
  In principle we could have that as default argument, although that requires including `duchain.h` in `duchainlock.h`.
  
  But I would actually advise against this. I would either make the argument explicit (meaning there is no default), or always implicit (meaning there is no way to specify another lock). If we make it implicit, then we don't have to use `DUChain::lock()` in the annotation, because that effectively becomes an implementation detail and can use a mock object instead.
  
  > Regardless, I would be in favor of the mock lock approach because I like keeping verbosity down when it really doesn't suit an immediate need.
  
  I don't find it terribly verbose. Also there are other locks used in the du-chain code, and I think it doesn't hurt to be a little bit more explicit.
  
  >> That is currently not supported.
  > 
  > That sounds like it could be an interesting project. It looks like they are pretty close since they have this whole "TRY_ACQUIRE" annotation which isn't exactly what we're looking for but almost. If it could be instead predicated on, say, the return value of a public boolean method or instance variable post-invocation, that would be enough for our needs. I have not looked into how this works and what kinds of things it can check.
  
  It would require adding an additional attribute, which I'm hesitant to do, since we have a good workaround.
  
  > I assume it does a very basic check at the caller to see that the return value is //immediately// used and that the failed-to-lock branch doesn't require the lock. That is mere speculation.
  
  It doesn't work like that actually, the branch point can be considerably later. Basically we check if a branch condition correlates to the return value of a try-acquire call and if it does, we assumes the lock is acquired on that branch.
  
    void k() {
        bool hold  = mu.TryLock();
        bool hold2 = hold;
        if (b)
            hold = true;
        if (hold) {
            a = 8;    // warning: writing variable 'a' requires holding mutex 'mu' exclusively [-Wthread-safety-analysis]
        }
        if (hold2) {
            a = 8;    // No warning here.
            mu.Unlock();
        }
    }
  
  There are some deficits in the analysis of try-acquires, but that's mostly because it just isn't used very often.
  
  > Not sure that will be needed if one of us adds support for that slight augmentation to clang's existing TRY_ACQUIRE annotation. Currently whenever this timeout try-lock behaviour is performed, the `locked()` instance method is immediately invoked and usually leads to either returning or continuing with the rest of the function.
  
  I find the current behavior confusing actually. A constructor is meant to construct a "valid" object or fail (that's the idea of RAII), yet here it might not be valid and we always have to check with some function. This isn't very idiomatic and also error-prone. Getting rid of this pattern is a value in and of itself, independent of whether we want to pursue the analysis.
  
  >> Adopting means that we don't lock the mutex, we just take over an already locked mutex. (See e.g. the second constructor of a lock_guard <https://en.cppreference.com/w/cpp/thread/lock_guard/lock_guard>.) The annotation for that is `REQUIRES(...)` on the constructor.
  >> 
  >> Then you can do:
  >> 
  >>   if (!DUChain::lock().lockForRead())
  >>       return; // or error handling...
  >>   DUChain{Read,Write}Locker lock(DUChain::lock(), std::adopt_lock);
  > 
  > I am not sure I follow your example. Obviously we would just invoke `DUChain{Read,Write}Locker` instead of going through this sadistic exercise. It is unclear to me when we would actually want to adopt a lock in a `DUChain{Read,Write}Locker` constructor invocation.
  
  You are aware that the code this is meant to replace isn't just a constructor call? This is the pattern that we find right now:
  
    DUChain{Read,Write}Locker lock(DUChain::lock(), /*timeout*/ 100);
    if (!lock.locked())
        return; // or error handling...
  
  What do you find sadistic about my proposed replacement?
  
  > That said, adopting a lock seems plenty relevant to the many functions present in KDevelop that expect prior locking, e.g. methods beginning with `ENSURE_CAN_READ`.
  
  Adopting means the lock will be released upon leaving the function. That's not what we want there, we want an annotation on the function `REQUIRES_SHARED(...)`. Remember that the main purpose of scoped locks is to ensure release on all code paths. If you don't want to release anything, there is no reason for a scoped lock.

REPOSITORY
  R32 KDevelop

REVISION DETAIL
  https://phabricator.kde.org/D22182

To: mswan, #kdevelop
Cc: aaronpuchert, kdevelop-devel, hmitonneau, christiant, glebaccon, domson, antismap, iodelay, alexeymin, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20190707/57fe2e08/attachment.html>