[MERGE] convert locking problems in selftest at win32 into known errors

Alexander Belchenko bialix at ukr.net
Mon Nov 12 20:25:30 GMT 2007


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

Robert Collins пишет:
> bb:comment
> 
> I'm not quite ready to vote, would like to discuss a little so I
> understand better.
> 
> On Tue, 2007-10-30 at 12:27 +0200, Alexander Belchenko wrote:
>> I'm sick to see locking errors in selftest output again and agin.
>> Here the patch, that adds 2 new methods: expectError (to convert known
>> [locking] errors to knownFailure) and get_xbit (to read executable bit
>> in the right way depending on underlying platform).
> 
>  
>> +    def expectError(self, reason, excClass, callableObj, *args, **kwargs):
>> +        """Invoke test, expecting it to raise error for the given reason.
>> +
>> +        See docstring for expectFailure and assertRaises for details.
>> +
>> +        :param  reason: expected error reason.
>> +        :param excClass: As for the except statement, this may be either an
>> +            exception class, or a tuple of classes.
>> +        :param callableObj: A callable, will be passed ``*args`` and
>> +            ``**kwargs``.
>> +        """
>> +        try:
>> +            self.assertRaises(excClass, callableObj, *args, **kwargs)
>> +        except self.failureException:
>> +            self.fail('Unexpected success.  Should raise error: %s' % reason)
>> +        else:
>> +            raise KnownFailure(reason)
> 
> This looks nice. ^^
> 
> You might even want a expectLockContentionWin32 helper to factor out the
> common code more.
> 
> Looking at it though, I think a variation might be more useful - a
> maybeExpectError(reason, excClass, error_expected, callableObj, *args,
> **kwargs):
> ...
> :param error_expected: If False and no exception was raised, return the
> result of callableObj, otherwise raise an exception..
> 
> or something like that. I say this because the pattern of
> if is_win_32(): errorExpected(... callable)
> callable()
> 
> further down in the code is very duplicative and its possible for that
> to introduce skew between the expected-error calls and the actual calls.

I think you're right here. It was boring to copy-paste everywhere failed function calls.

>> @@ -2120,6 +2138,22 @@
>>          else:
>>              self.assertIs(tree.path2id(path), None, path+' in working tree.')
>>  
>> +    def _get_xbit_posix(self, tree, file_id):
>> +        return tree.is_executable(file_id)
>> +
>> +    def _get_xbit_win32(self, tree, file_id):
>> +        tree.lock_read()
>> +        try:
>> +            xbit = tree.is_executable(file_id)
>> +        finally:
>> +            tree.unlock()
>> +        return xbit
>> +
>> +    if sys.platform != 'win32':
>> +        get_xbit = _get_xbit_posix
>> +    else:
>> +        get_xbit = _get_xbit_win32
>> +
> 
> _get_xbit_win32 looks like it will work on posix too - so why not a
> single defintion ?

Because is_executable method actually have 2 code paths inside: and only win32 ones requires
locking. Although they locking behavior should be similar on both platforms, original tests does not
lock the tree, so I decided to follow linux behavior without explicit locking. I'm OK to use the
same locking scheme everywhere, if you think it's correct.

> The following bundle changes I don't understand. Why is bundle running
> into locking errors?

I'm also don't understand why it running into locking error, but I do understand why it succeed on
linux: on linux locks are not exclusive for the same process. So I just mark all places where tests
are failed with LockContention error as expected error.

> I realise you just want to make them expected
> errors, and with the duplication removed I'll be happy for that - but
> I'd really like to know that there is a bug containing whatever analysis
> you have done on this already (e.g. does this affect users or just the
> test suite?)

 I don't understand test_bundle.py deep enough to answer on this question -- it uses a bunch of
helper functions, nad in fact it's whitebox testing, not blackbox. So in my understanding it's only
affects [whitebox] test suite. Here the traceback for one of bundle test:


testing: C:/work/Bazaar/mydev/bzr.dev/bzr
   C:\work\Bazaar\mydev\bzr.dev\bzrlib (0.93.0.dev.0 python2.5.1.final.0)

ERROR: test_bundle.V08BundleTester.test_binary_bundle
    Could not acquire lock
"C:/tmp/testbzr-nbds-7.tmp/tmpaqxosy/work/test-branch-12qldq/.bzr/checkout/dirstate"


======================================================================
ERROR: test_binary_bundle (bzrlib.tests.test_bundle.V08BundleTester)

