Rev 4732: (andrew) Add 'only_raises' decorator, in file:///home/pqm/archives/thelove/bzr/%2Btrunk/

Canonical.com Patch Queue Manager pqm at pqm.ubuntu.com
Thu Oct 8 04:11:43 BST 2009


At file:///home/pqm/archives/thelove/bzr/%2Btrunk/

------------------------------------------------------------
revno: 4732 [merge]
revision-id: pqm at pqm.ubuntu.com-20091008031135-6d7vxh4s0umav1eo
parent: pqm at pqm.ubuntu.com-20091006204548-bjnc3z4k256ppimz
parent: andrew.bennetts at canonical.com-20091008015533-pfzig4dpr3kqt2cc
committer: Canonical.com Patch Queue Manager <pqm at pqm.ubuntu.com>
branch nick: +trunk
timestamp: Thu 2009-10-08 04:11:35 +0100
message:
  (andrew) Add 'only_raises' decorator,
  	use it to suppress most errors from Branch/Repository.unlock.
modified:
  NEWS                           NEWS-20050323055033-4e00b5db738777ff
  bzrlib/branch.py               branch.py-20050309040759-e4baf4e0d046576e
  bzrlib/decorators.py           decorators.py-20060112082512-6bfc2d882df1698d
  bzrlib/lockable_files.py       control_files.py-20051111201905-bb88546e799d669f
  bzrlib/lockdir.py              lockdir.py-20060220222025-98258adf27fbdda3
  bzrlib/remote.py               remote.py-20060720103555-yeeg2x51vn0rbtdp-1
  bzrlib/repofmt/pack_repo.py    pack_repo.py-20070813041115-gjv5ma7ktfqwsjgn-1
  bzrlib/repository.py           rev_storage.py-20051111201905-119e9401e46257e3
  bzrlib/tests/__init__.py       selftest.py-20050531073622-8d0e3c8845c97a64
  bzrlib/tests/lock_helpers.py   LockHelpers.py-20060707151933-tav3o2hpibwi53u4-1
  bzrlib/tests/per_branch/test_locking.py test_locking.py-20060707151933-tav3o2hpibwi53u4-4
  bzrlib/tests/per_repository/test_write_group.py test_write_group.py-20070716105516-89n34xtogq5frn0m-1
  bzrlib/tests/test_decorators.py test_decorators.py-20060113063037-0e7bd4566758f4fa
  doc/developers/HACKING.txt     HACKING-20050805200004-2a5dc975d870f78c
