From e763d065a345f0af24f9b0ef8a094aa9f3537a03 Mon Sep 17 00:00:00 2001 From: Prakhar Pratyush Date: Fri, 13 Sep 2024 14:06:58 +0530 Subject: [PATCH] django_api: Rename 'send_event' to 'send_event_rollback_unsafe'. This commit renames the 'send_event' function to 'send_event_rollback_unsafe' to reflect the fact that it doesn't wait for the db transaction (within which it gets called, if any) to commit and sends event irrespective of commit or rollback. In most of the cases we don't want to send event in the case of rollbacks, so the caller should be aware that calling the function directly is rollback unsafe. --- zerver/actions/message_flags.py | 4 ++-- zerver/actions/presence.py | 4 ++-- zerver/actions/typing.py | 6 +++--- zerver/actions/users.py | 4 ++-- zerver/lib/create_user.py | 2 +- zerver/lib/test_classes.py | 10 +++++----- zerver/models/realms.py | 2 +- zerver/tests/test_events.py | 10 +++++----- zerver/tests/test_message_delete.py | 6 +++--- zerver/tests/test_reactions.py | 6 +++--- zerver/tests/test_submessage.py | 6 +++--- zerver/tornado/django_api.py | 16 +++++++++++----- 12 files changed, 41 insertions(+), 35 deletions(-) diff --git a/zerver/actions/message_flags.py b/zerver/actions/message_flags.py index b58ca637be..71e80e5d43 100644 --- a/zerver/actions/message_flags.py +++ b/zerver/actions/message_flags.py @@ -20,7 +20,7 @@ from zerver.lib.stream_subscription import get_subscribed_stream_recipient_ids_f from zerver.lib.topic import filter_by_topic_name_via_message from zerver.lib.user_message import DEFAULT_HISTORICAL_FLAGS, create_historical_user_messages from zerver.models import Message, Recipient, UserMessage, UserProfile -from zerver.tornado.django_api import send_event, send_event_on_commit +from zerver.tornado.django_api import send_event_on_commit, send_event_rollback_unsafe @dataclass @@ -96,7 +96,7 @@ def do_mark_all_as_read(user_profile: UserProfile, *, timeout: float | None = No all=True, ) ) - send_event(user_profile.realm, event, [user_profile.id]) + send_event_rollback_unsafe(user_profile.realm, event, [user_profile.id]) return count diff --git a/zerver/actions/presence.py b/zerver/actions/presence.py index d5cc9378e7..91e5792db3 100644 --- a/zerver/actions/presence.py +++ b/zerver/actions/presence.py @@ -15,7 +15,7 @@ from zerver.lib.users import get_user_ids_who_can_access_user from zerver.models import Client, UserPresence, UserProfile from zerver.models.clients import get_client from zerver.models.users import active_user_ids -from zerver.tornado.django_api import send_event +from zerver.tornado.django_api import send_event_rollback_unsafe logger = logging.getLogger(__name__) @@ -72,7 +72,7 @@ def send_presence_changed( server_timestamp=time.time(), presence={presence_dict["client"]: presence_dict}, ) - send_event(user_profile.realm, event, user_ids) + send_event_rollback_unsafe(user_profile.realm, event, user_ids) def consolidate_client(client: Client) -> Client: diff --git a/zerver/actions/typing.py b/zerver/actions/typing.py index 1300da882b..43f63de32d 100644 --- a/zerver/actions/typing.py +++ b/zerver/actions/typing.py @@ -5,7 +5,7 @@ from zerver.lib.exceptions import JsonableError from zerver.lib.stream_subscription import get_active_subscriptions_for_stream_id from zerver.models import Realm, Stream, UserProfile from zerver.models.users import get_user_by_id_in_realm_including_cross_realm -from zerver.tornado.django_api import send_event +from zerver.tornado.django_api import send_event_rollback_unsafe def do_send_typing_notification( @@ -32,7 +32,7 @@ def do_send_typing_notification( if user.is_active and user.receives_typing_notifications ] - send_event(realm, event, user_ids_to_notify) + send_event_rollback_unsafe(realm, event, user_ids_to_notify) # check_send_typing_notification: @@ -98,4 +98,4 @@ def do_send_stream_typing_notification( .values_list("user_profile_id", flat=True) ) - send_event(sender.realm, event, user_ids_to_notify) + send_event_rollback_unsafe(sender.realm, event, user_ids_to_notify) diff --git a/zerver/actions/users.py b/zerver/actions/users.py index 314d5aa45e..4962770df6 100644 --- a/zerver/actions/users.py +++ b/zerver/actions/users.py @@ -123,8 +123,8 @@ def do_delete_user_preserving_messages(user_profile: UserProfile) -> None: conversations that they may have participated in. Not recommended for general use due to the following quirks: - * Does not live-update other clients via `send_event` about the - user's new name, email, or other attributes. + * Does not live-update other clients via `send_event_on_commit` + about the user's new name, email, or other attributes. * Not guaranteed to clear caches containing the deleted users. The temporary user may be visible briefly in caches due to the UserProfile model's post_save hook. diff --git a/zerver/lib/create_user.py b/zerver/lib/create_user.py index 72fe62d811..cc296426d6 100644 --- a/zerver/lib/create_user.py +++ b/zerver/lib/create_user.py @@ -24,7 +24,7 @@ def copy_default_settings( settings_source: UserProfile | RealmUserDefault, target_profile: UserProfile ) -> None: # Important note: Code run from here to configure the user's - # settings should not call send_event, as that would cause clients + # settings should not send events, as that would cause clients # to throw an exception (we haven't sent the realm_user/add event # yet, so that event will include the updated details of target_profile). # diff --git a/zerver/lib/test_classes.py b/zerver/lib/test_classes.py index 1f8c4b7ba1..ae77de0643 100644 --- a/zerver/lib/test_classes.py +++ b/zerver/lib/test_classes.py @@ -2052,11 +2052,11 @@ class ZulipTestCase(ZulipTestCaseMixin, TestCase): # So explicitly change parameter name to 'notice' to work around this problem with ( mock.patch("zerver.tornado.event_queue.process_notification", lst.append), - # Some `send_event` calls need to be executed only after the current transaction - # commits (using `on_commit` hooks). Because the transaction in Django tests never - # commits (rather, gets rolled back after the test completes), such events would - # never be sent in tests, and we would be unable to verify them. Hence, we use - # this helper to make sure the `send_event` calls actually run. + # Some `send_event_rollback_unsafe` calls need to be executed only after the + # current transaction commits (using `on_commit` hooks). Because the transaction + # in Django tests never commits (rather, gets rolled back after the test completes), + # such events would never be sent in tests, and we would be unable to verify them. + # Hence, we use this helper to make sure the `send_event_rollback_unsafe` calls actually run. self.captureOnCommitCallbacks(execute=True), ): yield lst diff --git a/zerver/models/realms.py b/zerver/models/realms.py index a2190f5422..cdf1f6a7cb 100644 --- a/zerver/models/realms.py +++ b/zerver/models/realms.py @@ -856,7 +856,7 @@ class Realm(models.Model): # type: ignore[django-manager-missing] # django-stub self, include_realm_owners: bool = True ) -> QuerySet["UserProfile"]: """Use this in contexts where we want administrative users as well as - bots with administrator privileges, like send_event calls for + bots with administrator privileges, like send_event_on_commit calls for notifications to all administrator users. """ if include_realm_owners: diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index 15fba21367..2b933c6925 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -252,7 +252,7 @@ from zerver.models.realm_audit_logs import AuditLogEventType from zerver.models.streams import get_stream from zerver.models.users import get_user_by_delivery_email from zerver.openapi.openapi import validate_against_openapi_schema -from zerver.tornado.django_api import send_event +from zerver.tornado.django_api import send_event_rollback_unsafe from zerver.tornado.event_queue import ( allocate_client_descriptor, clear_client_event_queues_for_testing, @@ -267,7 +267,7 @@ from zerver.worker.thumbnail import ensure_thumbnails class BaseAction(ZulipTestCase): """Core class for verifying the apply_event race handling logic as - well as the event formatting logic of any function using send_event. + well as the event formatting logic of any function using send_event_rollback_unsafe. See https://zulip.readthedocs.io/en/latest/subsystems/events-system.html#testing for extensive design details for this testing system. @@ -355,8 +355,8 @@ class BaseAction(ZulipTestCase): events: list[dict[str, Any]] = [] - # We want even those `send_event` calls which have been hooked to - # `transaction.on_commit` to execute in tests. + # We want even those `send_event_rollback_unsafe` calls which have + # been hooked to `transaction.on_commit` to execute in tests. # See the comment in `ZulipTestCase.capture_send_event_calls`. with self.captureOnCommitCallbacks(execute=True): yield events @@ -1174,7 +1174,7 @@ class NormalActionsTest(BaseAction): def test_heartbeat_event(self) -> None: with self.verify_action(state_change_expected=False) as events: - send_event( + send_event_rollback_unsafe( self.user_profile.realm, create_heartbeat_event(), [self.user_profile.id], diff --git a/zerver/tests/test_message_delete.py b/zerver/tests/test_message_delete.py index 982110fb9a..29c4a1f482 100644 --- a/zerver/tests/test_message_delete.py +++ b/zerver/tests/test_message_delete.py @@ -488,9 +488,9 @@ class DeleteMessageTest(ZulipTestCase): def test_delete_event_sent_after_transaction_commits(self) -> None: """ - Tests that `send_event` is hooked to `transaction.on_commit`. This is important, because - we don't want to end up holding locks on message rows for too long if the event queue runs - into a problem. + Tests that `send_event_rollback_unsafe` is hooked to `transaction.on_commit`. + This is important, because we don't want to end up holding locks on message rows + for too long if the event queue runs into a problem. """ hamlet = self.example_user("hamlet") self.send_stream_message(hamlet, "Denmark") diff --git a/zerver/tests/test_reactions.py b/zerver/tests/test_reactions.py index f7afc23796..d69a00d107 100644 --- a/zerver/tests/test_reactions.py +++ b/zerver/tests/test_reactions.py @@ -1124,9 +1124,9 @@ class ReactionAPIEventTest(EmojiReactionBase): def test_events_sent_after_transaction_commits(self) -> None: """ - Tests that `send_event` is hooked to `transaction.on_commit`. This is important, because - we don't want to end up holding locks on message rows for too long if the event queue runs - into a problem. + Tests that `send_event_rollback_unsafe` is hooked to `transaction.on_commit`. + This is important, because we don't want to end up holding locks on message rows + for too long if the event queue runs into a problem. """ hamlet = self.example_user("hamlet") self.send_stream_message(hamlet, "Denmark") diff --git a/zerver/tests/test_submessage.py b/zerver/tests/test_submessage.py index dbdeee263d..182180900d 100644 --- a/zerver/tests/test_submessage.py +++ b/zerver/tests/test_submessage.py @@ -187,9 +187,9 @@ class TestBasics(ZulipTestCase): def test_submessage_event_sent_after_transaction_commits(self) -> None: """ - Tests that `send_event` is hooked to `transaction.on_commit`. This is important, because - we don't want to end up holding locks on message rows for too long if the event queue runs - into a problem. + Tests that `send_event_rollback_unsafe` is hooked to `transaction.on_commit`. + This is important, because we don't want to end up holding locks on message rows + for too long if the event queue runs into a problem. """ hamlet = self.example_user("hamlet") message_id = self.send_stream_message(hamlet, "Denmark") diff --git a/zerver/tornado/django_api.py b/zerver/tornado/django_api.py index fbff9253eb..b9bc56b0cc 100644 --- a/zerver/tornado/django_api.py +++ b/zerver/tornado/django_api.py @@ -170,15 +170,21 @@ def send_notification_http(port: int, data: Mapping[str, Any]) -> None: # The core function for sending an event from Django to Tornado (which # will then push it to web and mobile clients for the target users). -# By convention, send_event should only be called from -# zerver/actions/*.py, which helps make it easy to find event -# generation code. +# +# One should generally use `send_event_on_commit` unless there's a strong +# reason to use `send_event_rollback_unsafe` directly, as it doesn't wait +# for the db transaction (within which it gets called, if any) to commit +# and sends event irrespective of commit or rollback. +# +# By convention, `send_event_rollback_unsafe` / `send_event_on_commit` +# should only be called from zerver/actions/*.py, which helps make it +# easy to find event generation code. # # Every call point should be covered by a test in `test_events.py`, # with the schema verified in `zerver/lib/event_schema.py`. # # See https://zulip.readthedocs.io/en/latest/subsystems/events-system.html -def send_event( +def send_event_rollback_unsafe( realm: Realm, event: Mapping[str, Any], users: Iterable[int] | Iterable[Mapping[str, Any]] ) -> None: """`users` is a list of user IDs, or in some special cases like message @@ -203,4 +209,4 @@ def send_event( def send_event_on_commit( realm: Realm, event: Mapping[str, Any], users: Iterable[int] | Iterable[Mapping[str, Any]] ) -> None: - transaction.on_commit(lambda: send_event(realm, event, users)) + transaction.on_commit(lambda: send_event_rollback_unsafe(realm, event, users))