From 4a1150b41d1312feacd4316b2691227f5a509ae9 Mon Sep 17 00:00:00 2001 From: SirAbhi13 Date: Tue, 7 Jun 2022 21:20:46 +0530 Subject: [PATCH] Fixed #33616 -- Allowed registering callbacks that can fail in transaction.on_commit(). Thanks David Wobrock and Mariusz Felisiak for reviews. --- django/db/backends/base/base.py | 38 ++++++++++++++++++++++++----- django/db/transaction.py | 4 ++-- django/test/testcases.py | 19 +++++++++++++-- docs/releases/4.2.txt | 4 ++++ docs/topics/db/transactions.txt | 23 ++++++++++++++---- tests/test_utils/tests.py | 26 ++++++++++++++++++++ tests/transaction_hooks/tests.py | 41 ++++++++++++++++++++++++++++++++ 7 files changed, 140 insertions(+), 15 deletions(-) diff --git a/django/db/backends/base/base.py b/django/db/backends/base/base.py index f8bea6fd23..66aff8335c 100644 --- a/django/db/backends/base/base.py +++ b/django/db/backends/base/base.py @@ -1,6 +1,7 @@ import _thread import copy import datetime +import logging import threading import time import warnings @@ -26,6 +27,8 @@ from django.utils.functional import cached_property NO_DB_ALIAS = "__no_db__" RAN_DB_VERSION_CHECK = set() +logger = logging.getLogger("django.db.backends.base") + # RemovedInDjango50Warning def timezone_constructor(tzname): @@ -417,7 +420,9 @@ class BaseDatabaseWrapper: # Remove any callbacks registered while this savepoint was active. self.run_on_commit = [ - (sids, func) for (sids, func) in self.run_on_commit if sid not in sids + (sids, func, robust) + for (sids, func, robust) in self.run_on_commit + if sid not in sids ] @async_unsafe @@ -723,12 +728,12 @@ class BaseDatabaseWrapper: ) return self.SchemaEditorClass(self, *args, **kwargs) - def on_commit(self, func): + def on_commit(self, func, robust=False): if not callable(func): raise TypeError("on_commit()'s callback must be a callable.") if self.in_atomic_block: # Transaction in progress; save for execution on commit. - self.run_on_commit.append((set(self.savepoint_ids), func)) + self.run_on_commit.append((set(self.savepoint_ids), func, robust)) elif not self.get_autocommit(): raise TransactionManagementError( "on_commit() cannot be used in manual transaction management" @@ -736,15 +741,36 @@ class BaseDatabaseWrapper: else: # No transaction in progress and in autocommit mode; execute # immediately. - func() + if robust: + try: + func() + except Exception as e: + logger.error( + f"Error calling {func.__qualname__} in on_commit() (%s).", + e, + exc_info=True, + ) + else: + func() def run_and_clear_commit_hooks(self): self.validate_no_atomic_block() current_run_on_commit = self.run_on_commit self.run_on_commit = [] while current_run_on_commit: - sids, func = current_run_on_commit.pop(0) - func() + _, func, robust = current_run_on_commit.pop(0) + if robust: + try: + func() + except Exception as e: + logger.error( + f"Error calling {func.__qualname__} in on_commit() during " + f"transaction (%s).", + e, + exc_info=True, + ) + else: + func() @contextmanager def execute_wrapper(self, wrapper): diff --git a/django/db/transaction.py b/django/db/transaction.py index b3c7b4bbaa..e57e1d0a6f 100644 --- a/django/db/transaction.py +++ b/django/db/transaction.py @@ -125,12 +125,12 @@ def mark_for_rollback_on_error(using=None): raise -def on_commit(func, using=None): +def on_commit(func, using=None, robust=False): """ Register `func` to be called when the current transaction is committed. If the current transaction is rolled back, `func` will not be called. """ - get_connection(using).on_commit(func) + get_connection(using).on_commit(func, robust) ################################# diff --git a/django/test/testcases.py b/django/test/testcases.py index 53d3838bb8..b0fd5e89a1 100644 --- a/django/test/testcases.py +++ b/django/test/testcases.py @@ -59,6 +59,8 @@ from django.utils.functional import classproperty from django.utils.version import PY310 from django.views.static import serve +logger = logging.getLogger("django.test") + __all__ = ( "TestCase", "TransactionTestCase", @@ -1510,10 +1512,23 @@ class TestCase(TransactionTestCase): finally: while True: callback_count = len(connections[using].run_on_commit) - for _, callback in connections[using].run_on_commit[start_count:]: + for _, callback, robust in connections[using].run_on_commit[ + start_count: + ]: callbacks.append(callback) if execute: - callback() + if robust: + try: + callback() + except Exception as e: + logger.error( + f"Error calling {callback.__qualname__} in " + f"on_commit() (%s).", + e, + exc_info=True, + ) + else: + callback() if callback_count == len(connections[using].run_on_commit): break diff --git a/docs/releases/4.2.txt b/docs/releases/4.2.txt index 71ba5c0dc6..e4394c5887 100644 --- a/docs/releases/4.2.txt +++ b/docs/releases/4.2.txt @@ -212,6 +212,10 @@ Models * :ref:`Registering lookups ` on :class:`~django.db.models.Field` instances is now supported. +* The new ``robust`` argument for :func:`~django.db.transaction.on_commit` + allows performing actions that can fail after a database transaction is + successfully committed. + Requests and Responses ~~~~~~~~~~~~~~~~~~~~~~ diff --git a/docs/topics/db/transactions.txt b/docs/topics/db/transactions.txt index b4c3fbba48..004f8351b8 100644 --- a/docs/topics/db/transactions.txt +++ b/docs/topics/db/transactions.txt @@ -297,7 +297,7 @@ include a `Celery`_ task, an email notification, or a cache invalidation. Django provides the :func:`on_commit` function to register callback functions that should be executed after a transaction is successfully committed: -.. function:: on_commit(func, using=None) +.. function:: on_commit(func, using=None, robust=False) Pass any function (that takes no arguments) to :func:`on_commit`:: @@ -325,6 +325,15 @@ If that hypothetical database write is instead rolled back (typically when an unhandled exception is raised in an :func:`atomic` block), your function will be discarded and never called. +It's sometimes useful to register callback functions that can fail. Passing +``robust=True`` allows the next functions to be executed even if the current +function throws an exception. All errors derived from Python's ``Exception`` +class are caught and logged to the ``django.db.backends.base`` logger. + +.. versionchanged:: 4.2 + + The ``robust`` argument was added. + Savepoints ---------- @@ -366,10 +375,14 @@ registered. Exception handling ------------------ -If one on-commit function within a given transaction raises an uncaught -exception, no later registered functions in that same transaction will run. -This is the same behavior as if you'd executed the functions sequentially -yourself without :func:`on_commit`. +If one on-commit function registered with ``robust=False`` within a given +transaction raises an uncaught exception, no later registered functions in that +same transaction will run. This is the same behavior as if you'd executed the +functions sequentially yourself without :func:`on_commit`. + +.. versionchanged:: 4.2 + + The ``robust`` argument was added. Timing of execution ------------------- diff --git a/tests/test_utils/tests.py b/tests/test_utils/tests.py index fb19d6e464..709060d76f 100644 --- a/tests/test_utils/tests.py +++ b/tests/test_utils/tests.py @@ -2285,6 +2285,32 @@ class CaptureOnCommitCallbacksTests(TestCase): self.assertEqual(callbacks, [branch_1, branch_2, leaf_3, leaf_1, leaf_2]) + def test_execute_robust(self): + class MyException(Exception): + pass + + def hook(): + self.callback_called = True + raise MyException("robust callback") + + with self.assertLogs("django.test", "ERROR") as cm: + with self.captureOnCommitCallbacks(execute=True) as callbacks: + transaction.on_commit(hook, robust=True) + + self.assertEqual(len(callbacks), 1) + self.assertIs(self.callback_called, True) + + log_record = cm.records[0] + self.assertEqual( + log_record.getMessage(), + "Error calling CaptureOnCommitCallbacksTests.test_execute_robust.." + "hook in on_commit() (robust callback).", + ) + self.assertIsNotNone(log_record.exc_info) + raised_exception = log_record.exc_info[1] + self.assertIsInstance(raised_exception, MyException) + self.assertEqual(str(raised_exception), "robust callback") + class DisallowedDatabaseQueriesTests(SimpleTestCase): def test_disallowed_database_connections(self): diff --git a/tests/transaction_hooks/tests.py b/tests/transaction_hooks/tests.py index 75cac5a3e9..938e92575f 100644 --- a/tests/transaction_hooks/tests.py +++ b/tests/transaction_hooks/tests.py @@ -43,6 +43,47 @@ class TestConnectionOnCommit(TransactionTestCase): self.do(1) self.assertDone([1]) + def test_robust_if_no_transaction(self): + def robust_callback(): + raise ForcedError("robust callback") + + with self.assertLogs("django.db.backends.base", "ERROR") as cm: + transaction.on_commit(robust_callback, robust=True) + self.do(1) + + self.assertDone([1]) + log_record = cm.records[0] + self.assertEqual( + log_record.getMessage(), + "Error calling TestConnectionOnCommit.test_robust_if_no_transaction." + ".robust_callback in on_commit() (robust callback).", + ) + self.assertIsNotNone(log_record.exc_info) + raised_exception = log_record.exc_info[1] + self.assertIsInstance(raised_exception, ForcedError) + self.assertEqual(str(raised_exception), "robust callback") + + def test_robust_transaction(self): + def robust_callback(): + raise ForcedError("robust callback") + + with self.assertLogs("django.db.backends", "ERROR") as cm: + with transaction.atomic(): + transaction.on_commit(robust_callback, robust=True) + self.do(1) + + self.assertDone([1]) + log_record = cm.records[0] + self.assertEqual( + log_record.getMessage(), + "Error calling TestConnectionOnCommit.test_robust_transaction.." + "robust_callback in on_commit() during transaction (robust callback).", + ) + self.assertIsNotNone(log_record.exc_info) + raised_exception = log_record.exc_info[1] + self.assertIsInstance(raised_exception, ForcedError) + self.assertEqual(str(raised_exception), "robust callback") + def test_delays_execution_until_after_transaction_commit(self): with transaction.atomic(): self.do(1)