[Merge] Two Phase Locking

[John Arbash Meinel]

Martin Pool wrote:
> On 30 Jun 2006, John Arbash Meinel <john at arbash-meinel.com> wrote:
> 

...

>>
>> After discussing with Robert on IRC, we figured that objects should
>> always lock Other before Self. In a single threaded app, it doesn't
>> really matter, and there is a nice property of making sure you can lock
>> yourself before you try to lock other (shallow is checked before deep).
>> However, if you had a multi-threaded app, it might believe that it was
>> successfully locked, even though Other wasn't locked yet.
>> So we went for Other before Self, because it seems more correct.
> 
> I'm not totally sure I buy that - in a mt app we will need to consider 
> locking between threads, which can be done in various ways.  I think on
> the whole you would not want to override the existing external locks
> to do that, but rather have between-thread locks.  Those would naturally
> be held during the process of taking or releasing the external locks, so
> this would not be visible.
> 
> I think I can give a better explanation for that.  We want locks to be
> taken in a consistent order to avoid deadlocks.  We have a layered
> structure, so we need to specify the ordering between layers, and also
> between lockable objects on the same layer.  In general it's better to
> lock from the bottom up.  (OK, that's not much better...)

Well, other before self I would consider to be bottom up.

...

> Just one thing I'd like to fix or discuss, otherwise +1 to come in.  
> 
>>      def lock_write(self):
>> -        # TODO: test for failed two phase locks. This is known broken.
>> -        self.control_files.lock_write()
>>          self.repository.lock_write()
>> +        try:
>> +            self.control_files.lock_write()
>> +        except:
>> +            self.repository.unlock()
>> +            raise
> 
> I just discovered something interesting in Python - I had always
> imagined that a bare 'raise' was equivalent to raising the same object,
> and so you lost the original traceback.  But it turns out that it does
> preserve it, which is rather nice (and different to Java and C#, I
> think).  That relieves something that had always worried me about this
> try/except/raise pattern.

Yeah, it is a lot nicer than:

try:
except Exception, e:
  raise e

Which does munge the traceback.


> 
>> === modified file bzrlib/lock.py // last-changed:john at arbash-meinel.com-2006063
>> ... 0195802-c6e78240f04c9d52
>> --- bzrlib/lock.py
>> +++ bzrlib/lock.py
>> @@ -39,7 +39,66 @@
>>  import sys
>>  
>>  from bzrlib.trace import mutter
>> -from bzrlib.errors import LockError
>> +from bzrlib.errors import LockError, TestPreventLocking
>> +
>> +
>> +class LockWrapper(object):
>> +    """A wrapper which lets us set locking ability.
>> +
>> +    This also lets us record what objects were locked in what order,
>> +    to ensure that locking happens correctly.
>> +    """
> 
> I'd rather strongly prefer that code like this which is only used during
> testing be put in the test subdirectory, rather than in the module which
> it supports.  (Or maybe the intention is that this be used elsewhere?
> But it seems unlikely.)
> 
> A couple of reasons:
>  
>  - having code here that's never used in normal operation can't help
>    with load time and memory usage
> 
>  - things like this shouldn't be relied on by non-test code
> 
>  - people reading this file in the first instance just want to see what
>    it actually does, not how it's tested
> 
>  - it makes code review easier - for example I think the getattr would
>    be harder to justify in regular code, but makes sense for what it's
>    doing here
> 
> Does this make sense?
> 
> Some things are a matter of judgement - having just a couple of test
> helper methods on a class is probably fine, as is having transport
> servers that are currently only used by the tests.
> 
> Then you could also move TestPreventLocking there too.
> 

I agree with all of your points. And I'm fine with moving both of them.
But where do you want to move it to. Because both
'workingtree_implementations/test_workingtree.py' and
branch_implementations/test_branch.py
depend on it.

I think we have a small problem that our test utilities are mixed in
with our test cases. I went ahead and created a new file
bzrlib/tests/LockHelpers.py (in the vein of stub_server.py).

I don't usually use CamelCase, but TestUtil and a few others use it, and
it makes it more obvious what files are tests, and what files are not.
(treeshape.py really doesn't stand out if you are looking for helper
functions to write your tests).

Would we want to create a new subdir, and put testing facilities there?
Then we could also streamline our files a little bit more. And have a
TestCaseInTempDir.py rather than everything in one file.

Are you happy enough with tests/LockHelpers.py that I can merge it? Or
do you want to me to hold off?

John
=:->

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