launchpad-reviewers team mailing list archive

[Aaron Bentley <aaron@xxxxxxxxxxxxx>]

Aaron Bentley has proposed merging lp:~abentley/launchpad/safe-branch-upgrade into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #777958 in Launchpad itself: "branch upgrade jobs keep transaction open"
  https://bugs.launchpad.net/launchpad/+bug/777958

For more details, see:
https://code.launchpad.net/~abentley/launchpad/safe-branch-upgrade/+merge/60634

= Summary =
Fix bug #777958: branch upgrade jobs keep transaction open

== Proposed fix ==
Use a context manager to check for open transactions.

== Pre-implementation notes ==
Pre-and mid-implementation call with deryck.

== Implementation details ==
Implement TransactionFreeOperation context manager to use with operations that
must have no transaction active (i.e. long-running operations).

Provide a second contextmanager, TransactionFreeOperation.require, that ensures
TransactionFreeOperation is used at least once in its context.

== Tests ==
bin/test -t test_helpers -t test_upgrades_branch


== Demo and Q/A ==
None

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/scripts/helpers.py
  lib/lp/code/model/tests/test_branchjob.py
  lib/lp/scripts/tests/test_helpers.py
  lib/lp/code/model/branchjob.py
-- 
https://code.launchpad.net/~abentley/launchpad/safe-branch-upgrade/+merge/60634
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/safe-branch-upgrade into lp:launchpad.

=== modified file 'lib/lp/code/model/branchjob.py'
--- lib/lp/code/model/branchjob.py	2010-11-12 00:18:20 +0000
+++ lib/lp/code/model/branchjob.py	2011-05-11 14:33:21 +0000
@@ -54,7 +54,6 @@
     classProvides,
     implements,
     )
-from zope.security.proxy import removeSecurityProxy
 
 from canonical.config import config
 from canonical.database.enumcol import EnumCol
@@ -105,6 +104,7 @@
 from lp.services.job.interfaces.job import JobStatus
 from lp.services.job.model.job import Job
 from lp.services.job.runner import BaseRunnableJob
+from lp.scripts.helpers import TransactionFreeOperation
 from lp.translations.interfaces.translationimportqueue import (
     ITranslationImportQueue,
     )
@@ -171,6 +171,7 @@
         This job scans a branch for new revisions.
         """)
 
+
 class BranchJob(SQLBase):
     """Base class for jobs related to branches."""
 
@@ -365,7 +366,7 @@
         yield
         server.stop_server()
 
-    def run(self):
+    def run(self, _check_transaction=False):
         """See `IBranchUpgradeJob`."""
         # Set up the new branch structure
         upgrade_branch_path = tempfile.mkdtemp()
@@ -376,10 +377,13 @@
                 self.branch.getInternalBzrUrl())
             source_branch_transport.clone('.bzr').copy_tree_to_transport(
                 upgrade_transport.clone('.bzr'))
+            transaction.commit()
             upgrade_branch = BzrBranch.open_from_transport(upgrade_transport)
 
-            # Perform the upgrade.
-            upgrade(upgrade_branch.base)
+            # No transactions are open so the DB connection won't be killed.
+            with TransactionFreeOperation():
+                # Perform the upgrade.
+                upgrade(upgrade_branch.base)
 
             # Re-open the branch, since its format has changed.
             upgrade_branch = BzrBranch.open_from_transport(

=== modified file 'lib/lp/code/model/tests/test_branchjob.py'
--- lib/lp/code/model/tests/test_branchjob.py	2011-01-20 19:07:26 +0000
+++ lib/lp/code/model/tests/test_branchjob.py	2011-05-11 14:33:21 +0000
@@ -75,6 +75,7 @@
 from lp.code.model.branchrevision import BranchRevision
 from lp.code.model.revision import RevisionSet
 from lp.codehosting.vfs import branch_id_to_path
+from lp.scripts.helpers import TransactionFreeOperation
 from lp.services.job.interfaces.job import JobStatus
 from lp.services.job.model.job import Job
 from lp.services.osutils import override_environ
@@ -193,7 +194,6 @@
         """
         self.useBzrBranches(direct_database=True)
         branch, tree = self.create_branch_and_tree()
-        first_revision = 'rev-1'
         tree_transport = tree.bzrdir.root_transport
         tree_transport.put_bytes("hello.txt", "Hello World\n")
         tree.add('hello.txt')
@@ -307,7 +307,8 @@
 
         job = BranchUpgradeJob.create(db_branch)
         self.becomeDbUser(config.upgrade_branches.dbuser)
