From fc41d6085ba72341023fb8d7ddd5b7f0413292f4 Mon Sep 17 00:00:00 2001 From: Alex Vandiver Date: Sat, 10 Feb 2024 03:19:08 +0000 Subject: [PATCH] tornado: Split server restart events from web client reload events. --- api_docs/changelog.md | 8 ++++ version.py | 2 +- web/src/server_events_dispatch.js | 7 +++- web/tests/dispatch.test.js | 9 +++- web/tests/lib/events.js | 14 ++++--- zerver/lib/event_schema.py | 9 +++- zerver/lib/events.py | 13 +++--- zerver/openapi/zulip.yaml | 68 +++++++++++++++++++++++-------- zerver/tests/test_event_system.py | 43 +++++++++---------- zerver/tests/test_events.py | 10 ++--- zerver/tornado/event_queue.py | 17 ++++++-- 11 files changed, 135 insertions(+), 65 deletions(-) diff --git a/api_docs/changelog.md b/api_docs/changelog.md index c01622b33b..6825abc7d8 100644 --- a/api_docs/changelog.md +++ b/api_docs/changelog.md @@ -20,6 +20,14 @@ format used by the Zulip server that they are interacting with. ## Changes in Zulip 9.0 +**Feature level 240** + +* [`GET /events`](/api/get-events): The `restart` event no longer contains an + optional `immediate` flag. +* [`GET /events`](/api/get-events): A new `web_reload_client` event has been + added; it is used to signal to website-based clients that they should reload + their code. This was previously implied by the `restart` event. + Feature levels 238-239 are reserved for future use in 8.x maintenance releases. diff --git a/version.py b/version.py index ebcc908499..8384e52b96 100644 --- a/version.py +++ b/version.py @@ -33,7 +33,7 @@ DESKTOP_WARNING_VERSION = "5.9.3" # Changes should be accompanied by documentation explaining what the # new level means in api_docs/changelog.md, as well as "**Changes**" # entries in the endpoint's documentation in `zulip.yaml`. -API_FEATURE_LEVEL = 237 +API_FEATURE_LEVEL = 240 # Bump the minor PROVISION_VERSION to indicate that folks should provision # only when going from an old version of the code to a newer version. Bump diff --git a/web/src/server_events_dispatch.js b/web/src/server_events_dispatch.js index 05c328bcfd..a1639c46f1 100644 --- a/web/src/server_events_dispatch.js +++ b/web/src/server_events_dispatch.js @@ -165,7 +165,12 @@ export function dispatch_normal_event(event) { activity_ui.update_presence_info(event.user_id, event.presence, event.server_timestamp); break; - case "restart": { + case "restart": + realm.zulip_version = event.zulip_version; + realm.zulip_merge_base = event.zulip_merge_base; + break; + + case "web_reload_client": { const reload_options = { save_pointer: true, save_narrow: true, diff --git a/web/tests/dispatch.test.js b/web/tests/dispatch.test.js index 99c6fabb3e..3de9ecad23 100644 --- a/web/tests/dispatch.test.js +++ b/web/tests/dispatch.test.js @@ -747,8 +747,15 @@ run_test("realm_user", ({override}) => { assert.equal(removed_person.full_name, "translated: Unknown user"); }); -run_test("restart", ({override}) => { +run_test("restart", ({_override}) => { const event = event_fixtures.restart; + dispatch(event); + assert_same(realm.zulip_version, event.zulip_version); + assert_same(realm.zulip_merge_base, event.zulip_merge_base); +}); + +run_test("web_reload_client", ({override}) => { + const event = event_fixtures.web_reload_client; const stub = make_stub(); override(reload, "initiate", stub.f); dispatch(event); diff --git a/web/tests/lib/events.js b/web/tests/lib/events.js index 269b6e9187..747aca1bd5 100644 --- a/web/tests/lib/events.js +++ b/web/tests/lib/events.js @@ -604,11 +604,10 @@ exports.fixtures = { restart: { type: "restart", - zulip_version: "4.0-dev+git", - zulip_merge_base: "", - zulip_feature_level: 55, - server_generation: 2, - immediate: true, + zulip_version: "9.0-dev-753-gced3e85da9", + zulip_merge_base: "9.0-dev-743-g54053c1d28", + zulip_feature_level: 237, + server_generation: 1707511515, }, scheduled_messages__add: { @@ -1084,4 +1083,9 @@ exports.fixtures = { last_updated: fake_now, visibility_policy: 1, }, + + web_reload_client: { + type: "web_reload_client", + immediate: true, + }, }; diff --git a/zerver/lib/event_schema.py b/zerver/lib/event_schema.py index b54a008353..a64c4e2074 100644 --- a/zerver/lib/event_schema.py +++ b/zerver/lib/event_schema.py @@ -1253,11 +1253,18 @@ restart_event = event_dict_type( ("zulip_merge_base", str), ("zulip_feature_level", int), ("server_generation", int), - ("immediate", bool), ] ) check_restart_event = make_checker(restart_event) +web_reload_client_event = event_dict_type( + required_keys=[ + ("type", Equals("web_reload_client")), + ("immediate", bool), + ] +) +check_web_reload_client_event = make_checker(web_reload_client_event) + scheduled_message_fields = DictType( required_keys=[ ("scheduled_message_id", int), diff --git a/zerver/lib/events.py b/zerver/lib/events.py index 487a895a62..c984dceb81 100644 --- a/zerver/lib/events.py +++ b/zerver/lib/events.py @@ -91,9 +91,10 @@ from zerver.tornado.django_api import get_user_events, request_event_queue from zproject.backends import email_auth_enabled, password_auth_enabled -class RestartEventError(Exception): - """ - Special error for handling restart events in apply_events. +class WebReloadClientError(Exception): + """Special error for handling web_reload_client events in + apply_events. + """ @@ -711,8 +712,8 @@ def apply_events( user_list_incomplete: bool, ) -> None: for event in events: - if event["type"] == "restart": - raise RestartEventError + if event["type"] == "web_reload_client": + raise WebReloadClientError if fetch_event_types is not None and event["type"] not in fetch_event_types: # TODO: continuing here is not, most precisely, correct. # In theory, an event of one type, e.g. `realm_user`, @@ -1652,7 +1653,7 @@ def do_events_register( linkifier_url_template=linkifier_url_template, user_list_incomplete=user_list_incomplete, ) - except RestartEventError: + except WebReloadClientError: # This represents a rare race condition, where Tornado # restarted (and sent `restart` events) while we were waiting # for fetch_initial_state_data to return. To avoid the client diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index 1a3f2576d0..89f4a8c11f 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -4000,10 +4000,15 @@ paths: for the user is restarted; in particular, this will always happen when the Zulip server is upgraded. - Clients can use this event to know when they should get a new - event queue after a server upgrade. Clients doing so must implement - a random delay strategy to spread such restarts over 10 minutes or - more to avoid creating a synchronized thundering herd effect. + Clients should use this event to update their tracking of the + server's capabilities, and to decide if they wish to get a new + event queue after a server upgrade. Clients doing so must + implement a random delay strategy to spread such restarts over 5 + minutes or more to avoid creating a synchronized thundering herd + effect. + + **Changes**: Removed the `immediate` flag, which was only used by + web clients in development, in Zulip 9.0 (feature level 240). properties: id: $ref: "#/components/schemas/EventIdSchema" @@ -4034,20 +4039,15 @@ paths: The [Zulip feature level](/api/changelog) of the server after the restart. - Clients can safely avoid refetching their state and - creating a new event queue when the API feature level has not - changed, or when they know the specific feature level change - is not relevant to the client (E.g. it just adds a new endpoint - that the client doesn't use). + Clients should use this to update their tracking of the + server's capabilities, and may choose to refetch their state + and create a new event queue when the API feature level has + changed in a way that the client finds significant. Clients + choosing to do so must implement a random delay strategy to + spread such restarts over 5 or more minutes to avoid creating + a synchronized thundering herd effect. **Changes**: New in Zulip 4.0 (feature level 59). - immediate: - type: boolean - description: | - Whether the client should fetch a new event queue immediately, - rather than using a backoff strategy to avoid thundering herds. - A Zulip development server uses this parameter to reload - clients immediately. server_generation: type: integer description: | @@ -4056,13 +4056,47 @@ paths: example: { "id": 0, - "immediate": true, "server_generation": 1619334181, "type": "restart", "zulip_feature_level": 57, "zulip_version": "5.0-dev-1650-gc3fd37755f", "zulip_merge_base": "5.0-dev-1646-gea6b21cd8c", } + - type: object + description: | + An event which signals the official Zulip web/desktop app to update, + by reloading the page and fetching a new queue; this will generally + follow a `restart` event. Clients which do not obtain their code + from the server (e.g. mobile and terminal clients, which store their + code locally) should ignore this event. + + Clients choosing to reload the application must implement a random + delay strategy to spread such restarts over 5 or more minutes to + avoid creating a synchronized thundering herd effect. + + **Changes**: New in Zulip 9.0 (feature level 240). + properties: + id: + $ref: "#/components/schemas/EventIdSchema" + type: + allOf: + - $ref: "#/components/schemas/EventTypeSchema" + - enum: + - web_reload_client + immediate: + type: boolean + description: | + Whether the client should fetch a new event queue immediately, + rather than using a backoff strategy to avoid thundering herds. + A Zulip development server uses this parameter to reload + clients immediately. + additionalProperties: false + example: + { + "id": 0, + "type": "web_reload_client", + "immediate": true, + } - type: object additionalProperties: false description: | diff --git a/zerver/tests/test_event_system.py b/zerver/tests/test_event_system.py index ea58f0a8ea..924a524662 100644 --- a/zerver/tests/test_event_system.py +++ b/zerver/tests/test_event_system.py @@ -10,13 +10,12 @@ from django.test import override_settings from django.utils.timezone import now as timezone_now from typing_extensions import override -from version import API_FEATURE_LEVEL, ZULIP_MERGE_BASE, ZULIP_VERSION from zerver.actions.custom_profile_fields import try_update_realm_custom_profile_field from zerver.actions.message_send import check_send_message from zerver.actions.presence import do_update_user_presence from zerver.actions.user_settings import do_change_user_setting from zerver.actions.users import do_change_user_role -from zerver.lib.event_schema import check_restart_event +from zerver.lib.event_schema import check_web_reload_client_event from zerver.lib.events import fetch_initial_state_data from zerver.lib.exceptions import AccessDeniedError from zerver.lib.request import RequestVariableMissingError @@ -38,7 +37,7 @@ from zerver.tornado.event_queue import ( clear_client_event_queues_for_testing, get_client_info_for_message_event, process_message_event, - send_restart_events, + send_web_reload_client_events, ) from zerver.tornado.exceptions import BadEventQueueIdError from zerver.tornado.views import get_events, get_events_backend @@ -1100,7 +1099,7 @@ class ClientDescriptorsTest(ZulipTestCase): ) -class RestartEventsTest(ZulipTestCase): +class WebReloadClientsTest(ZulipTestCase): def tornado_call( self, view_func: Callable[[HttpRequest, UserProfile], HttpResponse], @@ -1122,7 +1121,7 @@ class RestartEventsTest(ZulipTestCase): ) return view_func(request, user_profile) - def test_restart(self) -> None: + def test_web_reload_clients(self) -> None: hamlet = self.example_user("hamlet") realm = hamlet.realm @@ -1141,28 +1140,24 @@ class RestartEventsTest(ZulipTestCase): ) client = allocate_client_descriptor(queue_data) - send_restart_events(immediate=True) + send_web_reload_client_events() self.assert_length(client.event_queue.queue, 1) - restart_event = client.event_queue.queue[0] + reload_event = client.event_queue.queue[0] - check_restart_event("restart_event", restart_event) + check_web_reload_client_event("web_reload_client_event", reload_event) self.assertEqual( - restart_event, + reload_event, dict( - type="restart", - zulip_version=ZULIP_VERSION, - zulip_merge_base=ZULIP_MERGE_BASE, - zulip_feature_level=API_FEATURE_LEVEL, - server_generation=settings.SERVER_GENERATION, - immediate=True, + type="web_reload_client", + immediate=False, id=0, ), ) - def test_restart_event_recursive_call_logic(self) -> None: + def test_web_reload_client_event_recursive_call_logic(self) -> None: # This is a test for a subtle corner case; see the comments - # around RestartEventError for details. + # around WebReloadClientError for details. hamlet = self.example_user("hamlet") realm = hamlet.realm @@ -1182,16 +1177,16 @@ class RestartEventsTest(ZulipTestCase): ) client = allocate_client_descriptor(queue_data) - # Add a restart event to it. - send_restart_events(immediate=True) + # Add a reload event to it. + send_web_reload_client_events() - # Make a second queue after the restart events were sent. + # Make a second queue after the reload events were sent. second_client = allocate_client_descriptor(queue_data) - # Fetch the restart event just sent above, without removing it + # Fetch the reload event just sent above, without removing it # from the queue. We will use this as a mock return value in # get_user_events. - restart_event = orjson.loads( + reload_event = orjson.loads( self.tornado_call( get_events_backend, hamlet, @@ -1209,7 +1204,7 @@ class RestartEventsTest(ZulipTestCase): # Now the tricky part: We call events_register_backend, # arranging it so that the first `get_user_events` call - # returns our restart event (triggering the recursive + # returns our reload event (triggering the recursive # behavior), but the second (with a new queue) returns no # events. # @@ -1219,7 +1214,7 @@ class RestartEventsTest(ZulipTestCase): with mock.patch( "zerver.lib.events.request_event_queue", side_effect=[client.event_queue.id, second_client.event_queue.id], - ), mock.patch("zerver.lib.events.get_user_events", side_effect=[restart_event, []]): + ), mock.patch("zerver.lib.events.get_user_events", side_effect=[reload_event, []]): self.tornado_call( events_register_backend, hamlet, diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index 30462754e3..533fec669e 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -201,7 +201,7 @@ from zerver.lib.event_schema import ( check_user_topic, ) from zerver.lib.events import ( - RestartEventError, + WebReloadClientError, apply_events, fetch_initial_state_data, post_process_state, @@ -251,7 +251,7 @@ from zerver.tornado.event_queue import ( allocate_client_descriptor, clear_client_event_queues_for_testing, create_heartbeat_event, - send_restart_events, + send_web_reload_client_events, ) from zerver.views.realm_playgrounds import access_playground_by_id @@ -3456,9 +3456,9 @@ class NormalActionsTest(BaseAction): events = self.verify_action(lambda: do_set_zoom_token(self.user_profile, None)) check_has_zoom_token("events[0]", events[0], value=False) - def test_restart_event(self) -> None: - with self.assertRaises(RestartEventError): - self.verify_action(lambda: send_restart_events(immediate=True)) + def test_web_reload_client_event(self) -> None: + with self.assertRaises(WebReloadClientError): + self.verify_action(lambda: send_web_reload_client_events()) def test_display_setting_event_not_sent(self) -> None: events = self.verify_action( diff --git a/zerver/tornado/event_queue.py b/zerver/tornado/event_queue.py index 20aea8f6c4..e7c9d38fdf 100644 --- a/zerver/tornado/event_queue.py +++ b/zerver/tornado/event_queue.py @@ -629,7 +629,7 @@ def load_event_queues(port: int) -> None: ) -def send_restart_events(immediate: bool = False) -> None: +def send_restart_events() -> None: event: Dict[str, Any] = dict( type="restart", zulip_version=ZULIP_VERSION, @@ -637,8 +637,16 @@ def send_restart_events(immediate: bool = False) -> None: zulip_feature_level=API_FEATURE_LEVEL, server_generation=settings.SERVER_GENERATION, ) - if immediate: - event["immediate"] = True + for client in clients.values(): + if client.accepts_event(event): + client.add_event(event) + + +def send_web_reload_client_events(immediate: bool = False) -> None: + event: Dict[str, Any] = dict( + type="web_reload_client", + immediate=immediate, + ) for client in clients.values(): if client.accepts_event(event): client.add_event(event) @@ -656,7 +664,8 @@ async def setup_event_queue(server: tornado.httpserver.HTTPServer, port: int) -> pc = tornado.ioloop.PeriodicCallback(lambda: gc_event_queues(port), EVENT_QUEUE_GC_FREQ_MSECS) pc.start() - send_restart_events(immediate=settings.DEVELOPMENT) + send_restart_events() + send_web_reload_client_events(immediate=settings.DEVELOPMENT) def fetch_events(