=== modified file 'NEWS'
--- a/NEWS	2009-10-06 20:45:48 +0000
+++ b/NEWS	2009-10-08 01:55:33 +0000
@@ -157,6 +157,13 @@
   See also <https://answers.launchpad.net/bzr/+faq/703>.
   (Martin Pool, #406113, #430529)
 
+* Secondary errors that occur during Branch.unlock and Repository.unlock
+  no longer obscure the original error.  These methods now use a new
+  decorator, ``only_raises``.  This fixes many causes of
+  ``TooManyConcurrentRequests`` and similar errors.
+  (Andrew Bennetts, #429747)
+
+
 Documentation
 *************
 

=== modified file 'bzrlib/branch.py'
--- a/bzrlib/branch.py	2009-08-19 18:04:49 +0000
+++ b/bzrlib/branch.py	2009-09-30 07:02:41 +0000
@@ -46,7 +46,7 @@
     )
 """)
 
-from bzrlib.decorators import needs_read_lock, needs_write_lock
+from bzrlib.decorators import needs_read_lock, needs_write_lock, only_raises
 from bzrlib.hooks import HookPoint, Hooks
 from bzrlib.inter import InterObject
 from bzrlib import registry
@@ -2160,6 +2160,7 @@
                 self.repository.unlock()
             raise
 
+    @only_raises(errors.LockNotHeld, errors.LockBroken)
     def unlock(self):
         try:
             self.control_files.unlock()

=== modified file 'bzrlib/decorators.py'
--- a/bzrlib/decorators.py	2009-03-23 14:59:43 +0000
+++ b/bzrlib/decorators.py	2009-10-02 06:12:20 +0000
@@ -24,6 +24,8 @@
 
 import sys
 
+from bzrlib import trace
+
 
 def _get_parameters(func):
     """Recreate the parameters for a function using introspection.
@@ -204,6 +206,31 @@
     return write_locked
 
 
+def only_raises(*errors):
+    """Make a decorator that will only allow the given error classes to be
+    raised.  All other errors will be logged and then discarded.
+
+    Typical use is something like::
+
+        @only_raises(LockNotHeld, LockBroken)
+        def unlock(self):
+            # etc
+    """
+    def decorator(unbound):
+        def wrapped(*args, **kwargs):
+            try:
+                return unbound(*args, **kwargs)
+            except errors:
+                raise
+            except:
+                trace.mutter('Error suppressed by only_raises:')
+                trace.log_exception_quietly()
+        wrapped.__doc__ = unbound.__doc__
+        wrapped.__name__ = unbound.__name__
+        return wrapped
+    return decorator
+
+
 # Default is more functionality, 'bzr' the commandline will request fast
 # versions.
 needs_read_lock = _pretty_needs_read_lock

=== modified file 'bzrlib/lockable_files.py'
--- a/bzrlib/lockable_files.py	2009-07-27 05:39:01 +0000
+++ b/bzrlib/lockable_files.py	2009-09-30 07:02:41 +0000
@@ -32,8 +32,7 @@
 """)
 
 from bzrlib.decorators import (
-    needs_read_lock,
-    needs_write_lock,
+    only_raises,
     )
 from bzrlib.symbol_versioning import (
     deprecated_in,
@@ -221,6 +220,7 @@
         """Setup a write transaction."""
         self._set_transaction(transactions.WriteTransaction())
 
+    @only_raises(errors.LockNotHeld, errors.LockBroken)
     def unlock(self):
         if not self._lock_mode:
             return lock.cant_unlock_not_held(self)

=== modified file 'bzrlib/lockdir.py'
--- a/bzrlib/lockdir.py	2009-07-27 05:24:02 +0000
+++ b/bzrlib/lockdir.py	2009-09-30 07:02:41 +0000
@@ -112,6 +112,7 @@
     lock,
     )
 import bzrlib.config
+from bzrlib.decorators import only_raises
 from bzrlib.errors import (
         DirectoryNotEmpty,
         FileExists,
@@ -286,6 +287,7 @@
                                             info_bytes)
         return tmpname
 
+    @only_raises(LockNotHeld, LockBroken)
     def unlock(self):
         """Release a held lock
         """

=== modified file 'bzrlib/remote.py'
--- a/bzrlib/remote.py	2009-10-02 05:43:41 +0000
+++ b/bzrlib/remote.py	2009-10-08 01:50:30 +0000
@@ -33,7 +33,7 @@
 )
 from bzrlib.branch import BranchReferenceFormat
 from bzrlib.bzrdir import BzrDir, RemoteBzrDirFormat
-from bzrlib.decorators import needs_read_lock, needs_write_lock
+from bzrlib.decorators import needs_read_lock, needs_write_lock, only_raises
 from bzrlib.errors import (
     NoSuchRevision,
     SmartProtocolError,
@@ -1082,6 +1082,7 @@
         else:
             raise errors.UnexpectedSmartServerResponse(response)
 
+    @only_raises(errors.LockNotHeld, errors.LockBroken)
     def unlock(self):
         if not self._lock_count:
             return lock.cant_unlock_not_held(self)
@@ -2383,6 +2384,7 @@
             return
         raise errors.UnexpectedSmartServerResponse(response)
 
+    @only_raises(errors.LockNotHeld, errors.LockBroken)
     def unlock(self):
         try:
             self._lock_count -= 1

=== modified file 'bzrlib/repofmt/pack_repo.py'
--- a/bzrlib/repofmt/pack_repo.py	2009-09-08 05:51:36 +0000
+++ b/bzrlib/repofmt/pack_repo.py	2009-09-30 07:02:41 +0000
@@ -54,7 +54,7 @@
     revision as _mod_revision,
     )
 
-from bzrlib.decorators import needs_write_lock
+from bzrlib.decorators import needs_write_lock, only_raises
 from bzrlib.btree_index import (
     BTreeGraphIndex,
     BTreeBuilder,
@@ -2345,6 +2345,7 @@
         packer = ReconcilePacker(collection, packs, extension, revs)
         return packer.pack(pb)
 
+    @only_raises(errors.LockNotHeld, errors.LockBroken)
     def unlock(self):
         if self._write_lock_count == 1 and self._write_group is not None:
             self.abort_write_group()

=== modified file 'bzrlib/repository.py'
--- a/bzrlib/repository.py	2009-09-24 04:54:19 +0000
+++ b/bzrlib/repository.py	2009-10-08 01:50:30 +0000
@@ -49,7 +49,7 @@
 from bzrlib.testament import Testament
 """)
 