-        job.run()
+        with TransactionFreeOperation.require():
+            job.run()
         new_branch = Branch.open(tree.branch.base)
         self.assertEqual(
             new_branch.repository._format.get_format_string(),
@@ -632,9 +633,9 @@
             tree.merge_from_branch(tree3.branch, force=True)
             if include_ghost:
                 tree.add_parent_tree_id('rev2c-id')
-            tree.commit('rev2d', rev_id='rev2d-id', timestamp=1000, timezone=0,
-                committer='J. Random Hacker <jrandom@xxxxxxxxxxx>',
-                authors=authors)
+            tree.commit('rev2d', rev_id='rev2d-id', timestamp=1000,
+                timezone=0, authors=authors,
+                committer='J. Random Hacker <jrandom@xxxxxxxxxxx>')
         return RevisionsAddedJob.create(branch, 'rev2d-id', 'rev2d-id', '')
 
     def test_getMergedRevisionIDs(self):
@@ -719,7 +720,7 @@
 
     def test_getRevisionMessage_with_merge_authors(self):
         """Merge authors are included after the main bzr log."""
-        person = self.factory.makePerson(name='baz',
+        self.factory.makePerson(name='baz',
             displayname='Basil Blaine',
             email='baz@xxxxxxxxxx',
             email_address_status=EmailAddressStatus.VALIDATED)
@@ -911,15 +912,6 @@
         self.assertEqual(
             job.getRevisionMessage(first_revision, 1), expected)
 
-        expected_diff = (
-            "=== modified file 'hello.txt'" '\n'
-            "--- hello.txt" '\t' "2001-09-09 01:46:40 +0000" '\n'
-            "+++ hello.txt" '\t' "2001-09-10 05:33:20 +0000" '\n'
-            "@@ -1,1 +1,3 @@" '\n'
-            " Hello World" '\n'
-            "+" '\n'
-            "+Foo Bar" '\n'
-            '\n')
         expected_message = (
             u"-"*60 + '\n'
             "revno: 2" '\n'
@@ -1039,7 +1031,6 @@
             in which case an arbitrary unique string is used.
         :returns: The revision of this commit.
         """
-        seen_dirs = set()
         for file_pair in files:
             file_name = file_pair[0]
             try:
@@ -1182,7 +1173,7 @@
         # The content of the uploaded file is stored in the librarian.
         # The uploader of a POT is the series owner.
         POT_CONTENT = "pot content\n"
-        entries = self._runJobWithFile(
+        self._runJobWithFile(
             TranslationsBranchImportMode.IMPORT_TEMPLATES,
             'foo.pot', POT_CONTENT)
         # Commit so that the file is stored in the librarian.
@@ -1307,7 +1298,7 @@
         self._makeProductSeries(
             TranslationsBranchImportMode.IMPORT_TEMPLATES)
         # Put the job in ready state.
-        job = self._makeRosettaUploadJob()
+        self._makeRosettaUploadJob()
         ready_jobs = list(RosettaUploadJob.iterReady())
         self.assertEqual([], ready_jobs)
 

=== modified file 'lib/lp/scripts/helpers.py'
--- lib/lp/scripts/helpers.py	2010-08-20 20:31:18 +0000
+++ lib/lp/scripts/helpers.py	2011-05-11 14:33:21 +0000
@@ -4,8 +4,9 @@
 """Helpers for command line tools."""
 
 __metaclass__ = type
-__all__ = ["LPOptionParser"]
+__all__ = ["LPOptionParser", "TransactionFreeOperation", ]
 
+import contextlib
 from copy import copy
 from datetime import datetime
 from optparse import (
@@ -14,6 +15,8 @@
     OptionValueError,
     )
 
+import transaction
+
 from canonical.launchpad.scripts.logger import logger_options
 
 
@@ -55,8 +58,46 @@
     Automatically adds our standard --verbose, --quiet options that
     tie into our logging system.
     """
+
     def __init__(self, *args, **kw):
         kw.setdefault('option_class', LPOption)
         OptionParser.__init__(self, *args, **kw)
         logger_options(self)
 
+
+class TransactionFreeOperation:
+    """Ensure that an operation has no active transactions.
+
+    This helps ensure that long-running operations do not hold a database
+    transaction.  Long-running operations that hold a database transaction
+    may have their database connection killed, and hold locks that interfere
+    with other updates.
+    """
+
+    count = 0
+
+    @staticmethod
+    def any_active_transactions():
+        return transaction.manager._txns != {}
+
+    @classmethod
+    def __enter__(cls):
+        if cls.any_active_transactions():
+            raise AssertionError('Transaction open before operation!')
+
+    @classmethod
+    def __exit__(cls, exc_type, exc_value, traceback):
+        if cls.any_active_transactions():
+            raise AssertionError('Operation opened transaction!')
+        cls.count += 1
+
+    @classmethod
+    @contextlib.contextmanager
+    def require(cls):
+        """Require that TransactionFreeOperation is used at least once."""
+        old_count = cls.count
+        try:
+            yield
+        finally:
+            if old_count >= cls.count:
+                raise AssertionError('TransactionFreeOperation was not used.')

=== added file 'lib/lp/scripts/tests/test_helpers.py'
--- lib/lp/scripts/tests/test_helpers.py	1970-01-01 00:00:00 +0000
+++ lib/lp/scripts/tests/test_helpers.py	2011-05-11 14:33:21 +0000
@@ -0,0 +1,53 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test the helpers."""
+
+__metaclass__ = type
+
+from testtools.testcase import ExpectedException
+import transaction
+
+from lp.testing import TestCase
+from lp.scripts.helpers import TransactionFreeOperation
+
+
+class TestTransactionFreeOperation(TestCase):
+
+    def setUp(self):
+        """We can ignore transactions in general, but this test case cares."""
+        super(TestTransactionFreeOperation, self).setUp()
+        transaction.abort()
+
+    def test_pending_transaction(self):
+        """When a transaction is pending before the operation, raise."""
+        transaction.begin()
+        with ExpectedException(
+            AssertionError, 'Transaction open before operation'):
+            with TransactionFreeOperation:
+                pass
+
+    def test_transaction_during_operation(self):
+        """When the operation creates a transaction, raise."""
+        with ExpectedException(
+            AssertionError, 'Operation opened transaction!'):
+            with TransactionFreeOperation:
+                transaction.begin()
+
+    def test_transaction_free(self):
+        """When there are no transactions, do not raise."""
+        with TransactionFreeOperation:
+            pass
+
+    def test_require_no_TransactionFreeOperation(self):
+        """If TransactionFreeOperation is not used, raise."""
+        with ExpectedException(
+                AssertionError, 'TransactionFreeOperation was not used.'):
+            with TransactionFreeOperation.require():
+                pass
+
+    def test_require_with_TransactionFreeOperation(self):
+        """If TransactionFreeOperation is used, do not raise."""
+        with TransactionFreeOperation.require():
+            with TransactionFreeOperation:
+                pass