branch locking mk2.

John A Meinel john at arbash-meinel.com
Mon Feb 13 15:40:19 GMT 2006


Martin Pool wrote:
> OK, an implementation of this is up here:
> 
>   http://people.ubuntu.com/~mbp/bzr.mbp.locks/
> 
> Sadly this does not pass the tests yet because some of them attempt to
> take locks that are already held - a problem which is normally hidden on
> unix.  (Making it consistent across platforms and transports is an
> advantage of doing it with lock directories, I think.)
> 
> If anyone would like to look over lockfile.py and test_lockfile.py
> (attached) that'd be great.
> 
> 

I have a couple inline comments. But one overall comment is why call it
LockFile? Since it is using directories to do the locking it seems misnamed.

...

>     def test_20_lock_peek(self):
>         """Peek at the state of a lock"""
>         t = self.get_transport()
>         lf1 = LockFile(t, 'test_lock')
>         lf1.attempt_lock()
>         # peeking at a held lock should always be true
>         info1 = lf1.peek()
>         self.assertEqual(set(info1.keys()),
>                          set(['user', 'nonce', 'hostname', 'pid', 'start_time']))
>         # peeking at another lock with the same name should also be true
>         info2 = LockFile(t, 'test_lock').peek()
>         self.assertEqual(info1, info2)
>         # locks which are never used should be false
>         self.assertEqual(LockFile(t, 'other_lock').peek(), None)

The comments here are incorrect. I'm guessing you used to have peek()
return True/False, but now it returns a dictionary.

...

>     def test_32_lock_wait_succeed(self):
>         """Succeed when waiting on a lock that gets released
> 
>         One thread holds on a lock and then releases it; another tries to lock it.
>         """
>         t = self.get_transport()
>         lf1 = LockFile(t, 'test_lock')
>         lf1.attempt_lock()
> 
>         def wait_and_unlock():
>             time.sleep(0.1)
>             lf1.unlock()
>         unlocker = Thread(target=wait_and_unlock)
>         unlocker.start()
>         try:
>             lf2 = LockFile(t, 'test_lock')
>             before = time.time()
>             lf2.wait_lock(timeout=0.4, poll=0.1)
>             after = time.time()
>             self.assertTrue(after - before <= 1.0)
>         finally:
>             unlocker.join()
> 
>     def test_33_wait(self):
>         """Succeed when waiting on a lock that gets released
> 
>         One thread holds on a lock and then releases it; another waits 
>         for it to be released
>         """
>         t = self.get_transport()
>         lf1 = LockFile(t, 'test_lock')
>         lf1.attempt_lock()
> 
>         def wait_and_unlock():
>             time.sleep(0.1)
>             lf1.unlock()
>         unlocker = Thread(target=wait_and_unlock)
>         unlocker.start()
>         try:
>             lf2 = LockFile(t, 'test_lock')
>             before = time.time()
>             lf2.wait(timeout=0.4, poll=0.1)
>             after = time.time()
>             self.assertTrue(after - before <= 1.0)
>         finally:
>             unlocker.join()
> 

test_32_lock_wait_succeed is identical to test_33_wait

I'm guessing you want to test a LockFile.wait() which fails.

...

...

>     def attempt_lock(self):
>         """Take the lock; fail if it's already held
>         
>         If you wish to block until the lock can be obtained, call wait_lock()
>         instead.
>         """
>         if self.transport.is_readonly():
>             raise UnlockableTransport(self.transport)
>         if not (hasattr(self.transport, 'has_atomic_rename')
>                 and self.transport.has_atomic_rename()):
>             raise UnlockableTransport(self.transport)

I thought we were avoiding hasattr, prefering to do:
fn = getattr(self.transport, 'has_atomic_rename', None)
if not (fn and fn()):
  rais Unlockable....

Though are we worried about a platform that doesn't have atomic rename?

>         try:
>             tmpname = '%s.pending.%s.tmp' % (self.path, rand_chars(20))
>             self.transport.mkdir(tmpname)
>             sio = StringIO()
>             self._prepare_info(sio)
>             sio.seek(0)
>             self.transport.put(tmpname + self.INFO_NAME, sio)
>             # FIXME: this turns into os.rename on posix, but into a fancy rename 
>             # on Windows that may overwrite existing directory trees.  
>             # NB: posix rename will overwrite empty directories, but not 
>             # non-empty directories.

Definitely needs to be fixed. Perhaps with a 'fancy=False'.

>             self.transport.move(tmpname, self.path)
>             self._lock_held = True
>             return
>         except FileExists, e:
>             pass
>         # fall through to here on contention
>         raise LockContention(self)
> 
>     def unlock(self):
>         if not self._lock_held:
>             raise LockNotHeld(self)
>         # rename before deleting, because we can't atomically remove the whole
>         # tree
>         tmpname = '%s.releasing.%s.tmp' % (self.path, rand_chars(20))
>         self.transport.move(self.path, tmpname)
>         self._lock_held = False
>         self.transport.delete(tmpname + self.INFO_NAME)
>         self.transport.rmdir(tmpname)
> 
>     def peek(self):
>         """Check if the lock is held by anyone.
>         
>         If it is held, this returns the lock info structure as a rio Stanza,
>         which contains some information about the current lock holder.
>         Otherwise returns None.
>         """
>         try:
>             info = self._parse_info(self.transport.get(self._info_path))
>             assert isinstance(info, Stanza), \
>                     "bad parse result %r" % info
>             return info.as_dict()
>         except NoSuchFile, e:
>             return None
> 
>     def _prepare_info(self, outf):
>         """Write information about a pending lock to a temporary file.
>         """
>         import socket
>         # XXX: is creating this here inefficient?
>         config = bzrlib.config.GlobalConfig()
>         s = Stanza(hostname=socket.gethostname(),
>                    pid=str(os.getpid()),
>                    start_time=str(int(time.time())),
>                    nonce=self.nonce,
>                    user=config.user_email(),

...
I can see why the test above is different, since you have a 'wait_lock'
function, versus a wait() function.

>     def wait_lock(self, timeout=20, poll=0.5):
>         """Wait a certain period for a lock.
> 
>         If the lock can be acquired within the bounded time, it
>         is taken and this returns.  Otherwise, LockContention
>         is raised.  Either way, this function should return within
>         approximately `timeout` seconds.  (It may be a bit more if
>         a transport operation takes a long time to complete.)
>         """
>         # XXX: the transport interface doesn't let us guard 
>         # against operations there taking a long time.
>         deadline = time.time() + timeout
>         while True:
>             try:
>                 self.attempt_lock()
>                 return
>             except LockContention:
>                 pass
>             if time.time() + poll < deadline:
>                 time.sleep(poll)
>             else:
>                 raise LockContention(self)
> 
>     def wait(self, timeout=20, poll=0.5):
>         """Wait a certain period for a lock to be released.
>         """
>         # XXX: the transport interface doesn't let us guard 
>         # against operations there taking a long time.
>         deadline = time.time() + timeout
>         while True:
>             if self.peek():
>                 return
>             if time.time() + poll < deadline:
>                 time.sleep(poll)
>             else:
>                 raise LockContention(self)

I'm not sure what you are doing with wait(). Because 'self.peek()'
returns information if the lock is held. Which means you return right
away if the lock is held. Maybe you want 'if not self.peek(): return'.

Also, you have no api or tests for breaking a lock that is held by
someone else.

But the basics look pretty good here.

John
=:->


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 249 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060213/fad99668/attachment.pgp 


More information about the bazaar mailing list