[MERGE] Cleanup and test Lock objects

[John Arbash Meinel]

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

Alexander Belchenko wrote:
> John Arbash Meinel ?8H5B:
>>> 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?

Yeah. I wasn't getting an error, so I didn't notice. But yes, we should
be more specific there.


> 
>>> +    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')
> 

I think it was originally "LockError". ReadOnlyLockError is a specific
subclass. I just wanted to make sure that if the file is readonly, it
raises a specific error. Rather than a generic LockError. I should
remove the TODO.

> ^-- What you mean here in your TODO?
> 
> v-- IIUC these tests disabled because of Linux fcntl (mis)behaviour?
> 

For now. If we make these pass, then Linux fails at the same location
Windows does for all of the LockErrors. So I don't want to change the
semantics until I can fix all of that code. And then we can enable these
tests.

>>> +    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')
> 

...

> 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.

This is used later when I make fcntl locks act like win32 locks. I can
certainly remove it for now.

> 
>>> @@ -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
> 

...

>>> +    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.

Either way. I was looking for a way to get a pointer, and this is what I
found in cytypes.

Your idea is

result = _LockFileEx(self.hfile,
		     lockmode,
		     0,
		     0x7fffffff,
                     0x00000000,
                     overlapped.byref(),
                    )

Is that correct?

> 
>>> +            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?

Well the original one was 0x7ffff0000. I really don't know where that
came from.

The api supports a lock range up to 2^64. I figured 2^31 is enough for us.

I chose 2^31 instead of 2^32 and 2^32, because I didn't want to deal
with python overflowing a 32-bit signed integer.

> 
> 
> Other code I also read, understand, and found:
> Errors: 0, Warnings: 0.
> 
> +1.
> 
> [µ]

I actually submitted this, because I thought you approved from my add-on
patch. But I'm happy to do your cleanups and submit them.

By the way, Alexander, my branch at:

http://bzr.arbash-meinel.com/branches/bzr/0.15-dev/locking

now has proper support for updating the dirstate during 'bzr status'.

It also has updated fcntl locks, so it exposes the same bugs in our
code. (Places where we are opening the same tree 2 times, and expecting
to be able to take a read and write lock at the same time).

You may want to give it a look.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.5 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFF+ZP3JdeBCYSNAAMRAmUeAJ9/U4SH6d2N8PmJVrp/8tT8VckOjgCghiz+
ZD7HMs5mqMrjvINNUJeijKs=
=K0Y4
-----END PGP SIGNATURE-----