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))