D22182: Remove anchorClicked lock which causes deadlock

[Michael Swan]

mswan added a comment.


  In D22182#491022 <https://phabricator.kde.org/D22182#491022>, @aaronpuchert wrote:
  
  > If we make the lock explicit, we can just annotate the constructor with `ACQUIRE[_SHARED](duChainLock)` (using the macros from the docs).
  
  
  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, but even if by some chance it eventually did, i.e. if someone at some point in the future wants to add support for finer-grained locking on `DUChain`, the fairly simple work of changing the locker constructors to more or less the way they are now would be trivial work, especially compared to the source code studying that they are going to have to do to determine that their finer grained locking is actually safe and efficient. Also, the practical difference between "hiding" our global mutex behind `DUChain::lock()` and just presenting a global mutex, say `g_DUChainLock`, is lost on me. I remember hearing as a young computer science student that "global variables are just bad" and that singletons somehow are a cure-all for that global state itch. Practically speaking it seems to just add overhead and makes things feel more together, i.e. `DUChain::lock()` //feels// more official than `g_DUChainLock`. I digress.
  
  > When we call it with `DUChain::lock()`, Clang assumes that whatever this function returns is now locked, so if something else is annotated with `REQUIRES(DUChain::lock())`, it should be able to figure out that this is fine.
  
  You are right. I actually watched a talk on the first iteration of Clang Thread Safety which confirms this point, i.e. https://www.youtube.com/watch?v=5Xx0zktqSs4&t=15m46s.
  
  > If we always make the lock implicit, we could use a “mock lock”. This would be a global dummy object that we annotate as mutex, and that we claim is acquired by `DUChain{Read,Write}Locker`. That this isn't actually a lock is irrelevant for the Analysis.
  
  This is actually what I initially had in mind.
  
  > Both approaches should be workable in my opinion. The former has the advantage that it would generalize well to multiple locks, should the need ever arise. The latter means less code to write.
  > 
  > I'm slightly in favor of the former approach, because we wouldn't lose the ability to change things later, and because it would be less confusing.
  
  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? 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.
  
  > Sometimes scoped locks are passed around as parameters.
  
  I have also noticed this in what I think is its singular incarnation, namely `DUChainPrivate::storeAllInformation` which is part of the fairly complex `DUChainPrivate::doMoreCleanup` procedure. In this case, it is unsurprisingly referring to `DUChain::lock()` and so should be trivial to support lock adoption by this method, regardless of which approach we decide on.
  
  > The constructor of the scoped locks is actually a try-lock.
  
  That is a good point and I think we would be better off making different constructors. One that accepts a timeout, one that doesn't. The former does not take any default values so it is not ambiguous.
  
  > 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. 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.
  
  > Perhaps we can rewrite the few places with timeout with adopting as well.
  
  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.
  
  > 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. 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`.

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/8f10ed29/attachment.html>