[MERGE] new locks branch

[Robert Collins]

On Wed, 2006-02-22 at 16:13 +1100, Martin Pool wrote:
> === added file 'bzrlib/lockdir.py'
> --- /dev/null   
> +++ bzrlib/lockdir.py   
> @@ -0,0 +1,334 @@
> +# Copyright (C) 2006 Canonical Ltd
> +
> +# This program is free software; you can redistribute it and/or
> modify
> +# it under the terms of the GNU General Public License as published
> by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write to the Free Software
> +# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
> 02111-1307  USA

It would be nice to standardise on either:
# comment
#
# continues

or
# comment

# continues

I find the former more clear that its a single comment rather than two.

> +"""lock file protecting a resource
> +
> +bzr objects are locked by the existence of a file with a particular
> name

perhaps 'persistent bzr object' ? Or 'bzr disk objects' ?
...
> +A lock is represented on disk by a directory of a particular name,
> +containing an information file.  Taking a lock is done by renaming a
> +temporary directory into place.  We use temporary directories because
> +for all known transports and filesystems we believe that exactly one
> +attempt to claim the lock will succeed and the others will fail.
> (Files
> +won't do because some filesystems or transports only have
> +rename-and-overwrite, making it hard to tell who won.)

The first paragraph and this one conflict on what the lock is
represented by ;)


> +The desired characteristics are:
> +
> +* Locks are not reentrant.  (That is, a client that tries to take a 
> +  lock it already holds may deadlock or fail.)
> +* Stale locks can be guessed at by a heuristic
> +* Lost locks can be broken by any client
> +* Failed lock operations leave little or no mess
> +* Deadlocks are avoided by having a timeout always in use, clients
> +  desiring indefinite waits can retry or set a silly big timeout.
> +
> +Storage formats use the locks, and also need to consider concurrency
> +issues underneath the lock.  A format may choose not to use a lock
> +at all for some operations.
> +
> +LockDirs always operate over a Transport.  The transport may be
> readonly, in
> +which case the lock can be queried but not acquired.
> +
> +Locks are identified by a path name, relative to a base transport.
> +
> +Calling code will typically want to make sure there is exactly one
> LockDir
> +object per actual lock on disk.  This module does nothing to prevent
> aliasing
> +and deadlocks will likely occur if the locks are aliased.
> +
> +In the future we may add a "freshen" method which can be called
> +by a lock holder to check that their lock has not been broken, and
> to 
> +update the timestamp within it.
> +
> +Example usage:
> +
> +>>> from bzrlib.transport.memory import MemoryTransport
> +>>> # typically will be obtained from a BzrDir, Branch, etc
> +>>> t = MemoryTransport()
> +>>> l = LockDir(t, 'sample-lock')
> +>>> l.wait_lock()
> +>>> # do something here
> +>>> l.unlock()

Perhaps the sample usage should be in 'bzrlib.doc.api.lockdir.txt' ?



> +# TODO: After renaming the directory, check the contents are what we
> +# expected.  It's possible that the rename failed but the transport
> lost
> +# the failure indication.

For some reason I thought you do this already ?

> +
> +class LockDir(object):
> +    """Write-lock guarding access to data.
> +    """

PEP8:
class LockDir(object):
    """Write-lock guarding access to data."""


> +    INFO_NAME = '/info'

Is this meant to be mutable and public? If not public may I suggest
   
       __INFO_NAME = '/info'

And if not mutable, accessing it via LockDir.__INFO_NAME rather than
self.__INFO_NAME.

We had a thread on the use of class members to initialize instance
members a while back :).


...
> +    def attempt_lock(self):
> +        """Take the lock; fail if it's already held

missing full stop I think.



> +    def unlock(self):

Docstring per favour ?


> +    def wait(self, timeout=20, poll=0.5):
> +        """Wait a certain period for a lock to be released.
> +        """
PEP8 - same line for the closing '"""'.



> +
> +"""Tests for LockDir"""
> +
> +from threading import Thread
> +import time
> +
> +from bzrlib.errors import (
> +        LockBreakMismatch,
> +        LockContention, LockError, UnlockableTransport,
> +        LockNotHeld, LockBroken
> +        )
> +from bzrlib.lockdir import LockDir
> +from bzrlib.tests import TestCaseInTempDir, TestCaseWithTransport

Is TestCaseInTempDir used here ?


> ...
> -                raise ReadOnlyError("can't upgrade to a write lock
> from %r" %
> -                                self._lock_mode)
> +                raise ReadOnlyError()

I think its useful to specify the lock path here, otherwise figuring out
*which* branch is incorrectly locked will be a biatch.



+1 with the above resolved.

Rob

-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060222/876b6aa5/attachment.pgp