Rev 4775: (andrew) Add bzrlib.cleanup.OperationWithCleanups, in file:///home/pqm/archives/thelove/bzr/%2Btrunk/
Canonical.com Patch Queue Manager
pqm at pqm.ubuntu.com
Wed Oct 28 01:08:26 GMT 2009
At file:///home/pqm/archives/thelove/bzr/%2Btrunk/
------------------------------------------------------------
revno: 4775 [merge]
revision-id: pqm at pqm.ubuntu.com-20091028010822-2qrdd38w5rcfki8s
parent: pqm at pqm.ubuntu.com-20091027165411-s996a4rrdm66jq99
parent: andrew.bennetts at canonical.com-20091028001203-m7lgs1wtnilgo3br
committer: Canonical.com Patch Queue Manager <pqm at pqm.ubuntu.com>
branch nick: +trunk
timestamp: Wed 2009-10-28 01:08:22 +0000
message:
(andrew) Add bzrlib.cleanup.OperationWithCleanups,
and use it to make commit.py simpler and more robust.
added:
bzrlib/cleanup.py cleanup.py-20090922032110-mv6i6y8t04oon9np-1
bzrlib/tests/test_cleanup.py test_cleanup.py-20090922032110-mv6i6y8t04oon9np-2
modified:
NEWS NEWS-20050323055033-4e00b5db738777ff
bzrlib/commit.py commit.py-20050511101309-79ec1a0168e0e825
bzrlib/repository.py rev_storage.py-20051111201905-119e9401e46257e3
bzrlib/tests/__init__.py selftest.py-20050531073622-8d0e3c8845c97a64
=== modified file 'NEWS'
--- a/NEWS 2009-10-27 14:13:40 +0000
+++ b/NEWS 2009-10-28 00:12:03 +0000
@@ -27,6 +27,11 @@
* Diff parsing handles "Binary files differ" hunks. (Aaron Bentley, #436325)
+* Errors during commit are handled more robustly so that knock-on errors
+ are less likely to occur, and will not obscure the original error if
+ they do occur. This fixes some causes of ``TooManyConcurrentRequests``
+ and similar errors. (Andrew Bennetts, #429747, #243391)
+
* Fetching from stacked pre-2a repository via a smart server no longer
fails intermittently with "second push failed to complete".
(Andrew Bennetts, #437626)
=== added file 'bzrlib/cleanup.py'
--- a/bzrlib/cleanup.py 1970-01-01 00:00:00 +0000
+++ b/bzrlib/cleanup.py 2009-10-26 06:23:14 +0000
@@ -0,0 +1,177 @@
+# Copyright (C) 2009 Canonical Ltd
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+
+"""Helpers for managing cleanup functions and the errors they might raise.
+
+The usual way to run cleanup code in Python is::
+
+ try:
+ do_something()
+ finally:
+ cleanup_something()
+
+However if both `do_something` and `cleanup_something` raise an exception
+Python will forget the original exception and propagate the one from
+cleanup_something. Unfortunately, this is almost always much less useful than
+the original exception.
+
+If you want to be certain that the first, and only the first, error is raised,
+then use::
+
+ operation = OperationWithCleanups(lambda operation: do_something())
+ operation.add_cleanup(cleanup_something)
+ operation.run()
+
+This is more inconvenient (because you need to make every try block a
+function), but will ensure that the first error encountered is the one raised,
+while also ensuring all cleanups are run. See OperationWithCleanups for more
+details.
+"""
+
+
+from collections import deque
+import sys
+from bzrlib import (
+ debug,
+ trace,
+ )
+
+def _log_cleanup_error(exc):
+ trace.mutter('Cleanup failed:')
+ trace.log_exception_quietly()
+ if 'cleanup' in debug.debug_flags:
+ trace.warning('bzr: warning: Cleanup failed: %s', exc)
+
+
+def _run_cleanup(func, *args, **kwargs):
+ """Run func(*args, **kwargs), logging but not propagating any error it
+ raises.
+
+ :returns: True if func raised no errors, else False.
+ """
+ try:
+ func(*args, **kwargs)
+ except KeyboardInterrupt:
+ raise
+ except Exception, exc:
+ _log_cleanup_error(exc)
+ return False
+ return True
+
+
+def _run_cleanups(funcs):
+ """Run a series of cleanup functions."""
+ for func, args, kwargs in funcs:
+ _run_cleanup(func, *args, **kwargs)
+
+
+class OperationWithCleanups(object):
+ """A way to run some code with a dynamic cleanup list.
+
+ This provides a way to add cleanups while the function-with-cleanups is
+ running.
+
+ Typical use::
+
+ operation = OperationWithCleanups(some_func)
+ operation.run(args...)
+
+ where `some_func` is::
+
+ def some_func(operation, args, ...)
+ do_something()
+ operation.add_cleanup(something)
+ # etc
+
+ Note that the first argument passed to `some_func` will be the
+ OperationWithCleanups object.
+ """
+
+ def __init__(self, func):
+ self.func = func
+ self.cleanups = deque()
+
+ def add_cleanup(self, cleanup_func, *args, **kwargs):
+ """Add a cleanup to run.
+
+ Cleanups may be added at any time before or during the execution of
+ self.func. Cleanups will be executed in LIFO order.
+ """
+ self.cleanups.appendleft((cleanup_func, args, kwargs))
+
+ def run(self, *args, **kwargs):
+ return _do_with_cleanups(
+ self.cleanups, self.func, self, *args, **kwargs)
+
+
+def _do_with_cleanups(cleanup_funcs, func, *args, **kwargs):
+ """Run `func`, then call all the cleanup_funcs.
+
+ All the cleanup_funcs are guaranteed to be run. The first exception raised
+ by func or any of the cleanup_funcs is the one that will be propagted by
+ this function (subsequent errors are caught and logged).
+
+ Conceptually similar to::
+
+ try:
+ return func(*args, **kwargs)
+ finally:
+ for cleanup, cargs, ckwargs in cleanup_funcs:
+ cleanup(*cargs, **ckwargs)
+
+ It avoids several problems with using try/finally directly:
+ * an exception from func will not be obscured by a subsequent exception
+ from a cleanup.
+ * an exception from a cleanup will not prevent other cleanups from
+ running (but the first exception encountered is still the one
+ propagated).
+
+ Unike `_run_cleanup`, `_do_with_cleanups` can propagate an exception from a
+ cleanup, but only if there is no exception from func.
+ """
+ # As correct as Python 2.4 allows.
+ try:
+ result = func(*args, **kwargs)
+ except:
+ # We have an exception from func already, so suppress cleanup errors.
+ _run_cleanups(cleanup_funcs)
+ raise
+ else:
+ # No exception from func, so allow the first exception from
+ # cleanup_funcs to propagate if one occurs (but only after running all
+ # of them).
+ exc_info = None
+ for cleanup, c_args, c_kwargs in cleanup_funcs:
+ # XXX: Hmm, if KeyboardInterrupt arrives at exactly this line, we
+ # won't run all cleanups... perhaps we should temporarily install a
+ # SIGINT handler?
+ if exc_info is None:
+ try:
+ cleanup(*c_args, **c_kwargs)
+ except:
+ # This is the first cleanup to fail, so remember its
+ # details.
+ exc_info = sys.exc_info()
+ else:
+ # We already have an exception to propagate, so log any errors
+ # but don't propagate them.
+ _run_cleanup(cleanup, *c_args, **kwargs)
+ if exc_info is not None:
+ raise exc_info[0], exc_info[1], exc_info[2]
+ # No error, so we can return the result
+ return result
+
+
=== modified file 'bzrlib/commit.py'
--- a/bzrlib/commit.py 2009-08-28 05:00:33 +0000
+++ b/bzrlib/commit.py 2009-10-15 02:53:30 +0000
@@ -65,6 +65,7 @@
xml_serializer,
)
from bzrlib.branch import Branch
+from bzrlib.cleanup import OperationWithCleanups
import bzrlib.config
from bzrlib.errors import (BzrError, PointlessCommit,
ConflictsInTree,
@@ -234,6 +235,31 @@
commit. Pending changes to excluded files will be ignored by the
commit.
"""
+ operation = OperationWithCleanups(self._commit)
+ return operation.run(
+ message=message,
+ timestamp=timestamp,
+ timezone=timezone,
+ committer=committer,
+ specific_files=specific_files,
+ rev_id=rev_id,
+ allow_pointless=allow_pointless,
+ strict=strict,
+ verbose=verbose,
+ revprops=revprops,
+ working_tree=working_tree,
+ local=local,
+ reporter=reporter,
+ config=config,
+ message_callback=message_callback,
+ recursive=recursive,
+ exclude=exclude,
+ possible_master_transports=possible_master_transports)
+
+ def _commit(self, operation, message, timestamp, timezone, committer,
+ specific_files, rev_id, allow_pointless, strict, verbose, revprops,
+ working_tree, local, reporter, config, message_callback, recursive,
+ exclude, possible_master_transports):
mutter('preparing to commit')
if working_tree is None:
@@ -262,7 +288,6 @@
self.exclude = []
self.local = local
self.master_branch = None
- self.master_locked = False
self.recursive = recursive
self.rev_id = None
# self.specific_files is None to indicate no filter, or any iterable to
@@ -283,6 +308,7 @@
self.verbose = verbose
self.work_tree.lock_write()
+ operation.add_cleanup(self.work_tree.unlock)
self.parents = self.work_tree.get_parent_ids()
# We can use record_iter_changes IFF iter_changes is compatible with
# the command line parameters, and the repository has fast delta
@@ -293,119 +319,118 @@
(self.branch.repository._format.fast_deltas or
len(self.parents) < 2))
self.pb = bzrlib.ui.ui_factory.nested_progress_bar()
+ operation.add_cleanup(self.pb.finished)
self.basis_revid = self.work_tree.last_revision()
self.basis_tree = self.work_tree.basis_tree()
self.basis_tree.lock_read()
+ operation.add_cleanup(self.basis_tree.unlock)
+ # Cannot commit with conflicts present.
+ if len(self.work_tree.conflicts()) > 0:
+ raise ConflictsInTree
+
+ # Setup the bound branch variables as needed.
+ self._check_bound_branch(operation, possible_master_transports)
+
+ # Check that the working tree is up to date
+ old_revno, new_revno = self._check_out_of_date_tree()
+
+ # Complete configuration setup
+ if reporter is not None:
+ self.reporter = reporter
+ elif self.reporter is None:
+ self.reporter = self._select_reporter()
+ if self.config is None:
+ self.config = self.branch.get_config()
+
+ self._set_specific_file_ids()
+
+ # Setup the progress bar. As the number of files that need to be
+ # committed in unknown, progress is reported as stages.
+ # We keep track of entries separately though and include that
+ # information in the progress bar during the relevant stages.
+ self.pb_stage_name = ""
+ self.pb_stage_count = 0
+ self.pb_stage_total = 5
+ if self.bound_branch:
+ self.pb_stage_total += 1
+ self.pb.show_pct = False
+ self.pb.show_spinner = False
+ self.pb.show_eta = False
+ self.pb.show_count = True
+ self.pb.show_bar = True
+
+ self._gather_parents()
+ # After a merge, a selected file commit is not supported.
+ # See 'bzr help merge' for an explanation as to why.
+ if len(self.parents) > 1 and self.specific_files is not None:
+ raise errors.CannotCommitSelectedFileMerge(self.specific_files)
+ # Excludes are a form of selected file commit.
+ if len(self.parents) > 1 and self.exclude:
+ raise errors.CannotCommitSelectedFileMerge(self.exclude)
+
+ # Collect the changes
+ self._set_progress_stage("Collecting changes", counter=True)
+ self.builder = self.branch.get_commit_builder(self.parents,
+ self.config, timestamp, timezone, committer, revprops, rev_id)
+
try:
- # Cannot commit with conflicts present.
- if len(self.work_tree.conflicts()) > 0:
- raise ConflictsInTree
-
- # Setup the bound branch variables as needed.
- self._check_bound_branch(possible_master_transports)
-
- # Check that the working tree is up to date
- old_revno, new_revno = self._check_out_of_date_tree()
-
- # Complete configuration setup
- if reporter is not None:
- self.reporter = reporter
- elif self.reporter is None:
- self.reporter = self._select_reporter()
- if self.config is None:
- self.config = self.branch.get_config()
-
- self._set_specific_file_ids()
-
- # Setup the progress bar. As the number of files that need to be
- # committed in unknown, progress is reported as stages.
- # We keep track of entries separately though and include that
- # information in the progress bar during the relevant stages.
- self.pb_stage_name = ""
- self.pb_stage_count = 0
- self.pb_stage_total = 5
- if self.bound_branch:
- self.pb_stage_total += 1
- self.pb.show_pct = False
- self.pb.show_spinner = False
- self.pb.show_eta = False
- self.pb.show_count = True
- self.pb.show_bar = True
-
- self._gather_parents()
- # After a merge, a selected file commit is not supported.
- # See 'bzr help merge' for an explanation as to why.
- if len(self.parents) > 1 and self.specific_files is not None:
- raise errors.CannotCommitSelectedFileMerge(self.specific_files)
- # Excludes are a form of selected file commit.
- if len(self.parents) > 1 and self.exclude:
- raise errors.CannotCommitSelectedFileMerge(self.exclude)
-
- # Collect the changes
- self._set_progress_stage("Collecting changes", counter=True)
- self.builder = self.branch.get_commit_builder(self.parents,
- self.config, timestamp, timezone, committer, revprops, rev_id)
-
- try:
- self.builder.will_record_deletes()
- # find the location being committed to
- if self.bound_branch:
- master_location = self.master_branch.base
- else:
- master_location = self.branch.base
-
- # report the start of the commit
- self.reporter.started(new_revno, self.rev_id, master_location)
-
- self._update_builder_with_changes()
- self._check_pointless()
-
- # TODO: Now the new inventory is known, check for conflicts.
- # ADHB 2006-08-08: If this is done, populate_new_inv should not add
- # weave lines, because nothing should be recorded until it is known
- # that commit will succeed.
- self._set_progress_stage("Saving data locally")
- self.builder.finish_inventory()
-
- # Prompt the user for a commit message if none provided
- message = message_callback(self)
- self.message = message
-
- # Add revision data to the local branch
- self.rev_id = self.builder.commit(self.message)
-
- except Exception, e:
- mutter("aborting commit write group because of exception:")
- trace.log_exception_quietly()
- note("aborting commit write group: %r" % (e,))
- self.builder.abort()
- raise
-
- self._process_pre_hooks(old_revno, new_revno)
-
- # Upload revision data to the master.
- # this will propagate merged revisions too if needed.
- if self.bound_branch:
- self._set_progress_stage("Uploading data to master branch")
- # 'commit' to the master first so a timeout here causes the
- # local branch to be out of date
- self.master_branch.import_last_revision_info(
- self.branch.repository, new_revno, self.rev_id)
-
- # and now do the commit locally.
- self.branch.set_last_revision_info(new_revno, self.rev_id)
-
- # Make the working tree be up to date with the branch. This
- # includes automatic changes scheduled to be made to the tree, such
- # as updating its basis and unversioning paths that were missing.
- self.work_tree.unversion(self.deleted_ids)
- self._set_progress_stage("Updating the working tree")
- self.work_tree.update_basis_by_delta(self.rev_id,
- self.builder.get_basis_delta())
- self.reporter.completed(new_revno, self.rev_id)
- self._process_post_hooks(old_revno, new_revno)
- finally:
- self._cleanup()
+ self.builder.will_record_deletes()
+ # find the location being committed to
+ if self.bound_branch:
+ master_location = self.master_branch.base
+ else:
+ master_location = self.branch.base
+
+ # report the start of the commit
+ self.reporter.started(new_revno, self.rev_id, master_location)
+
+ self._update_builder_with_changes()
+ self._check_pointless()
+
+ # TODO: Now the new inventory is known, check for conflicts.
+ # ADHB 2006-08-08: If this is done, populate_new_inv should not add
+ # weave lines, because nothing should be recorded until it is known
+ # that commit will succeed.
+ self._set_progress_stage("Saving data locally")
+ self.builder.finish_inventory()
+
+ # Prompt the user for a commit message if none provided
+ message = message_callback(self)
+ self.message = message
+
+ # Add revision data to the local branch
+ self.rev_id = self.builder.commit(self.message)
+
+ except Exception, e:
+ mutter("aborting commit write group because of exception:")
+ trace.log_exception_quietly()
+ note("aborting commit write group: %r" % (e,))
+ self.builder.abort()
+ raise
+
+ self._process_pre_hooks(old_revno, new_revno)
+
+ # Upload revision data to the master.
+ # this will propagate merged revisions too if needed.
+ if self.bound_branch:
+ self._set_progress_stage("Uploading data to master branch")
+ # 'commit' to the master first so a timeout here causes the
+ # local branch to be out of date
+ self.master_branch.import_last_revision_info(
+ self.branch.repository, new_revno, self.rev_id)
+
+ # and now do the commit locally.
+ self.branch.set_last_revision_info(new_revno, self.rev_id)
+
+ # Make the working tree be up to date with the branch. This
+ # includes automatic changes scheduled to be made to the tree, such
+ # as updating its basis and unversioning paths that were missing.
+ self.work_tree.unversion(self.deleted_ids)
+ self._set_progress_stage("Updating the working tree")
+ self.work_tree.update_basis_by_delta(self.rev_id,
+ self.builder.get_basis_delta())
+ self.reporter.completed(new_revno, self.rev_id)
+ self._process_post_hooks(old_revno, new_revno)
return self.rev_id
def _select_reporter(self):
@@ -433,7 +458,7 @@
return
raise PointlessCommit()
- def _check_bound_branch(self, possible_master_transports=None):
+ def _check_bound_branch(self, operation, possible_master_transports=None):
"""Check to see if the local branch is bound.
If it is bound, then most of the commit will actually be
@@ -474,7 +499,7 @@
# so grab the lock
self.bound_branch = self.branch
self.master_branch.lock_write()
- self.master_locked = True
+ operation.add_cleanup(self.master_branch.unlock)
def _check_out_of_date_tree(self):
"""Check that the working tree is up to date.
@@ -565,42 +590,6 @@
old_revno, old_revid, new_revno, self.rev_id,
tree_delta, future_tree)
- def _cleanup(self):
- """Cleanup any open locks, progress bars etc."""
- cleanups = [self._cleanup_bound_branch,
- self.basis_tree.unlock,
- self.work_tree.unlock,
- self.pb.finished]
- found_exception = None
- for cleanup in cleanups:
- try:
- cleanup()
- # we want every cleanup to run no matter what.
- # so we have a catchall here, but we will raise the
- # last encountered exception up the stack: and
- # typically this will be useful enough.
- except Exception, e:
- found_exception = e
- if found_exception is not None:
- # don't do a plan raise, because the last exception may have been
- # trashed, e is our sure-to-work exception even though it loses the
- # full traceback. XXX: RBC 20060421 perhaps we could check the
- # exc_info and if its the same one do a plain raise otherwise
- # 'raise e' as we do now.
- raise e
-
- def _cleanup_bound_branch(self):
- """Executed at the end of a try/finally to cleanup a bound branch.
-
- If the branch wasn't bound, this is a no-op.
- If it was, it resents self.branch to the local branch, instead
- of being the master.
- """
- if not self.bound_branch:
- return
- if self.master_locked:
- self.master_branch.unlock()
-
def _gather_parents(self):
"""Record the parents of a merge for merge detection."""
# TODO: Make sure that this list doesn't contain duplicate
=== modified file 'bzrlib/repository.py'
--- a/bzrlib/repository.py 2009-10-12 22:04:23 +0000
+++ b/bzrlib/repository.py 2009-10-28 00:12:03 +0000
@@ -50,7 +50,6 @@
""")
from bzrlib.decorators import needs_read_lock, needs_write_lock, only_raises
-from bzrlib.lock import _RelockDebugMixin
from bzrlib.inter import InterObject
from bzrlib.inventory import (
Inventory,
@@ -58,6 +57,7 @@
ROOT_ID,
entry_factory,
)
+from bzrlib.lock import _RelockDebugMixin
from bzrlib import registry
from bzrlib.trace import (
log_exception_quietly, note, mutter, mutter_callsite, warning)
=== modified file 'bzrlib/tests/__init__.py'
--- a/bzrlib/tests/__init__.py 2009-10-19 10:59:16 +0000
+++ b/bzrlib/tests/__init__.py 2009-10-28 00:12:03 +0000
@@ -3722,6 +3722,7 @@
'bzrlib.tests.test_chk_serializer',
'bzrlib.tests.test_chunk_writer',
'bzrlib.tests.test_clean_tree',
+ 'bzrlib.tests.test_cleanup',
'bzrlib.tests.test_commands',
'bzrlib.tests.test_commit',
'bzrlib.tests.test_commit_merge',
=== added file 'bzrlib/tests/test_cleanup.py'
--- a/bzrlib/tests/test_cleanup.py 1970-01-01 00:00:00 +0000
+++ b/bzrlib/tests/test_cleanup.py 2009-10-26 06:23:14 +0000
@@ -0,0 +1,280 @@
+# Copyright (C) 2009 Canonical Ltd
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+
+from cStringIO import StringIO
+import re
+
+from bzrlib.cleanup import (
+ _do_with_cleanups,
+ _run_cleanup,
+ OperationWithCleanups,
+ )
+from bzrlib.tests import TestCase
+from bzrlib import (
+ debug,
+ trace,
+ )
+
+
+class CleanupsTestCase(TestCase):
+
+ def setUp(self):
+ super(CleanupsTestCase, self).setUp()
+ self.call_log = []
+
+ def no_op_cleanup(self):
+ self.call_log.append('no_op_cleanup')
+
+ def assertLogContains(self, regex):
+ log = self._get_log(keep_log_file=True)
+ self.assertContainsRe(log, regex, re.DOTALL)
+
+ def failing_cleanup(self):
+ self.call_log.append('failing_cleanup')
+ raise Exception("failing_cleanup goes boom!")
+
+
+class TestRunCleanup(CleanupsTestCase):
+
+ def test_no_errors(self):
+ """The function passed to _run_cleanup is run."""
+ self.assertTrue(_run_cleanup(self.no_op_cleanup))
+ self.assertEqual(['no_op_cleanup'], self.call_log)
+
+ def test_cleanup_with_args_kwargs(self):
+ def func_taking_args_kwargs(*args, **kwargs):
+ self.call_log.append(('func', args, kwargs))
+ _run_cleanup(func_taking_args_kwargs, 'an arg', kwarg='foo')
+ self.assertEqual(
+ [('func', ('an arg',), {'kwarg': 'foo'})], self.call_log)
+
+ def test_cleanup_error(self):
+ """An error from the cleanup function is logged by _run_cleanup, but not
+ propagated.
+
+ This is there's no way for _run_cleanup to know if there's an existing
+ exception in this situation::
+ try:
+ some_func()
+ finally:
+ _run_cleanup(cleanup_func)
+ So, the best _run_cleanup can do is always log errors but never raise
+ them.
+ """
+ self.assertFalse(_run_cleanup(self.failing_cleanup))
+ self.assertLogContains('Cleanup failed:.*failing_cleanup goes boom')
+
+ def test_cleanup_error_debug_flag(self):
+ """The -Dcleanup debug flag causes cleanup errors to be reported to the
+ user.
+ """
+ log = StringIO()
+ trace.push_log_file(log)
+ debug.debug_flags.add('cleanup')
+ self.assertFalse(_run_cleanup(self.failing_cleanup))
+ self.assertContainsRe(
+ log.getvalue(),
+ "bzr: warning: Cleanup failed:.*failing_cleanup goes boom")
+
+ def test_prior_error_cleanup_succeeds(self):
+ """Calling _run_cleanup from a finally block will not interfere with an
+ exception from the try block.
+ """
+ def failing_operation():
+ try:
+ 1/0
+ finally:
+ _run_cleanup(self.no_op_cleanup)
+ self.assertRaises(ZeroDivisionError, failing_operation)
+ self.assertEqual(['no_op_cleanup'], self.call_log)
+
+ def test_prior_error_cleanup_fails(self):
+ """Calling _run_cleanup from a finally block will not interfere with an
+ exception from the try block even when the cleanup itself raises an
+ exception.
+
+ The cleanup exception will be logged.
+ """
+ def failing_operation():
+ try:
+ 1/0
+ finally:
+ _run_cleanup(self.failing_cleanup)
+ self.assertRaises(ZeroDivisionError, failing_operation)
+ self.assertLogContains('Cleanup failed:.*failing_cleanup goes boom')
+
+
+class TestDoWithCleanups(CleanupsTestCase):
+
+ def trivial_func(self):
+ self.call_log.append('trivial_func')
+ return 'trivial result'
+
+ def test_runs_func(self):
+ """_do_with_cleanups runs the function it is given, and returns the
+ result.
+ """
+ result = _do_with_cleanups([], self.trivial_func)
+ self.assertEqual('trivial result', result)
+
+ def test_runs_cleanups(self):
+ """Cleanup functions are run (in the given order)."""
+ cleanup_func_1 = (self.call_log.append, ('cleanup 1',), {})
+ cleanup_func_2 = (self.call_log.append, ('cleanup 2',), {})
+ _do_with_cleanups([cleanup_func_1, cleanup_func_2], self.trivial_func)
+ self.assertEqual(
+ ['trivial_func', 'cleanup 1', 'cleanup 2'], self.call_log)
+
+ def failing_func(self):
+ self.call_log.append('failing_func')
+ 1/0
+
+ def test_func_error_propagates(self):
+ """Errors from the main function are propagated (after running
+ cleanups).
+ """
+ self.assertRaises(
+ ZeroDivisionError, _do_with_cleanups,
+ [(self.no_op_cleanup, (), {})], self.failing_func)
+ self.assertEqual(['failing_func', 'no_op_cleanup'], self.call_log)
+
+ def test_func_error_trumps_cleanup_error(self):
+ """Errors from the main function a propagated even if a cleanup raises
+ an error.
+
+ The cleanup error is be logged.
+ """
+ self.assertRaises(
+ ZeroDivisionError, _do_with_cleanups,
+ [(self.failing_cleanup, (), {})], self.failing_func)
+ self.assertLogContains('Cleanup failed:.*failing_cleanup goes boom')
+
+ def test_func_passes_and_error_from_cleanup(self):
+ """An error from a cleanup is propagated when the main function doesn't
+ raise an error. Later cleanups are still executed.
+ """
+ exc = self.assertRaises(
+ Exception, _do_with_cleanups,
+ [(self.failing_cleanup, (), {}), (self.no_op_cleanup, (), {})],
+ self.trivial_func)
+ self.assertEqual('failing_cleanup goes boom!', exc.args[0])
+ self.assertEqual(
+ ['trivial_func', 'failing_cleanup', 'no_op_cleanup'],
+ self.call_log)
+
+ def test_multiple_cleanup_failures(self):
+ """When multiple cleanups fail (as tends to happen when something has
+ gone wrong), the first error is propagated, and subsequent errors are
+ logged.
+ """
+ cleanups = self.make_two_failing_cleanup_funcs()
+ self.assertRaises(ErrorA, _do_with_cleanups, cleanups,
+ self.trivial_func)
+ self.assertLogContains('Cleanup failed:.*ErrorB')
+ log = self._get_log(keep_log_file=True)
+ self.assertFalse('ErrorA' in log)
+
+ def make_two_failing_cleanup_funcs(self):
+ def raise_a():
+ raise ErrorA('Error A')
+ def raise_b():
+ raise ErrorB('Error B')
+ return [(raise_a, (), {}), (raise_b, (), {})]
+
+ def test_multiple_cleanup_failures_debug_flag(self):
+ log = StringIO()
+ trace.push_log_file(log)
+ debug.debug_flags.add('cleanup')
+ cleanups = self.make_two_failing_cleanup_funcs()
+ self.assertRaises(ErrorA, _do_with_cleanups, cleanups,
+ self.trivial_func)
+ self.assertContainsRe(
+ log.getvalue(), "bzr: warning: Cleanup failed:.*Error B\n")
+ self.assertEqual(1, log.getvalue().count('bzr: warning:'),
+ log.getvalue())
+
+ def test_func_and_cleanup_errors_debug_flag(self):
+ log = StringIO()
+ trace.push_log_file(log)
+ debug.debug_flags.add('cleanup')
+ cleanups = self.make_two_failing_cleanup_funcs()
+ self.assertRaises(ZeroDivisionError, _do_with_cleanups, cleanups,
+ self.failing_func)
+ self.assertContainsRe(
+ log.getvalue(), "bzr: warning: Cleanup failed:.*Error A\n")
+ self.assertContainsRe(
+ log.getvalue(), "bzr: warning: Cleanup failed:.*Error B\n")
+ self.assertEqual(2, log.getvalue().count('bzr: warning:'))
+
+ def test_func_may_mutate_cleanups(self):
+ """The main func may mutate the cleanups before it returns.
+
+ This allows a function to gradually add cleanups as it acquires
+ resources, rather than planning all the cleanups up-front. The
+ OperationWithCleanups helper relies on this working.
+ """
+ cleanups_list = []
+ def func_that_adds_cleanups():
+ self.call_log.append('func_that_adds_cleanups')
+ cleanups_list.append((self.no_op_cleanup, (), {}))
+ return 'result'
+ result = _do_with_cleanups(cleanups_list, func_that_adds_cleanups)
+ self.assertEqual('result', result)
+ self.assertEqual(
+ ['func_that_adds_cleanups', 'no_op_cleanup'], self.call_log)
+
+ def test_cleanup_error_debug_flag(self):
+ """The -Dcleanup debug flag causes cleanup errors to be reported to the
+ user.
+ """
+ log = StringIO()
+ trace.push_log_file(log)
+ debug.debug_flags.add('cleanup')
+ self.assertRaises(ZeroDivisionError, _do_with_cleanups,
+ [(self.failing_cleanup, (), {})], self.failing_func)
+ self.assertContainsRe(
+ log.getvalue(),
+ "bzr: warning: Cleanup failed:.*failing_cleanup goes boom")
+ self.assertEqual(1, log.getvalue().count('bzr: warning:'))
+
+
+class ErrorA(Exception): pass
+class ErrorB(Exception): pass
+
+
+class TestOperationWithCleanups(CleanupsTestCase):
+
+ def test_cleanup_ordering(self):
+ """Cleanups are added in LIFO order.
+
+ So cleanups added before run is called are run last, and the last
+ cleanup added during the func is run first.
+ """
+ call_log = []
+ def func(op, foo):
+ call_log.append(('func called', foo))
+ op.add_cleanup(call_log.append, 'cleanup 2')
+ op.add_cleanup(call_log.append, 'cleanup 1')
+ return 'result'
+ owc = OperationWithCleanups(func)
+ owc.add_cleanup(call_log.append, 'cleanup 4')
+ owc.add_cleanup(call_log.append, 'cleanup 3')
+ result = owc.run('foo')
+ self.assertEqual('result', result)
+ self.assertEqual(
+ [('func called', 'foo'), 'cleanup 1', 'cleanup 2', 'cleanup 3',
+ 'cleanup 4'], call_log)
+
More information about the bazaar-commits
mailing list