launchpad-reviewers team mailing list archive

[Gavin Panella <gavin.panella@xxxxxxxxxxxxx>]

Gavin Panella has proposed merging lp:~allenap/launchpad/handle-status-ro-crash-bug-905853 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #905853 in Launchpad itself: "Most PackageBuild._handleStatus* methods attempt to write in a read-only transaction"
  https://bugs.launchpad.net/launchpad/+bug/905853

For more details, see:
https://code.launchpad.net/~allenap/launchpad/handle-status-ro-crash-bug-905853/+merge/86298

This fixes the problem described in the bug, but it also fixes the tests to (a) run correctly (i.e. with AsynchronousDeferredRunTest) and (b) run with a read-only transaction access mode by default. Combined, these brought out the problems described in the bug. This branch also (c) changes DatabaseTransactionPolicy.__enter__() to only care about open transactions when we're moving from a read-write access mode to read-only.
-- 
https://code.launchpad.net/~allenap/launchpad/handle-status-ro-crash-bug-905853/+merge/86298
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/launchpad/handle-status-ro-crash-bug-905853 into lp:launchpad.

=== modified file 'lib/lp/buildmaster/model/packagebuild.py'
--- lib/lp/buildmaster/model/packagebuild.py	2011-12-08 11:39:10 +0000
+++ lib/lp/buildmaster/model/packagebuild.py	2011-12-19 21:41:27 +0000
@@ -475,7 +475,9 @@
         MANUALDEPWAIT, store available information, remove BuildQueue
         entry and release builder slave for another job.
         """
-        self.status = BuildStatus.MANUALDEPWAIT
+        with DatabaseTransactionPolicy(read_only=False):
+            self.status = BuildStatus.MANUALDEPWAIT
+            transaction.commit()
 
         def build_info_stored(ignored):
             logger.critical("***** %s is MANUALDEPWAIT *****"
@@ -495,7 +497,9 @@
         job as CHROOTFAIL, store available information, remove BuildQueue
         and release the builder.
         """
