Merge lp:~spiv/bzr/log-objectnotlocked-bug-445171 into lp:bzr/2.0

[Andrew Bennetts]

[Andrew Bennetts (spiv)]

This fixes ObjectNotLocked errors during actions like "bzr log -r 199 NEWS" in a branch of lp:bzr. The basic change is pretty simple: acquire the read_lock as soon as cmd_log acquires the branch, and hold it until the end of cmd_log. This case was supposedly already tested by blackbox tests, but because of the global chk_map page cache the bug wasn't being exposed. So this patch also adjusts TestCase._run_bzr_core to clear that global cache. We should perhaps look at what it would take to make that cache truly semantically transparent, e.g. always require a read lock even if the page is in the cache.

This also has the benefit that "bzr -Drelock log ..." is clean now :)

[John A Meinel (jameinel)]

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

Andrew Bennetts wrote:
> Andrew Bennetts has proposed merging lp:~spiv/bzr/log-objectnotlocked-bug-445171 into lp:bzr/2.0.
>
> Requested reviews:
> bzr-core (bzr-core)
> Related bugs:
> #445171 ObjectNotLocked doing 'bzr log -r 199 path/to/file'
> https://bugs.launchpad.net/bugs/445171
>
>
> This fixes ObjectNotLocked errors during actions like "bzr log -r 199 NEWS" in a branch of lp:bzr. The basic change is pretty simple: acquire the read_lock as soon as cmd_log acquires the branch, and hold it until the end of cmd_log. This case was supposedly already tested by blackbox tests, but because of the global chk_map page cache the bug wasn't being exposed. So this patch also adjusts TestCase._run_bzr_core to clear that global cache. We should perhaps look at what it would take to make that cache truly semantically transparent, e.g. always require a read lock even if the page is in the cache.
>
> This also has the benefit that "bzr -Drelock log ..." is clean now :)
>

This is starting to look like something that could benefit from
OperationWithCleanups :)

So what this *breaks* is 'bzr log' with '-r branch:' (I believe),
because -r branch:path acquires a write lock to fetch the remote
revisions locally. Note that -r -1:path *doesn't* and refers to the same
revision-id....

That said, I think it is the right thing to do.

 review: approve
 merge: approve

John
=:->

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

iEYEARECAAYFAkrpHE4ACgkQJdeBCYSNAAPS9ACfbE989hOUueKp3sEYRYmxfC1d
QgwAn25H+NiWz9Sj5KLpVuaDPZ9lYlFw
=A+4C
-----END PGP SIGNATURE-----

review: Approve

[Robert Collins (lifeless)]

 +1

[Andrew Bennetts (spiv)]

John A Meinel wrote:
[...]
> This is starting to look like something that could benefit from
> OperationWithCleanups :)

Oh, definitely. Or perhaps Command objects should have an add_cleanup method
(and probably implement it using OperationWithCleanups)...

But this is a fix for the 2.0 branch, and I'm not sure we really want to
backport OperationWithCleanups to it and then rearrange things to use it on the
conservative, stable branch.

> So what this *breaks* is 'bzr log' with '-r branch:' (I believe),
> because -r branch:path acquires a write lock to fetch the remote
> revisions locally. Note that -r -1:path *doesn't* and refers to the same
> revision-id....
>
> That said, I think it is the right thing to do.

You're right that it does break that, although obviously '-r branch: FILE' was
already broken. I'm not sure what do about that, although perhaps guarding the
fetch in RevisionSpec_branch with "if revision_b not in
branch.repository.get_parent_map([revision_b]):" would help some cases. We
really want a way to upgrade a read lock to a write lock...

Anyway, that case doesn't appear to have test coverage, so it can't be *that*
important ;)

Thanks for the swift review, I'll send it to PQM now and file a bug about the
newly broken case.

-Andrew.

