LockSet: make acquire() fail faster on wrong locks

This patch makes acquire() first look up all the locks in the dict and then try
to acquire them later. The advantage is that if a lockname is already wrong
since the beginning we won't need to first queue and acquire other locks to
find this out.

Of course since there is no locking between the two steps a delete() could
still happen in between, but SharedLocks are safe in this regard and will just
make the .acquire() operation fail if this unfortunate condition happens.

Since the right way to check if an instance/node exists and make sure it won't
stop existing after that is acquiring its lock this improves the common case
(checking for an incorrect name) while not penalizing correctness, or
performance as would happen if we kept a lock for the whole process.

Reviewed-by: iustinp

diff --git a/lib/locking.py b/lib/locking.py
index 15c33e7..216c205 100644 (file)
--- a/lib/locking.py
+++ b/lib/locking.py
@@ -377,16 +377,23 @@ class LockSet:
     else:
       names.sort()
 
-    # Now names contains a sorted list of resources whose lock we want to
-    # acquire. In order to get them we loop on this (private) list and look
-    # them up in __lockdict. Since we have no lock held on lockdict we have no
-    # guarantees on their presence, and they may even disappear after we looked
-    # them up. This is fine though as .acquire() itself is safe and will alert
-    # us if the lock gets deleted.
-
+    acquire_list = []
+    # First we look the locks up on __lockdict. We have no way of being sure
+    # they will still be there after, but this makes it a lot faster should
+    # just one of them be the already wrong
     try:
       for lname in names:
         lock = self.__lockdict[lname] # raises KeyError if the lock is not there
+        acquire_list.append((lname, lock))
+    except (KeyError):
+      raise errors.LockError('non-existing lock in set (%s)' % lname)
+
+    # Now acquire_list contains a sorted list of resources and locks we want.
+    # In order to get them we loop on this (private) list and acquire() them.
+    # We gave no real guarantee they will still exist till this is done but
+    # .acquire() itself is safe and will alert us if the lock gets deleted.
+    try:
+      for (lname, lock) in acquire_list:
         lock.acquire(shared=shared) # raises LockError if the lock is deleted
         try:
           # now the lock cannot be deleted, we have it!
@@ -398,7 +405,7 @@ class LockSet:
           lock.release()
           raise
 
-    except (KeyError, errors.LockError):
+    except (errors.LockError):
       name_fail = lname
       for lname in self._list_owned():
         self.__lockdict[lname].release()