D22182: Remove anchorClicked lock which causes deadlock

[Michael Swan]

mswan added a comment.


  In D22182#492046 <https://phabricator.kde.org/D22182#492046>, @aaronpuchert wrote:
  
  > 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.
  
  
  I agree, it doesn't really hurt readability but it just seems like the same constructor argument on repeat through the whole code seems like a blemish to me. It is really a triviality of syntax so if there are practical reasons to make it explicit then that is obviously what  should be done.
  
  >> 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.
  
  The workaround being that we eliminate the try-lock behaviour in our locker constructors and move that timeout behaviour elsewhere?
  
  >> 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.
  
  Interesting. Good to know.
  
  >> 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.
  
  C++ idioms are relatively foreign to me so I am not sure what a constructor like this "should" do but I see your point. We could have those //very few// cases where timeout is used invoke `DUChainLock` functions directly instead of us having to worry about that timeout try-lock weirdness in our locker implementation. I know of two places where there is a timeout being passed to the `DUChainReadLocker` which is not necessarily cause for changing the semantics of its 300+ other invocations.
  
  > 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?
  
  I see, that makes more sense. So, your proposal is probably what should be done in those few places where we actually use this timeout functionality. I mean, we are currently talking about like five callsites between both the read and write lockers so I'm not too worried about how this timeout code looks so long as it works. Ideally we could also test those callsites with clang thread safety but that too is not as essential as the hundreds of non-timeout invocations.
  
  >> 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.
  
  I see, I did not realize that.
  
  > 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.
  
  Err, so now I'm confused again. So, does `REQUIRES_SHARED(...)` require that this share of the lock is no longer held upon completion of this function? If so then is it possible to annotate a function in a semantically identical way to `ENSURE_CAN_READ` which implies that a read (i.e. shared) lock is held before and (possibly) after the function evaluates, or does that kind of behaviour break this whole thing? I imagine then we would have to be much more fine-grained with our usage of DUChain locks and move these lockers closer to the actual mutations to DUChain.

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/20190708/205d8a72/attachment-0001.html>