Merge lp:~allenap/maas/transactional-close-connections-on-outermost-exit-only into lp:~maas-committers/maas/trunk

[Gavin Panella]

When using the transactional decorator, only close old connections when leaving the outermost atomic block.

Previously it would close connections at all levels, causing transactions to be silently aborted.

[Graham Binns (gmb)]

review: Approve

=== modified file 'src/maasserver/utils/async.py'
--- src/maasserver/utils/async.py	2014-08-13 21:49:35 +0000
+++ src/maasserver/utils/async.py	2014-09-10 12:09:12 +0000
@@ -24,6 +24,7 @@
 from crochet import wait_for_reactor
 from django.db import (
     close_old_connections,
+    connection,
     transaction,
     )
 from maasserver.exceptions import IteratorReusedError
@@ -151,5 +152,7 @@
             with transaction.atomic():
                 return func(*args, **kwargs)
         finally:
-            close_old_connections()
+            # Close connections if we've left the outer-most atomic block.
+            if not connection.in_atomic_block:
+                close_old_connections()
     return call_within_transaction

=== modified file 'src/maasserver/utils/tests/test_async.py'
--- src/maasserver/utils/tests/test_async.py	2014-06-13 19:56:48 +0000
+++ src/maasserver/utils/tests/test_async.py	2014-09-10 12:09:12 +0000
@@ -127,7 +127,7 @@
 
 class TestTransactional(MAASTestCase):
 
-    def test_calls_function_within_transaction_then_closes_connections(self):
+    def test__calls_function_within_transaction_then_closes_connections(self):
         close_old_connections = self.patch(async, "close_old_connections")
 
         # No transaction has been entered (what Django calls an atomic
@@ -158,3 +158,21 @@
         # been exited, and old connections have been closed.
         self.assertFalse(connection.in_atomic_block)
         self.assertThat(close_old_connections, MockCalledOnceWith())
+
+    def test__closes_connections_only_when_leaving_atomic_block(self):
+        close_old_connections = self.patch(async, "close_old_connections")
+
+        @async.transactional
+        def inner():
+            # We're inside a `transactional` context here.
+            return "inner"
+
+        @async.transactional
+        def outer():
+            # We're inside a `transactional` context here too.
+            # Call `inner`, thus nesting `transactional` contexts.
+            return "outer > " + inner()
+
+        self.assertEqual("outer > inner", outer())
+        # Old connections have been closed only once.
+        self.assertThat(close_old_connections, MockCalledOnceWith())