[MERGE] Cleanup and test Lock objects

[Alexander Belchenko]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

John Arbash Meinel пишет:
> John Arbash Meinel wrote:
>> Martin Pool wrote:
>>>
>>> I agree with Alexander that we should perhaps have per-implementation
>>> tests.
>
> Well to give myself some more experience with writing adapted tests, I
> have went ahead and done this in the attached patch.

Many thanks.
I'm also finally understand how adapted tests is created and emm... adapted.

(Why we don't have any manual for this black black adaptor magic
in our HACKING guide?)

> It is a large enough change, that I would like someone to look it over,
> even though the other one was +1.

I ran 'selftest per_lock' on Python 2.4 and 2.5. Works OK.
I read the code and does not found any major warnings.

Some minor questions below.

> === added file bzrlib/tests/per_lock/test_lock.py // file-id:test_lock.py-20070
> ... 313190612-mfpoa7t8kvrgrhj2-1 // last-changed:john at arbash-meinel.com-2007031
> ... 4201552-bjtfua57456dviep
> --- /dev/null
> +++ bzrlib/tests/per_lock/test_lock.py
[...]
> +class TestLock(TestCaseWithLock):
> +
[...]
> +
> +    def test_create_if_needed_write(self):
> +        """We will create the file if it doesn't exist yet."""
> +        a_lock = self.write_lock('other-file')
> +        self.addCleanup(a_lock.unlock)
> +        txt = a_lock.f.read()
> +        self.assertEqual('', txt)
> +        a_lock.f.write('foo\n')
> +        a_lock.f.seek(0)
> +        txt = a_lock.f.read()
> +        self.assertEqual('foo\n', txt)

^-- I remember your warning about switch from read to write
and have pedantic question: between txt = a_lock.f.read()
and a_lock.f.write('foo\n') does we need to insert seek?
I think it works because file has zero length, but
may be for purity reason we need to add seek, even if it no-op?

> +    def test_readonly_file(self):
> +        """If the file is readonly, we can take a read lock.
> +
> +        But we shouldn't be able to take a write lock.
> +        """
> +        osutils.make_readonly('a-file')
> +        # Make sure the file is read-only (on all platforms)
> +        self.assertRaises(IOError, open, 'a-file', 'rb+')
> +        a_lock = self.read_lock('a-file')
> +        a_lock.unlock()
> +
> +        # TODO: jam 20070313 This should be a specific subclass
> +        self.assertRaises(errors.ReadOnlyLockError, self.write_lock, 'a-file')

^-- What you mean here in your TODO?

v-- IIUC these tests disabled because of Linux fcntl (mis)behaviour?

> +    def _disabled_test_read_then_write_excludes(self):
> +        """If a file is read-locked, taking out a write lock should fail."""
> +        a_lock = self.read_lock('a-file')
> +        self.addCleanup(a_lock.unlock)
> +        # Taking out a lock on a locked file should raise LockContention
> +        self.assertRaises(errors.LockContention, self.write_lock, 'a-file')
> +
> +    def _disabled_test_write_then_read_excludes(self):
> +        """If a file is write-locked, taking out a read lock should fail.
> +
> +        The file is exclusively owned by the write lock, so we shouldn't be
> +        able to take out a shared read lock.
> +        """
> +        a_lock = self.write_lock('a-file')
> +        self.addCleanup(a_lock.unlock)
> +        # Taking out a lock on a locked file should raise LockContention
> +        self.assertRaises(errors.LockContention, self.read_lock, 'a-file')

> === modified file bzrlib/lock.py
> --- bzrlib/lock.py
> +++ bzrlib/lock.py
> @@ -1,4 +1,4 @@
[...]
> +if have_fcntl:
> +    LOCK_SH = fcntl.LOCK_SH
> +    LOCK_NB = fcntl.LOCK_NB
> +    lock_EX = fcntl.LOCK_EX
> +

v-- You introduce open_locks dictionary, but I did not found is it used anywhere?
I guess, no. So probably better to remove it, because it absent in win32-locks.

