Ticket #19707: ticket_19707-2.diff

django/db/__init__.py

@@ -42 +42 @@
 # Register an event that closes the database connection
 # when a Django request is finished.
 def close_connection(**kwargs):
-    for conn in connections.all():
-        conn.close()
+    # Avoid circular imports
+    from django.db import transaction
+    for conn in connections:
+        try:
+            transaction.abort(conn)
+            connections[conn].close()
+        except:
+            # The connection's state is unknown, so it has to be
+            # abandoned. This could happen for example if the network
+            # connection has a failure.
+            del connections[conn]
 signals.request_finished.connect(close_connection)
 
 # Register an event that resets connection.queries

django/db/backends/__init__.py

@@ -88 +88 @@
             return
         self.cursor().execute(self.ops.savepoint_commit_sql(sid))
 
+    def abort(self):
+        """
+        Roll back any ongoing transaction and clean the transaction state
+        stack.
+        """
+        if self._dirty:
+            self._rollback()
+            self._dirty = False
+        while self.transaction_state:
+            self.leave_transaction_management()
+
     def enter_transaction_management(self, managed=True):
         """
         Enters transaction management for a running thread. It must be balanced with

django/db/transaction.py

@@ -24 +24 @@
     """
     pass
 
+def abort(using=None):
+    """
+    Roll back any ongoing transactions and clean the transaction management
+    state of the connection.
+
+    This method is to be used only in cases where using balanced
+    leave_transaction_management() calls isn't possible. For example after a
+    request has finished, the transaction state isn't known, yet the connection
+    must be cleaned up for the next request.
+    """
+    if using is None:
+        using = DEFAULT_DB_ALIAS
+    connection = connections[using]
+    connection.abort()
+
 def enter_transaction_management(managed=True, using=None):
     """
     Enters transaction management for a running thread. It must be balanced with

django/db/utils.py

@@ -98 +98 @@
     def __setitem__(self, key, value):
         setattr(self._connections, key, value)
 
+    def __delitem__(self, key):
+        delattr(self._connections, key)
+
     def __iter__(self):
         return iter(self.databases)
 

django/middleware/transaction.py

@@ -15 +15 @@
     def process_exception(self, request, exception):
         """Rolls back the database and leaves transaction management"""
         if transaction.is_dirty():
+            # This rollback might fail for closed connections for example.
             transaction.rollback()
         transaction.leave_transaction_management()
 
@@ -22 +23 @@
         """Commits and leaves transaction management."""
         if transaction.is_managed():
             if transaction.is_dirty():
-                transaction.commit()
+                # Note: it is possible that the commit fails. If the reason is
+                # closed connection or some similar reason, then there is
+                # little hope to proceed nicely. However, in case of deferred
+                # foreign key check failures we can still rollback(). In any
+                # case we want to have an informative exception raised.
+                try:
+                    transaction.commit()
+                except:
+                    # If the rollback fails, the transaction state will be
+                    # messed up. It doesn't matter, the connection will be set
+                    # to clean state after the request finishes. And, we can't
+                    # clean the state here properly even if we wanted to, the
+                    # connection is in transaction but we can't rollback...
+                    transaction.rollback()
+                    transaction.leave_transaction_management()
+                    raise
             transaction.leave_transaction_management()
         return response

django/test/testcases.py

@@ -70 +70 @@
 real_enter_transaction_management = transaction.enter_transaction_management
 real_leave_transaction_management = transaction.leave_transaction_management
 real_managed = transaction.managed
+real_abort = transaction.abort
 
 def nop(*args, **kwargs):
     return
@@ -80 +81 @@
     transaction.enter_transaction_management = nop
     transaction.leave_transaction_management = nop
     transaction.managed = nop
+    transaction.abort = nop
 
 def restore_transaction_methods():
     transaction.commit = real_commit
@@ -87 +89 @@
     transaction.enter_transaction_management = real_enter_transaction_management
     transaction.leave_transaction_management = real_leave_transaction_management
     transaction.managed = real_managed
+    transaction.abort = real_abort
 
 
 def assert_and_parse_html(self, html, user_msg, msg):

tests/regressiontests/middleware/tests.py

@@ -9 +9 @@
 
 from django.conf import settings
 from django.core import mail
-from django.db import transaction
-from django.http import HttpRequest
-from django.http import HttpResponse, StreamingHttpResponse
+from django.db import (transaction, connections, DEFAULT_DB_ALIAS,
+                       IntegrityError)
+from django.http import HttpRequest, HttpResponse, StreamingHttpResponse
 from django.middleware.clickjacking import XFrameOptionsMiddleware
 from django.middleware.common import CommonMiddleware, BrokenLinkEmailsMiddleware
 from django.middleware.http import ConditionalGetMiddleware
@@ -710 +710 @@
         TransactionMiddleware().process_exception(self.request, None)
         self.assertEqual(Band.objects.count(), 0)
         self.assertFalse(transaction.is_dirty())
+
+    def test_failing_commit(self):
+        # It is possible that connection.commit() fails. Check that
+        # TransactionMiddleware handles such cases correctly.
+        try:
+            def raise_exception():
+                raise IntegrityError()
+            connections[DEFAULT_DB_ALIAS].commit = raise_exception
+            transaction.enter_transaction_management()
+            transaction.managed(True)
+            Band.objects.create(name='The Beatles')
+            self.assertTrue(transaction.is_dirty())
+            with self.assertRaises(IntegrityError):
+                TransactionMiddleware().process_response(self.request, None)
+            self.assertEqual(Band.objects.count(), 0)
+            self.assertFalse(transaction.is_dirty())
+            self.assertFalse(transaction.is_managed())
+        finally:
+            del connections[DEFAULT_DB_ALIAS].commit

tests/regressiontests/requests/tests.py

@@ -6 +6 @@
 from datetime import datetime, timedelta
 from io import BytesIO
 
+from django.db import connection, connections, DEFAULT_DB_ALIAS
+from django.core import signals
 from django.core.exceptions import SuspiciousOperation
 from django.core.handlers.wsgi import WSGIRequest, LimitedStream
 from django.http import HttpRequest, HttpResponse, parse_cookie, build_request_repr, UnreadablePostError
+from django.test import TransactionTestCase
 from django.test.client import FakePayload
 from django.test.utils import override_settings, str_prefix
 from django.utils import six
@@ -524 +527 @@
 
         with self.assertRaises(UnreadablePostError):
             request.body
+
+class TransactionRequestTests(TransactionTestCase):
+    def test_request_finished_db_state(self):
+        # The GET below will not succeed, but it will give a response with
+        # defined ._handler_class. That is needed for sending the request_finished
+        # signal. However, the test client itself will not send request_finished
+        # signals.
+        response = self.client.get('/')
+        # Taking a cursor will open a new connection.
+        connection.cursor()
+        connection.enter_transaction_management()
+        connection.managed(True)
+        signals.request_finished.send(sender=response._handler_class)
+        # in-memory sqlite doesn't actually close connections when connections are closed.
+        if connection.vendor != 'sqlite':
+            self.assertIs(connection.connection, None)
+        # And the transaction state is reset.
+        self.assertEqual(len(connection.transaction_state), 0)
+    
+    @unittest.skipIf(connection.vendor == 'sqlite',
+            'This test will close the connection which must not be done with '
+            'in-memory sqlite')
+    def test_request_finished_failed_connection(self):
+        # See comments in test_request_finished_db_state() for the self.client
+        # usage.
+        response = self.client.get('/')
+
+        conn = connections[DEFAULT_DB_ALIAS]
+        conn.enter_transaction_management()
+        conn.managed(True)
+        conn.set_dirty()
+
+        # Test that the rollback doesn't succeed (for example network failure
+        # could cause this).
+        def fail_horribly():
+            raise Exception("Horrible failure!")
+        conn._rollback = fail_horribly
+
+        signals.request_finished.send(sender=response._handler_class)
+        # As even rollback wasn't possible the connection wrapper itself was
+        # abandoned.
+        self.assertIsNot(conn, connections[DEFAULT_DB_ALIAS])
+        # And for the connection (that is, django.db.connection) that it has
+        # its transaction state reset.
+        self.assertEqual(len(connection.transaction_state), 0)