=== modified file 'NEWS'
--- NEWS	2009-10-28 01:16:10 +0000
+++ NEWS	2009-10-29 05:53:10 +0000
@@ -35,12 +35,18 @@
 * Fix typos left after test_selftest refactoring.
   (Vincent Ladeuil, Matt Nordhoff, #461149)
 
+* Fixed ``ObjectNotLocked`` errors during ``bzr log -r NNN somefile``.
+  (Andrew Bennetts, #445171)
+  
 * PreviewTree file names are not limited by the encoding of the temp
   directory's filesystem. (Aaron Bentley, #436794)
 
 Improvements
 ************
 
+* ``bzr log`` now read-locks branches exactly once, so makes better use of
+  data caches.  (Andrew Bennetts)
+
 Documentation
 *************
 

=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2009-09-10 06:32:42 +0000
+++ bzrlib/builtins.py	2009-10-29 05:53:10 +0000
@@ -2274,50 +2274,51 @@
 
         file_ids = []
         filter_by_dir = False
-        if file_list:
-            # find the file ids to log and check for directory filtering
-            b, file_info_list, rev1, rev2 = _get_info_for_log_files(revision,
-                file_list)
-            for relpath, file_id, kind in file_info_list:
-                if file_id is None:
-                    raise errors.BzrCommandError(
-                        "Path unknown at end or start of revision range: %s" %
-                        relpath)
-                # If the relpath is the top of the tree, we log everything
-                if relpath == '':
-                    file_ids = []
-                    break
+        b = None
+        try:
+            if file_list:
+                # find the file ids to log and check for directory filtering
+                b, file_info_list, rev1, rev2 = _get_info_for_log_files(
+                    revision, file_list)
+                for relpath, file_id, kind in file_info_list:
+                    if file_id is None:
+                        raise errors.BzrCommandError(
+                            "Path unknown at end or start of revision range: %s" %
+                            relpath)
+                    # If the relpath is the top of the tree, we log everything
+                    if relpath == '':
+                        file_ids = []
+                        break
+                    else:
+                        file_ids.append(file_id)
+                    filter_by_dir = filter_by_dir or (
+                        kind in ['directory', 'tree-reference'])
+            else:
+                # log everything
+                # FIXME ? log the current subdir only RBC 20060203
+                if revision is not None \
+                        and len(revision) > 0 and revision[0].get_branch():
+                    location = revision[0].get_branch()
                 else:
-                    file_ids.append(file_id)
-                filter_by_dir = filter_by_dir or (
-                    kind in ['directory', 'tree-reference'])
-        else:
-            # log everything
-            # FIXME ? log the current subdir only RBC 20060203
-            if revision is not None \
-                    and len(revision) > 0 and revision[0].get_branch():
-                location = revision[0].get_branch()
-            else:
-                location = '.'
-            dir, relpath = bzrdir.BzrDir.open_containing(location)
-            b = dir.open_branch()
-            rev1, rev2 = _get_revision_range(revision, b, self.name())
-
-        # Decide on the type of delta & diff filtering to use
-        # TODO: add an --all-files option to make this configurable & consistent
-        if not verbose:
-            delta_type = None
-        else:
-            delta_type = 'full'
-        if not show_diff:
-            diff_type = None
-        elif file_ids:
-            diff_type = 'partial'
-        else:
-            diff_type = 'full'
-
-        b.lock_read()
-        try:
+                    location = '.'
+                dir, relpath = bzrdir.BzrDir.open_containing(location)
+                b = dir.open_branch()
+                b.lock_read()
+                rev1, rev2 = _get_revision_range(revision, b, self.name())
+
+            # Decide on the type of delta & diff filtering to use
+            # TODO: add an --all-files option to make this configurable & consistent
+            if not verbose:
+                delta_type = None
+            else:
+                delta_type = 'full'
+            if not show_diff:
+                diff_type = None
+            elif file_ids:
+                diff_type = 'partial'
+            else:
+                diff_type = 'full'
+
             # Build the log formatter
             if log_format is None:
                 log_format = log.log_formatter_registry.get_default(b)
@@ -2353,7 +2354,8 @@
                 diff_type=diff_type, _match_using_deltas=match_using_deltas)
             Logger(b, rqst).show(lf)
         finally:
-            b.unlock()
+            if b is not None:
+                b.unlock()
 
 
 def _get_revision_range(revisionspec_list, branch, command_name):
@@ -2423,10 +2425,15 @@
     @display_command
     def run(self, filename):
         tree, relpath = WorkingTree.open_containing(filename)
+        file_id = tree.path2id(relpath)
         b = tree.branch
-        file_id = tree.path2id(relpath)
-        for revno, revision_id, what in log.find_touching_revisions(b, file_id):
-            self.outf.write("%6d %s\n" % (revno, what))
+        b.lock_read()
+        try:
+            touching_revs = log.find_touching_revisions(b, file_id)
+            for revno, revision_id, what in touching_revs:
+                self.outf.write("%6d %s\n" % (revno, what))
+        finally:
+            b.unlock()
 
 
 class cmd_ls(Command):

=== modified file 'bzrlib/log.py'
--- bzrlib/log.py	2009-06-10 03:56:49 +0000
+++ bzrlib/log.py	2009-10-29 05:53:10 +0000
@@ -1846,9 +1846,11 @@
     :return: (branch, info_list, start_rev_info, end_rev_info) where
       info_list is a list of (relative_path, file_id, kind) tuples where
       kind is one of values 'directory', 'file', 'symlink', 'tree-reference'.
+      branch will be read-locked.
     """
     from builtins import _get_revision_range, safe_relpath_files
     tree, b, path = bzrdir.BzrDir.open_containing_tree_or_branch(file_list[0])
+    b.lock_read()
     # XXX: It's damn messy converting a list of paths to relative paths when
     # those paths might be deleted ones, they might be on a case-insensitive
     # filesystem and/or they might be in silly locations (like another branch).

=== modified file 'bzrlib/merge_directive.py'
--- bzrlib/merge_directive.py	2009-05-11 19:11:14 +0000
+++ bzrlib/merge_directive.py	2009-10-29 05:53:10 +0000
@@ -582,11 +582,13 @@
                     revision_id):
                     raise errors.PublicBranchOutOfDate(public_branch,
                                                        revision_id)
+            testament_sha1 = t.as_sha1()
         finally:
             for entry in reversed(locked):
                 entry.unlock()
-        return klass(revision_id, t.as_sha1(), time, timezone, target_branch,
-            patch, public_branch, message, bundle, base_revision_id)
+        return klass(revision_id, testament_sha1, time, timezone,
+            target_branch, patch, public_branch, message, bundle,
+            base_revision_id)
 
     def _verify_patch(self, repository):
         calculated_patch = self._generate_diff(repository, self.revision_id,

=== modified file 'bzrlib/tests/__init__.py'
--- bzrlib/tests/__init__.py	2009-08-24 20:30:18 +0000
+++ bzrlib/tests/__init__.py	2009-10-29 05:53:10 +0000
@@ -52,6 +52,7 @@
 from bzrlib import (
     branchbuilder,
     bzrdir,
+    chk_map,
     debug,
     errors,
     hooks,
@@ -1617,6 +1618,9 @@
 
     def _run_bzr_core(self, args, retcode, encoding, stdin,
             working_dir):
+        # Clear chk_map page cache, because the contents are likely to mask
+        # locking errors.
+        chk_map.clear_cache()
         if encoding is None:
             encoding = osutils.get_user_encoding()
         stdout = StringIOWrapper()