-from bzrlib.decorators import needs_read_lock, needs_write_lock
+from bzrlib.decorators import needs_read_lock, needs_write_lock, only_raises
 from bzrlib.inter import InterObject
 from bzrlib.inventory import (
     Inventory,
@@ -1720,6 +1720,7 @@
         self.start_write_group()
         return result
 
+    @only_raises(errors.LockNotHeld, errors.LockBroken)
     def unlock(self):
         if (self.control_files._lock_count == 1 and
             self.control_files._lock_mode == 'w'):

=== modified file 'bzrlib/tests/__init__.py'
--- a/bzrlib/tests/__init__.py	2009-10-01 14:44:43 +0000
+++ b/bzrlib/tests/__init__.py	2009-10-08 01:50:30 +0000
@@ -1146,6 +1146,25 @@
             self.fail("Incorrect length: wanted %d, got %d for %r" % (
                 length, len(obj_with_len), obj_with_len))
 
+    def assertLogsError(self, exception_class, func, *args, **kwargs):
+        """Assert that func(*args, **kwargs) quietly logs a specific exception.
+        """
+        from bzrlib import trace
+        captured = []
+        orig_log_exception_quietly = trace.log_exception_quietly
+        try:
+            def capture():
+                orig_log_exception_quietly()
+                captured.append(sys.exc_info())
+            trace.log_exception_quietly = capture
+            func(*args, **kwargs)
+        finally:
+            trace.log_exception_quietly = orig_log_exception_quietly
+        self.assertLength(1, captured)
+        err = captured[0][1]
+        self.assertIsInstance(err, exception_class)
+        return err
+
     def assertPositive(self, val):
         """Assert that val is greater than 0."""
         self.assertTrue(val > 0, 'expected a positive value, but got %s' % val)

=== modified file 'bzrlib/tests/lock_helpers.py'
--- a/bzrlib/tests/lock_helpers.py	2009-04-15 07:30:34 +0000
+++ b/bzrlib/tests/lock_helpers.py	2009-09-30 07:02:41 +0000
@@ -17,6 +17,7 @@
 """Helper functions/classes for testing locking"""
 
 from bzrlib import errors
+from bzrlib.decorators import only_raises
 
 
 class TestPreventLocking(errors.LockError):
@@ -68,6 +69,7 @@
             return self._other.lock_write()
         raise TestPreventLocking('lock_write disabled')
 
+    @only_raises(errors.LockNotHeld, errors.LockBroken)
     def unlock(self):
         self._sequence.append((self._other_id, 'ul', self._allow_unlock))
         if self._allow_unlock:

=== modified file 'bzrlib/tests/per_branch/test_locking.py'
--- a/bzrlib/tests/per_branch/test_locking.py	2009-07-10 10:46:00 +0000
+++ b/bzrlib/tests/per_branch/test_locking.py	2009-09-30 07:02:41 +0000
@@ -139,7 +139,7 @@
         try:
             self.assertTrue(b.is_locked())
             self.assertTrue(b.repository.is_locked())
-            self.assertRaises(TestPreventLocking, b.unlock)
+            self.assertLogsError(TestPreventLocking, b.unlock)
             if self.combined_control:
                 self.assertTrue(b.is_locked())
             else:
@@ -183,7 +183,7 @@
         try:
             self.assertTrue(b.is_locked())
             self.assertTrue(b.repository.is_locked())
-            self.assertRaises(TestPreventLocking, b.unlock)
+            self.assertLogsError(TestPreventLocking, b.unlock)
             self.assertTrue(b.is_locked())
             self.assertTrue(b.repository.is_locked())
 

=== modified file 'bzrlib/tests/per_repository/test_write_group.py'
--- a/bzrlib/tests/per_repository/test_write_group.py	2009-09-08 06:25:26 +0000
+++ b/bzrlib/tests/per_repository/test_write_group.py	2009-09-30 07:02:41 +0000
@@ -84,7 +84,7 @@
         # don't need a specific exception for now - this is
         # really to be sure it's used right, not for signalling
         # semantic information.
-        self.assertRaises(errors.BzrError, repo.unlock)
+        self.assertLogsError(errors.BzrError, repo.unlock)
         # after this error occurs, the repository is unlocked, and the write
         # group is gone.  you've had your chance, and you blew it. ;-)
         self.assertFalse(repo.is_locked())

=== modified file 'bzrlib/tests/test_decorators.py'
--- a/bzrlib/tests/test_decorators.py	2009-03-23 14:59:43 +0000
+++ b/bzrlib/tests/test_decorators.py	2009-10-02 06:12:20 +0000
@@ -23,11 +23,15 @@
 from bzrlib.tests import TestCase
 
 
-def create_decorator_sample(style, except_in_unlock=False):
+class SampleUnlockError(Exception):
+    pass
+
+
+def create_decorator_sample(style, unlock_error=None):
     """Create a DecoratorSample object, using specific lock operators.
 
     :param style: The type of lock decorators to use (fast/pretty/None)
-    :param except_in_unlock: If True, raise an exception during unlock
+    :param unlock_error: If specified, an error to raise from unlock.
     :return: An instantiated DecoratorSample object.
     """
 
@@ -58,10 +62,11 @@
         def lock_write(self):
             self.actions.append('lock_write')
 
+        @decorators.only_raises(SampleUnlockError)
         def unlock(self):
-            if except_in_unlock:
+            if unlock_error:
                 self.actions.append('unlock_fail')
-                raise KeyError('during unlock')
+                raise unlock_error
             else:
                 self.actions.append('unlock')
 
@@ -119,28 +124,28 @@
 
     def test_read_lock_raises_original_error(self):
         sam = create_decorator_sample(self._decorator_style,
-                                      except_in_unlock=True)
+                                      unlock_error=SampleUnlockError())
         self.assertRaises(TypeError, sam.fail_during_read)
         self.assertEqual(['lock_read', 'fail_during_read', 'unlock_fail'],
                          sam.actions)
 
     def test_write_lock_raises_original_error(self):
         sam = create_decorator_sample(self._decorator_style,
-                                      except_in_unlock=True)
+                                      unlock_error=SampleUnlockError())
         self.assertRaises(TypeError, sam.fail_during_write)
         self.assertEqual(['lock_write', 'fail_during_write', 'unlock_fail'],
                          sam.actions)
 
     def test_read_lock_raises_unlock_error(self):
         sam = create_decorator_sample(self._decorator_style,
-                                      except_in_unlock=True)
-        self.assertRaises(KeyError, sam.frob)
+                                      unlock_error=SampleUnlockError())
+        self.assertRaises(SampleUnlockError, sam.frob)
         self.assertEqual(['lock_read', 'frob', 'unlock_fail'], sam.actions)
 
     def test_write_lock_raises_unlock_error(self):
         sam = create_decorator_sample(self._decorator_style,
-                                      except_in_unlock=True)
-        self.assertRaises(KeyError, sam.bank, 'bar', biz='bing')
+                                      unlock_error=SampleUnlockError())
+        self.assertRaises(SampleUnlockError, sam.bank, 'bar', biz='bing')
         self.assertEqual(['lock_write', ('bank', 'bar', 'bing'),
                           'unlock_fail'], sam.actions)
 