-        self.status = BuildStatus.CHROOTWAIT
+        with DatabaseTransactionPolicy(read_only=False):
+            self.status = BuildStatus.CHROOTWAIT
+            transaction.commit()
 
         def build_info_stored(ignored):
             logger.critical(

=== modified file 'lib/lp/buildmaster/tests/test_packagebuild.py'
--- lib/lp/buildmaster/tests/test_packagebuild.py	2011-11-09 11:50:17 +0000
+++ lib/lp/buildmaster/tests/test_packagebuild.py	2011-12-19 21:41:27 +0000
@@ -12,6 +12,8 @@
 import tempfile
 
 from storm.store import Store
+from testtools.deferredruntest import AsynchronousDeferredRunTest
+import transaction
 from zope.component import getUtility
 from zope.security.interfaces import Unauthorized
 from zope.security.proxy import removeSecurityProxy
@@ -37,6 +39,7 @@
 from lp.buildmaster.model.packagebuild import PackageBuild
 from lp.buildmaster.tests.mock_slaves import WaitingSlave
 from lp.registry.interfaces.pocket import PackagePublishingPocket
+from lp.services.database.transaction_policy import DatabaseTransactionPolicy
 from lp.testing import (
     login,
     login_person,
@@ -285,6 +288,7 @@
     """Tests for `IPackageBuild`s handleStatus method."""
 
     layer = LaunchpadZopelessLayer
+    run_tests_with = AsynchronousDeferredRunTest.make_factory(timeout=20)
 
     def makeBuild(self):
         """Allow classes to override the build with which the test runs."""
@@ -319,6 +323,12 @@
         removeSecurityProxy(self.build).verifySuccessfulUpload = FakeMethod(
             result=True)
 
+        # Save everything done so far then shift into a read-only transaction
+        # access mode by default.
+        transaction.commit()
+        policy = DatabaseTransactionPolicy(read_only=True)
+        self.useContext(policy)
+
     def assertResultCount(self, count, result):
         self.assertEquals(
             1, len(os.listdir(os.path.join(self.upload_root, result))))
@@ -363,7 +373,9 @@
 
     def test_handleStatus_OK_sets_build_log(self):
         # The build log is set during handleStatus.
-        removeSecurityProxy(self.build).log = None
+        with DatabaseTransactionPolicy(read_only=False):
+            removeSecurityProxy(self.build).log = None
+            transaction.commit()
         self.assertEqual(None, self.build.log)
         d = self.build.handleStatus('OK', None, {
                 'filemap': {'myfile.py': 'test_file_hash'},
@@ -402,7 +414,10 @@
 
     def test_date_finished_set(self):
         # The date finished is updated during handleStatus_OK.
-        removeSecurityProxy(self.build).date_finished = None
+        with DatabaseTransactionPolicy(read_only=False):
+            removeSecurityProxy(self.build).date_finished = None
+            transaction.commit()
+
         self.assertEqual(None, self.build.date_finished)
         d = self.build.handleStatus('OK', None, {
                 'filemap': {'myfile.py': 'test_file_hash'},

=== modified file 'lib/lp/services/database/tests/test_transaction_policy.py'
--- lib/lp/services/database/tests/test_transaction_policy.py	2011-09-29 05:49:18 +0000
+++ lib/lp/services/database/tests/test_transaction_policy.py	2011-12-19 21:41:27 +0000
@@ -76,16 +76,26 @@
 
         self.assertRaises(InternalError, make_forbidden_update)
 
-    def test_will_not_start_in_ongoing_transaction(self):
-        # You cannot enter a DatabaseTransactionPolicy while already in
-        # a transaction.
+    def test_will_not_go_read_only_when_read_write_transaction_ongoing(self):
+        # You cannot enter a read-only DatabaseTransactionPolicy while in an
+        # active read-write transaction.
         def enter_policy():
-            with DatabaseTransactionPolicy():
+            with DatabaseTransactionPolicy(read_only=True):
                 pass
 
         self.writeToDatabase()
         self.assertRaises(TransactionInProgress, enter_policy)
 
+    def test_will_go_read_write_when_read_write_transaction_ongoing(self):
+        # You can enter a read-write DatabaseTransactionPolicy while in an
+        # active read-write transaction.
+        def enter_policy():
+            with DatabaseTransactionPolicy(read_only=False):
+                pass
+
+        self.writeToDatabase()
+        enter_policy()  # No exception.
+
     def test_successful_exit_requires_commit_or_abort(self):
         # If a read-write policy exits normally (which would probably
         # indicate successful completion of its code), it requires that

=== modified file 'lib/lp/services/database/transaction_policy.py'
--- lib/lp/services/database/transaction_policy.py	2011-10-10 06:23:12 +0000
+++ lib/lp/services/database/transaction_policy.py	2011-12-19 21:41:27 +0000
@@ -92,9 +92,18 @@
 
         :raise TransactionInProgress: if a transaction was already ongoing.
         """
-        self._checkNoTransaction(
-            "Entered DatabaseTransactionPolicy while in a transaction.")
+        # We must check the transaction status before checking the current
+        # policy because getting the policy causes a status change.
+        in_transaction = self._isInTransaction()
         self.previous_policy = self._getCurrentPolicy()
+        # If the current transaction is read-write and we're moving to
+        # read-only then we should check for a transaction. If the current
+        # policy is read-only then we don't care if a transaction is in
+        # progress.
+        if in_transaction and self.read_only and not self.previous_policy:
+            raise TransactionInProgress(
+                "Attempting to enter a read-only transaction while holding "
+                "open a read-write transaction.")
         self._setPolicy(self.read_only)
         # Commit should include the policy itself.  If this breaks
         # because the transaction was already in a failed state before

=== modified file 'lib/lp/soyuz/tests/test_binarypackagebuild.py'
--- lib/lp/soyuz/tests/test_binarypackagebuild.py	2011-11-14 06:36:57 +0000
+++ lib/lp/soyuz/tests/test_binarypackagebuild.py	2011-12-19 21:41:27 +0000
@@ -10,7 +10,6 @@
 
 import pytz
 from storm.store import Store
-from testtools.deferredruntest import AsynchronousDeferredRunTest
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
@@ -575,9 +574,6 @@
     MakeBinaryPackageBuildMixin, TestHandleStatusMixin, TestCaseWithFactory):
     """IPackageBuild.handleStatus works with binary builds."""
 
-    layer = LaunchpadZopelessLayer
-    run_tests_with = AsynchronousDeferredRunTest.make_factory(timeout=20)
-
 
 class TestBinaryPackageBuildWebservice(TestCaseWithFactory):
     """Test cases for BinaryPackageBuild on the webservice.