vvvv[log from bzrlib.tests.test_bundle.V08BundleTester.test_binary_bundle]----
created control directory in file:///C:/tmp/testbzr-nbds-7.tmp/tmpaqxosy/work/b1/
creating repository in file:///C:/tmp/testbzr-nbds-7.tmp/tmpaqxosy/work/b1/.bzr/.
creating branch <bzrlib.branch.BzrBranchFormat6 object at 0x013698D0> in
file:///C:/tmp/testbzr-nbds-7.tmp/tmpaqxosy/work/b1/.bzr/
trying to create missing lock 'C:/tmp/testbzr-nbds-7.tmp/tmpaqxosy/work/b1/.bzr/checkout/dirstate'
opening working tree 'C:/tmp/testbzr-nbds-7.tmp/tmpaqxosy/work/b1'
preparing to commit
    INFO  Committing revision 1 to "C:/tmp/testbzr-nbds-7.tmp/tmpaqxosy/work/b1/".
Selecting files for commit with filter None
    INFO  added file
    INFO  added file2
added revision_id {b at cset-0-1}
    INFO  Committed revision 1.
_handle_next u'message' => [u'add binary']
_handle_next u'committer' => u'F1 <F1 at BIALIX-LAPTOP>'
_handle_next u'date' => u'Mon 2007-11-12 20:22:34.312000036 +0200'
_handle_next u'revision_id' => u'b at cset-0-1'
_handle_next u'sha1' => u'f294bce66acd724f209f2e53101b08e238dc8692'
_handle_next u'inventory_sha1' => u'f87466bcfe8f9270cadeee7d9c3b9169614cc2dd'
_handle_next u'base_id' => u'null:'
_handle_next u'properties' => [u'branch-nick: b1']
created control directory in file:///C:/tmp/testbzr-nbds-7.tmp/tmpaqxosy/work/.%5Ctest-branch-a4dwvh/
creating repository in file:///C:/tmp/testbzr-nbds-7.tmp/tmpaqxosy/work/test-branch-a4dwvh/.bzr/.
creating branch <bzrlib.branch.BzrBranchFormat6 object at 0x013698D0> in
file:///C:/tmp/testbzr-nbds-7.tmp/tmpaqxosy/work/test-branch-a4dwvh/.bzr/
trying to create missing lock
'C:/tmp/testbzr-nbds-7.tmp/tmpaqxosy/work/test-branch-a4dwvh/.bzr/checkout/dirstate'
opening working tree 'C:/tmp/testbzr-nbds-7.tmp/tmpaqxosy/work/test-branch-a4dwvh'
Verified 0 sha hashes for the bundle.
added revision_id {b at cset-0-1}
    INFO  All changes applied successfully.
preparing to commit
    INFO  Committing revision 2 to "C:/tmp/testbzr-nbds-7.tmp/tmpaqxosy/work/b1/".
Selecting files for commit with filter None
    INFO  missing file
    INFO  deleted file
added revision_id {b at cset-0-2}
    INFO  Committed revision 2.
_handle_next u'message' => [u'delete binary']
_handle_next u'committer' => u'F1 <F1 at BIALIX-LAPTOP>'
_handle_next u'date' => u'Mon 2007-11-12 20:22:34.578000069 +0200'
_handle_next u'revision_id' => u'b at cset-0-2'
_handle_next u'sha1' => u'6b3e554d7328377f0f1243b7cf7a8137b637ab64'
_handle_next u'inventory_sha1' => u'0b910e98958ea2d0eae1af126cf88e5a64372909'
_handle_next u'parent_ids' => [u'b at cset-0-1']
_handle_next u'base_id' => u'b at cset-0-1'
_handle_next u'properties' => [u'branch-nick: b1']
created control directory in file:///C:/tmp/testbzr-nbds-7.tmp/tmpaqxosy/work/.%5Ctest-branch-12qldq/
creating repository in file:///C:/tmp/testbzr-nbds-7.tmp/tmpaqxosy/work/test-branch-12qldq/.bzr/.
creating branch <bzrlib.branch.BzrBranchFormat6 object at 0x013698D0> in
file:///C:/tmp/testbzr-nbds-7.tmp/tmpaqxosy/work/test-branch-12qldq/.bzr/
trying to create missing lock
'C:/tmp/testbzr-nbds-7.tmp/tmpaqxosy/work/test-branch-12qldq/.bzr/checkout/dirstate'
opening working tree 'C:/tmp/testbzr-nbds-7.tmp/tmpaqxosy/work/test-branch-12qldq'
_handle_next u'message' => [u'add binary']
_handle_next u'committer' => u'F1 <F1 at BIALIX-LAPTOP>'
_handle_next u'date' => u'Mon 2007-11-12 20:22:34.312000036 +0200'
_handle_next u'revision_id' => u'b at cset-0-1'
_handle_next u'sha1' => u'f294bce66acd724f209f2e53101b08e238dc8692'
_handle_next u'inventory_sha1' => u'f87466bcfe8f9270cadeee7d9c3b9169614cc2dd'
_handle_next u'base_id' => u'null:'
_handle_next u'properties' => [u'branch-nick: b1']
Verified 0 sha hashes for the bundle.
added revision_id {b at cset-0-1}
    INFO  All changes applied successfully.
