From 729e4ae4f0730585ac4640e7fa3aa06374677ff2 Mon Sep 17 00:00:00 2001 From: Aymeric Augustin Date: Mon, 28 Jul 2014 14:30:41 +0200 Subject: [PATCH] Fixed #23074 -- Avoided leaking savepoints in atomic. Thanks Chow Loong Jin for the report and the initial patch. --- django/db/backends/__init__.py | 1 + django/db/backends/mysql/base.py | 1 + .../db/backends/postgresql_psycopg2/base.py | 1 + django/db/backends/sqlite3/base.py | 4 ++++ django/db/transaction.py | 6 +++++ tests/transactions/tests.py | 22 +++++++++++++++++++ 6 files changed, 35 insertions(+) diff --git a/django/db/backends/__init__.py b/django/db/backends/__init__.py index 9e223c9cbb..5577d3b46e 100644 --- a/django/db/backends/__init__.py +++ b/django/db/backends/__init__.py @@ -497,6 +497,7 @@ class BaseDatabaseFeatures(object): can_return_id_from_insert = False has_bulk_insert = False uses_savepoints = False + can_release_savepoints = False can_combine_inserts_with_and_without_auto_increment_pk = False # If True, don't use integer foreign keys referring to, e.g., positive diff --git a/django/db/backends/mysql/base.py b/django/db/backends/mysql/base.py index 77369dcd8b..5aa17466ac 100644 --- a/django/db/backends/mysql/base.py +++ b/django/db/backends/mysql/base.py @@ -182,6 +182,7 @@ class DatabaseFeatures(BaseDatabaseFeatures): requires_explicit_null_ordering_when_grouping = True allows_auto_pk_0 = False uses_savepoints = True + can_release_savepoints = True atomic_transactions = False supports_column_check_constraints = False diff --git a/django/db/backends/postgresql_psycopg2/base.py b/django/db/backends/postgresql_psycopg2/base.py index 7b2cdc0aa2..4cd327a22b 100644 --- a/django/db/backends/postgresql_psycopg2/base.py +++ b/django/db/backends/postgresql_psycopg2/base.py @@ -50,6 +50,7 @@ class DatabaseFeatures(BaseDatabaseFeatures): has_select_for_update_nowait = True has_bulk_insert = True uses_savepoints = True + can_release_savepoints = True supports_tablespaces = True supports_transactions = True can_introspect_ip_address_field = True diff --git a/django/db/backends/sqlite3/base.py b/django/db/backends/sqlite3/base.py index 551c60759f..3b61a8a1e5 100644 --- a/django/db/backends/sqlite3/base.py +++ b/django/db/backends/sqlite3/base.py @@ -118,6 +118,10 @@ class DatabaseFeatures(BaseDatabaseFeatures): def uses_savepoints(self): return Database.sqlite_version_info >= (3, 6, 8) + @cached_property + def can_release_savepoints(self): + return self.uses_savepoints + @cached_property def supports_stddev(self): """Confirm support for STDDEV and related stats functions diff --git a/django/db/transaction.py b/django/db/transaction.py index 786bc53581..4fb58d6dd8 100644 --- a/django/db/transaction.py +++ b/django/db/transaction.py @@ -219,6 +219,9 @@ class Atomic(object): except DatabaseError: try: connection.savepoint_rollback(sid) + # The savepoint won't be reused. Release it to + # minimize overhead for the database server. + connection.savepoint_commit(sid) except Error: # If rolling back to a savepoint fails, mark for # rollback at a higher level and avoid shadowing @@ -249,6 +252,9 @@ class Atomic(object): else: try: connection.savepoint_rollback(sid) + # The savepoint won't be reused. Release it to + # minimize overhead for the database server. + connection.savepoint_commit(sid) except Error: # If rolling back to a savepoint fails, mark for # rollback at a higher level and avoid shadowing diff --git a/tests/transactions/tests.py b/tests/transactions/tests.py index 72e600d41d..005df0ba91 100644 --- a/tests/transactions/tests.py +++ b/tests/transactions/tests.py @@ -403,3 +403,25 @@ class AtomicMiscTests(TransactionTestCase): pass # Must not raise an exception transaction.atomic(Callable()) + + @skipUnlessDBFeature('can_release_savepoints') + def test_atomic_does_not_leak_savepoints_on_failure(self): + # Regression test for #23074 + + # Expect an error when rolling back a savepoint that doesn't exist. + # Done outside of the transaction block to ensure proper recovery. + with self.assertRaises(Error): + + # Start a plain transaction. + with transaction.atomic(): + + # Swallow the intentional error raised in the sub-transaction. + with six.assertRaisesRegex(self, Exception, "Oops"): + + # Start a sub-transaction with a savepoint. + with transaction.atomic(): + sid = connection.savepoint_ids[-1] + raise Exception("Oops") + + # This is expected to fail because the savepoint no longer exists. + connection.savepoint_rollback(sid)