@@ -276,3 +281,21 @@
         finally:
             decorators.needs_read_lock = cur_read
             decorators.needs_write_lock = cur_write
+
+
+class TestOnlyRaisesDecorator(TestCase):
+
+    def raise_ZeroDivisionError(self):
+        1/0
+        
+    def test_raises_approved_error(self):
+        decorator = decorators.only_raises(ZeroDivisionError)
+        decorated_meth = decorator(self.raise_ZeroDivisionError)
+        self.assertRaises(ZeroDivisionError, decorated_meth)
+
+    def test_quietly_logs_unapproved_errors(self):
+        decorator = decorators.only_raises(IOError)
+        decorated_meth = decorator(self.raise_ZeroDivisionError)
+        self.assertLogsError(ZeroDivisionError, decorated_meth)
+        
+

=== modified file 'doc/developers/HACKING.txt'
--- a/doc/developers/HACKING.txt	2009-09-30 14:56:21 +0000
+++ b/doc/developers/HACKING.txt	2009-10-08 01:50:30 +0000
@@ -671,6 +671,19 @@
     may not catch every case but it's still useful sometimes.
 
 
+Cleanup methods
+===============
+
+Often when something has failed later code, including cleanups invoked
+from ``finally`` blocks, will fail too.  These secondary failures are
+generally uninteresting compared to the original exception.  So use the
+``only_raises`` decorator (from ``bzrlib.decorators``) for methods that
+are typically called in ``finally`` blocks, such as ``unlock`` methods.
+For example, ``@only_raises(LockNotHeld, LockBroken)``.  All errors that
+are unlikely to be a knock-on failure from an previous failure should be
+allowed.
+
+
 Factories
 =========
 




More information about the bazaar-commits mailing list