Verified 0 sha hashes for the bundle.
added revision_id {b at cset-0-2}
opening working tree 'C:/tmp/testbzr-nbds-7.tmp'

^^^^[log from bzrlib.tests.test_bundle.V08BundleTester.test_binary_bundle]----
- ----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\work\Bazaar\mydev\bzr.dev\bzrlib\tests\test_bundle.py", line 699, in test_binary_bundle
    self.get_valid_bundle('b at cset-0-1', 'b at cset-0-2')
  File "C:\work\Bazaar\mydev\bzr.dev\bzrlib\tests\test_bundle.py", line 416, in get_valid_bundle
    checkout_dir=checkout_dir)
  File "C:\work\Bazaar\mydev\bzr.dev\bzrlib\tests\test_bundle.py", line 509, in valid_apply_bundle
    merge_bundle(info, to_tree, True, Merge3Merger, False, False)
  File "C:\work\Bazaar\mydev\bzr.dev\bzrlib\bundle\apply_bundle.py", line 71, in merge_bundle
    conflicts = merger.do_merge()
  File "C:\work\Bazaar\mydev\bzr.dev\bzrlib\merge.py", line 370, in do_merge
    self.base_tree.lock_read()
  File "C:\work\Bazaar\mydev\bzr.dev\bzrlib\workingtree_4.py", line 1603, in lock_read
    self._dirstate.lock_read()
  File "C:\work\Bazaar\mydev\bzr.dev\bzrlib\dirstate.py", line 2618, in lock_read
    self._lock_token = lock.ReadLock(self._filename)
  File "C:\work\Bazaar\mydev\bzr.dev\bzrlib\lock.py", line 293, in __init__
    self._lock(filename, 'rb', LOCK_SH + LOCK_NB)
  File "C:\work\Bazaar\mydev\bzr.dev\bzrlib\lock.py", line 274, in _lock
    raise errors.LockContention(filename)
LockContention: Could not acquire lock
"C:/tmp/testbzr-nbds-7.tmp/tmpaqxosy/work/test-branch-12qldq/.bzr/checkout/dirstate"


All other bundle tests fails in the same manner.

> 
>> === modified file 'bzrlib/tests/test_bundle.py'
>> --- bzrlib/tests/test_bundle.py	2007-09-24 08:46:35 +0000
>> +++ bzrlib/tests/test_bundle.py	2007-10-29 12:24:19 +0000
>> @@ -349,6 +349,8 @@
>  
> 
> 
>> === modified file 'bzrlib/tests/test_merge.py'
>> --- bzrlib/tests/test_merge.py	2007-09-14 09:51:38 +0000
>> +++ bzrlib/tests/test_merge.py	2007-10-29 13:21:49 +0000
>> @@ -16,6 +16,7 @@
>>  
>>  import os
>>  from StringIO import StringIO
>> +import sys
>>  
>>  from bzrlib import (
>>      conflicts,
>> @@ -138,7 +139,13 @@
>>          tree_b = dir_b.open_workingtree()
>>          tree_a.commit(message="hello again")
>>          log = StringIO()
>> -        merge_inner(tree_b.branch, tree_a, tree_b.basis_tree(), 
>> +        if sys.platform == 'win32':
>> +            self.expectError('on win32 OS locks are exclusive within process',
>> +                errors.LockContention,
>> +                merge_inner,
>> +                tree_b.branch, tree_a, tree_b.basis_tree(),
>> +                this_tree=tree_b, ignore_zero=True)
>> +        merge_inner(tree_b.branch, tree_a, tree_b.basis_tree(),
>>                      this_tree=tree_b, ignore_zero=True)

IIRC, here lock error occurs in tree_b.basis_tree() call. And in many other cases too.

> 
> And the same for the above
> 
>>          log = self._get_log(keep_log_file=True)
>>          self.failUnless('All changes applied successfully.\n' not in log)
> 
> 
> ...
> 
> In fact looking at all the following expected error changes it really
> looks strange, because even on linux we can't open the dirstate twice
> safely.

No, you can. Because in the same process the locks are not exclusive. That's main difference from win32.

> 
> Is there a bug open about this (I'm sure there probably is :)).
> 
> -Rob
> 

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

iD8DBQFHOLa6zYr338mxwCURAps4AKCUd2B92DTa0hLtcXy/qiAn2ZzExwCfYM6v
J26mP+rg2mNMqUSglChlbZg=
=rHoV
-----END PGP SIGNATURE-----



More information about the bazaar mailing list