> @@ -124,8 +149,10 @@
>  
>      class _fcntl_ReadLock(_fcntl_FileLock):
>  
> +        open_locks = {}
> +
>          def __init__(self, filename):
> -            # standard IO errors get exposed directly.
> +            super(_fcntl_ReadLock, self).__init__()
>              self._open(filename, 'rb')
>              try:
>                  # LOCK_NB will cause IOError to be raised if we can't grab a


> +if have_ctypes:
> +    # These constants were copied from the win32con.py module.
> +    LOCKFILE_FAIL_IMMEDIATELY = 1
> +    LOCKFILE_EXCLUSIVE_LOCK = 2
> +    # Constant taken from winerror.py module
> +    ERROR_LOCK_VIOLATION = 33
> +
> +    LOCK_SH = 0
> +    LOCK_EX = LOCKFILE_EXCLUSIVE_LOCK
> +    LOCK_NB = LOCKFILE_FAIL_IMMEDIATELY
> +    _LockFileEx = ctypes.windll.kernel32.LockFileEx
> +    _UnlockFileEx = ctypes.windll.kernel32.UnlockFileEx
> +    _GetLastError = ctypes.windll.kernel32.GetLastError
> +
> +    ### Define the OVERLAPPED structure.
> +    #   http://msdn2.microsoft.com/en-us/library/ms684342.aspx
> +    # typedef struct _OVERLAPPED {
> +    #   ULONG_PTR Internal;
> +    #   ULONG_PTR InternalHigh;
> +    #   union {
> +    #     struct {
> +    #       DWORD Offset;
> +    #       DWORD OffsetHigh;
> +    #     };
> +    #     PVOID Pointer;
> +    #   };
> +    #   HANDLE hEvent;
> +    # } OVERLAPPED,
> +
> +    class _inner_struct(ctypes.Structure):
> +        _fields_ = [('Offset', ctypes.c_uint), # DWORD
> +                    ('OffsetHigh', ctypes.c_uint), # DWORD
> +                   ]
> +
> +    class _inner_union(ctypes.Union):
> +        _fields_  = [('anon_struct', _inner_struct), # struct
> +                     ('Pointer', ctypes.c_void_p), # PVOID
> +                    ]
> +
> +    class OVERLAPPED(ctypes.Structure):
> +        _fields_ = [('Internal', ctypes.c_void_p), # ULONG_PTR
> +                    ('InternalHigh', ctypes.c_void_p), # ULONG_PTR
> +                    ('_inner_union', _inner_union),
> +                    ('hEvent', ctypes.c_void_p), # HANDLE
> +                   ]
> +
> +    class _ctypes_FileLock(_base_Lock):
> +
> +        def _lock(self, filename, openmode, lockmode):
> +            self._open(filename, openmode)
> +
> +            self.hfile = msvcrt.get_osfhandle(self.f.fileno())
> +            overlapped = OVERLAPPED()
> +            p_overlapped = ctypes.pointer(overlapped)

^-- In my own programs I use ctypes.byref() function to pass pointer to some
struct in C-function as argument. I'm okay with your implementation though.

> +            result = _LockFileEx(self.hfile, # HANDLE hFile
> +                                 lockmode,   # DWORD dwFlags
> +                                 0,          # DWORD dwReserved
> +                                 0x7fffffff, # DWORD nNumberOfBytesToLockLow
> +                                 0x00000000, # DWORD nNumberOfBytesToLockHigh
> +                                 p_overlapped, # lpOverlapped
> +                                )

^-- I know, constant 0x7fffffff was here before. Just curious: if it have real sense
to be 2147483647 bytes, or it related to fcntl-compatibility?


Other code I also read, understand, and found:
Errors: 0, Warnings: 0.

+1.

[µ]
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (MingW32)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFF+ZCozYr338mxwCURAg9MAJ4qg91wdMl/lQ5qtvq27p22FwNpYQCeODJ5
YScYc3l5h2SVlJ1iRgsUAE8=
=UEo+
-----END PGP SIGNATURE-----