Rev 2564: Add test and fix for locking robustly when rename of directories doesn't act as a mutex (thank John) in http://sourcefrog.net/bzr/dlock

[Martin Pool]

At http://sourcefrog.net/bzr/dlock

------------------------------------------------------------
revno: 2564
revision-id: mbp at sourcefrog.net-20070628060630-n7ygr23xhjcnay9w
parent: mbp at sourcefrog.net-20070628060052-2st5avps6i86neat
committer: Martin Pool <mbp at sourcefrog.net>
branch nick: dlock
timestamp: Thu 2007-06-28 16:06:30 +1000
message:
  Add test and fix for locking robustly when rename of directories doesn't act as a mutex (thank John)
modified:
  bzrlib/lockdir.py              lockdir.py-20060220222025-98258adf27fbdda3
  bzrlib/tests/test_lockdir.py   test_lockdir.py-20060220222025-33d4221569a3d600
=== modified file 'bzrlib/lockdir.py'
--- a/bzrlib/lockdir.py	2007-06-27 09:22:40 +0000
+++ b/bzrlib/lockdir.py	2007-06-28 06:06:30 +0000
@@ -194,6 +194,7 @@
         instead.
 
         :return: The lock token.
+        :raises LockContention: if the lock is held by someone else.
         """
         if self._fake_read_lock:
             raise LockContention(self)
@@ -202,7 +203,7 @@
         try:
             self._trace("lock_write...")
             start_time = time.time()
-            tmpname = '%s/pending.%s.tmp' % (self.path, rand_chars(20))
+            tmpname = '%s/%s.tmp' % (self.path, rand_chars(10))
             try:
                 self.transport.mkdir(tmpname)
             except NoSuchFile:
@@ -210,7 +211,6 @@
                 # which is okay, it will be caught later and determined
                 # to be a LockContention.
                 self.create(mode=self._dir_modebits)
-                
                 # After creating the lock directory, try again
                 self.transport.mkdir(tmpname)
 
@@ -224,11 +224,20 @@
                                                 info_bytes)
 
             self.transport.rename(tmpname, self._held_dir)
+            # We must check we really got the lock, because Launchpad's sftp server at one
+            # time had a bug were the rename would successfully move the new
+            # directory into the existing directory, which was incorrect.
+            # It's possible some other servers or filesystems will have a
+            # similar bug allowing someone to think they got the lock when
+            # it's already held.
+            info = self.peek()
+            if info['nonce'] != self.nonce:
+                raise errors.LockError("%s: rename succeeded, "
+                    "but lock is still held by someone else" % (self,))
+            # we don't call confirm here because we don't want to set
+            # _lock_held til we're sure it's true, and because it's really a
+            # problem, not just regular contention, if this fails
             self._lock_held = True
-            # we used to do self.confirm() at this point, but it's really
-            # unnecessary, we have no substantial chance of having it broken
-            # just as it's acquired, and we believe that this lock design is
-            # safe on all platforms.
             # FIXME: we should remove the pending lock if we fail, 
             # https://bugs.launchpad.net/bzr/+bug/109169
         except errors.PermissionDenied:

=== modified file 'bzrlib/tests/test_lockdir.py'
--- a/bzrlib/tests/test_lockdir.py	2007-06-27 09:17:22 +0000
+++ b/bzrlib/tests/test_lockdir.py	2007-06-28 06:06:30 +0000
@@ -27,6 +27,7 @@
     errors,
     osutils,
     tests,
+    transport,
     )
 from bzrlib.errors import (
         LockBreakMismatch,
@@ -633,3 +634,17 @@
         ld2 = self.get_lock()
         t2 = ld2.lock_write(token)
         self.assertEqual(token, t2)
+
+    def test_lock_with_buggy_rename(self):
+        # test that lock acquisition handles servers which pretend they
+        # renamed correctly but that actually fail
+        t = transport.get_transport('brokenrename+' + self.get_url())
+        ld1 = LockDir(t, 'test_lock')
+        ld1.create()
+        ld1.attempt_lock()
+        ld2 = LockDir(t, 'test_lock')
+        # we should notice that we failed to lock, even though the transport
+        # has the wrong rename behaviour
+        e = self.assertRaises(errors.LockError, ld2.attempt_lock)
+        self.assertContainsRe(str(e),
+                "rename succeeded, but lock is still held by someone else")