Merge lp:~rockstar/tarmac/lock-issues into lp:tarmac

[Paul Hummer]

Wrap branch and tree locks in a finally to guarantee the .unlock will be called

This solution is *pretty* naive, but I thought I'd give it a shot (and it's a good idea anyway). I wrapped all the .lock() calls in a try: block with the unlock in a finally: block. This may or may not fix the segfault problem in our current tarmac, but it's worth a shot, and might be an easy win, rather than having to set up some really complicated test environment to reproduce. *shrug*

[dobey (dobey)]

I don't think this code is where the problem is with the tarmac we have set up for ubuntuone. It is a fork with a patch I was working on, to add more extensive locking to deal with the multiple tarmac instances getting run, cases. I asked sidnei to link me to a couple logs where I could more explicitly where it was failing. We really need to properly fix the locking in tarmac though, as calling bzr_branch.lock_read() and such is incorrect. bzrlib should be holding the branch locked the entire time we're using it, but the way we're using bzr branches sort of breaks the way that works.

I'm going to vote Disapprove on this branch, because I'm like 10^1000% sure that the issue we're seeing, is not in these calls. Rather it's almost certainly happening in the command.py which is patched in the running instance, to keep the branch locked the entire time tarmac is dealing with that specific branch, so that other instances, or people, don't step on its feet while tests are running for a branch or something. We can discuss more on IRC tomorrow.

review: Disapprove

[dobey (dobey)]

So, based on our conversation in IRC, I don't disapprove of this branch on principle, as it's technically the correct thing to do with the code in trunk. I do think it might play a bit of potential havoc with the patch we're running tarmac with, which does locking/unlocking of the branch in the command.py code. But we can land this if you feel it's necessary, and poke about at fixing the locking properly, by refactoring the branch code.

review: Approve

=== modified file 'tarmac/branch.py'
--- tarmac/branch.py	2010-10-22 17:59:26 +0000
+++ tarmac/branch.py	2012-02-29 23:21:34 +0000
@@ -113,10 +113,13 @@
     @property
     def unmanaged_files(self):
         """Get the list of ignored and unknown files in the tree."""
-        self.tree.lock_read()
-        unmanaged = [x for x in self.tree.unknowns()]
-        unmanaged.extend([x[0] for x in self.tree.ignored_files()])
-        self.tree.unlock()
+        unmanaged = []
+        try:
+            self.tree.lock_read()
+            unmanaged = [x for x in self.tree.unknowns()]
+            unmanaged.extend([x[0] for x in self.tree.ignored_files()])
+        finally:
+            self.tree.unlock()
         return unmanaged
 
     @property
@@ -160,26 +163,28 @@
         author_list = []
 
         if self.target:
-            self.bzr_branch.lock_read()
-            self.target.bzr_branch.lock_read()
-
-            graph = self.bzr_branch.repository.get_graph(
-                self.target.bzr_branch.repository)
-
-            unique_ids = graph.find_unique_ancestors(
-                self.bzr_branch.last_revision(),
-                [self.target.bzr_branch.last_revision()])
-
-            revs = self.bzr_branch.repository.get_revisions(unique_ids)
-            for rev in revs:
-                apparent_authors = rev.get_apparent_authors()
-                for author in apparent_authors:
-                    author.replace('\n', '')
-                    if author not in author_list:
-                        author_list.append(author)
-
-            self.target.bzr_branch.unlock()
-            self.bzr_branch.unlock()
+            try:
+                self.bzr_branch.lock_read()
+                self.target.bzr_branch.lock_read()
+
+                graph = self.bzr_branch.repository.get_graph(
+                    self.target.bzr_branch.repository)
+
+                unique_ids = graph.find_unique_ancestors(
+                    self.bzr_branch.last_revision(),
+                    [self.target.bzr_branch.last_revision()])
+
+                revs = self.bzr_branch.repository.get_revisions(unique_ids)
+                for rev in revs:
+                    apparent_authors = rev.get_apparent_authors()
+                    for author in apparent_authors:
+                        author.replace('\n', '')
+                        if author not in author_list:
+                            author_list.append(author)
+
+            finally:
+                self.target.bzr_branch.unlock()
+                self.bzr_branch.unlock()
         else:
             last_rev = self.bzr_branch.last_revision()
             if last_rev != 'null:':
@@ -195,18 +200,20 @@
         """Return the list of bugs fixed by the branch."""
         bugs_list = []
 
-        self.bzr_branch.lock_read()
-        oldrevid = self.bzr_branch.get_rev_id(self.lp_branch.revision_count)
-        for rev_info in self.bzr_branch.iter_merge_sorted_revisions(
-            stop_revision_id=oldrevid):
-            try:
-                rev = self.bzr_branch.repository.get_revision(rev_info[0])
-                for bug in rev.iter_bugs():
-                    if bug[0].startswith('https://launchpad.net/bugs/'):
-                        bugs_list.append(bug[0].replace(
-                                'https://launchpad.net/bugs/', ''))
-            except NoSuchRevision:
-                continue
+        try:
+            self.bzr_branch.lock_read()
+            oldrevid = self.bzr_branch.get_rev_id(self.lp_branch.revision_count)
+            for rev_info in self.bzr_branch.iter_merge_sorted_revisions(
+                stop_revision_id=oldrevid):
+                try:
+                    rev = self.bzr_branch.repository.get_revision(rev_info[0])
+                    for bug in rev.iter_bugs():
+                        if bug[0].startswith('https://launchpad.net/bugs/'):
+                            bugs_list.append(bug[0].replace(
+                                    'https://launchpad.net/bugs/', ''))
+                except NoSuchRevision:
+                    continue
 
-        self.bzr_branch.unlock()
+        finally:
+            self.bzr_branch.unlock()
         return bugs_list