From 12228a0352331fe40f19282f9608402f17e690aa Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 9 Aug 2023 13:19:01 -0400 Subject: [PATCH 1/9] Use keyword arguments. --- synapse/handlers/presence.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/presence.py b/synapse/handlers/presence.py index cd7df0525f4f..f84bec0a160d 100644 --- a/synapse/handlers/presence.py +++ b/synapse/handlers/presence.py @@ -490,7 +490,9 @@ async def user_syncing( # what the spec wants: see comment in the BasePresenceHandler version # of this function. await self.set_state( - UserID.from_string(user_id), {"presence": presence_state}, True + UserID.from_string(user_id), + {"presence": presence_state}, + ignore_status_msg=True, ) curr_sync = self._user_to_num_current_syncs.get(user_id, 0) @@ -1010,7 +1012,9 @@ async def user_syncing( # updated always, which is not what the spec calls for, but synapse has done # this for... forever, I think. await self.set_state( - UserID.from_string(user_id), {"presence": presence_state}, True + UserID.from_string(user_id), + {"presence": presence_state}, + ignore_status_msg=True, ) # Retrieve the new state for the logic below. This should come from the # in-memory cache. From 63dc2547dd42a43b81a84016f3ef275f4c9b0e0b Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 9 Aug 2023 13:20:37 -0400 Subject: [PATCH 2/9] Return early if the call doesn't affect presence. --- synapse/handlers/presence.py | 92 ++++++++++++++++-------------------- 1 file changed, 42 insertions(+), 50 deletions(-) diff --git a/synapse/handlers/presence.py b/synapse/handlers/presence.py index f84bec0a160d..e706f1125f34 100644 --- a/synapse/handlers/presence.py +++ b/synapse/handlers/presence.py @@ -992,58 +992,51 @@ async def user_syncing( client that is being used by a user. presence_state: The presence state indicated in the sync request """ - # Override if it should affect the user's presence, if presence is - # disabled. - if not self.hs.config.server.use_presence: - affect_presence = False + if not affect_presence or not self._presence_enabled: + return _NullContextManager() - if affect_presence: - curr_sync = self.user_to_num_current_syncs.get(user_id, 0) - self.user_to_num_current_syncs[user_id] = curr_sync + 1 + curr_sync = self.user_to_num_current_syncs.get(user_id, 0) + self.user_to_num_current_syncs[user_id] = curr_sync + 1 - prev_state = await self.current_state_for_user(user_id) + prev_state = await self.current_state_for_user(user_id) - # If they're busy then they don't stop being busy just by syncing, - # so just update the last sync time. - if prev_state.state != PresenceState.BUSY: - # XXX: We set_state separately here and just update the last_active_ts above - # This keeps the logic as similar as possible between the worker and single - # process modes. Using set_state will actually cause last_active_ts to be - # updated always, which is not what the spec calls for, but synapse has done - # this for... forever, I think. - await self.set_state( - UserID.from_string(user_id), - {"presence": presence_state}, - ignore_status_msg=True, - ) - # Retrieve the new state for the logic below. This should come from the - # in-memory cache. - prev_state = await self.current_state_for_user(user_id) + # If they're busy then they don't stop being busy just by syncing, + # so just update the last sync time. + if prev_state.state != PresenceState.BUSY: + # XXX: We set_state separately here and just update the last_active_ts above + # This keeps the logic as similar as possible between the worker and single + # process modes. Using set_state will actually cause last_active_ts to be + # updated always, which is not what the spec calls for, but synapse has done + # this for... forever, I think. + await self.set_state( + UserID.from_string(user_id), + {"presence": presence_state}, + ignore_status_msg=True, + ) + # Retrieve the new state for the logic below. This should come from the + # in-memory cache. + prev_state = await self.current_state_for_user(user_id) - # To keep the single process behaviour consistent with worker mode, run the - # same logic as `update_external_syncs_row`, even though it looks weird. - if prev_state.state == PresenceState.OFFLINE: - await self._update_states( - [ - prev_state.copy_and_replace( - state=PresenceState.ONLINE, - last_active_ts=self.clock.time_msec(), - last_user_sync_ts=self.clock.time_msec(), - ) - ] - ) - # otherwise, set the new presence state & update the last sync time, - # but don't update last_active_ts as this isn't an indication that - # they've been active (even though it's probably been updated by - # set_state above) - else: - await self._update_states( - [ - prev_state.copy_and_replace( - last_user_sync_ts=self.clock.time_msec() - ) - ] - ) + # To keep the single process behaviour consistent with worker mode, run the + # same logic as `update_external_syncs_row`, even though it looks weird. + if prev_state.state == PresenceState.OFFLINE: + await self._update_states( + [ + prev_state.copy_and_replace( + state=PresenceState.ONLINE, + last_active_ts=self.clock.time_msec(), + last_user_sync_ts=self.clock.time_msec(), + ) + ] + ) + # otherwise, set the new presence state & update the last sync time, + # but don't update last_active_ts as this isn't an indication that + # they've been active (even though it's probably been updated by + # set_state above) + else: + await self._update_states( + [prev_state.copy_and_replace(last_user_sync_ts=self.clock.time_msec())] + ) async def _end() -> None: try: @@ -1065,8 +1058,7 @@ def _user_syncing() -> Generator[None, None, None]: try: yield finally: - if affect_presence: - run_in_background(_end) + run_in_background(_end) return _user_syncing() From 399afae0da2b1a48b1e69a9e9f7e14e409cf660f Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 9 Aug 2023 13:27:07 -0400 Subject: [PATCH 3/9] Remove duplicated code. --- synapse/handlers/presence.py | 33 +++++++++++---------------------- 1 file changed, 11 insertions(+), 22 deletions(-) diff --git a/synapse/handlers/presence.py b/synapse/handlers/presence.py index e706f1125f34..06fa75f69e3e 100644 --- a/synapse/handlers/presence.py +++ b/synapse/handlers/presence.py @@ -149,6 +149,15 @@ def __init__(self, hs: "HomeServer"): self._busy_presence_enabled = hs.config.experimental.msc3026_enabled + self.VALID_PRESENCE = ( + PresenceState.ONLINE, + PresenceState.UNAVAILABLE, + PresenceState.OFFLINE, + ) + + if self._busy_presence_enabled: + self.VALID_PRESENCE += (PresenceState.BUSY,) + active_presence = self.store.take_presence_startup_info() self.user_to_current_state = {state.user_id: state for state in active_presence} @@ -421,8 +430,6 @@ def __init__(self, hs: "HomeServer"): self.send_stop_syncing, UPDATE_SYNCING_USERS_MS ) - self._busy_presence_enabled = hs.config.experimental.msc3026_enabled - hs.get_reactor().addSystemEventTrigger( "before", "shutdown", @@ -603,16 +610,7 @@ async def set_state( """ presence = state["presence"] - valid_presence = ( - PresenceState.ONLINE, - PresenceState.UNAVAILABLE, - PresenceState.OFFLINE, - PresenceState.BUSY, - ) - - if presence not in valid_presence or ( - presence == PresenceState.BUSY and not self._busy_presence_enabled - ): + if presence not in self.VALID_PRESENCE: raise SynapseError(400, "Invalid presence state") user_id = target_user.to_string() @@ -1225,16 +1223,7 @@ async def set_state( status_msg = state.get("status_msg", None) presence = state["presence"] - valid_presence = ( - PresenceState.ONLINE, - PresenceState.UNAVAILABLE, - PresenceState.OFFLINE, - PresenceState.BUSY, - ) - - if presence not in valid_presence or ( - presence == PresenceState.BUSY and not self._busy_presence_enabled - ): + if presence not in self.VALID_PRESENCE: raise SynapseError(400, "Invalid presence state") # If presence is disabled, no-op From 12712146fec93d6872774251665e2a06b8a753c6 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 9 Aug 2023 13:29:28 -0400 Subject: [PATCH 4/9] Combine duplicate config flags. --- synapse/handlers/presence.py | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/synapse/handlers/presence.py b/synapse/handlers/presence.py index 06fa75f69e3e..b9361d0d35a4 100644 --- a/synapse/handlers/presence.py +++ b/synapse/handlers/presence.py @@ -141,6 +141,8 @@ def __init__(self, hs: "HomeServer"): self.state = hs.get_state_handler() self.is_mine_id = hs.is_mine_id + self._presence_enabled = hs.config.server.use_presence + self._federation = None if hs.should_send_federation(): self._federation = hs.get_federation_sender() @@ -404,8 +406,6 @@ def __init__(self, hs: "HomeServer"): self._presence_writer_instance = hs.config.worker.writers.presence[0] - self._presence_enabled = hs.config.server.use_presence - # Route presence EDUs to the right worker hs.get_federation_registry().register_instances_for_edu( EduTypes.PRESENCE, @@ -616,7 +616,7 @@ async def set_state( user_id = target_user.to_string() # If presence is disabled, no-op - if not self.hs.config.server.use_presence: + if not self._presence_enabled: return # Proxy request to instance that writes presence @@ -633,7 +633,7 @@ async def bump_presence_active_time(self, user: UserID) -> None: with the app. """ # If presence is disabled, no-op - if not self.hs.config.server.use_presence: + if not self._presence_enabled: return # Proxy request to instance that writes presence @@ -649,7 +649,6 @@ def __init__(self, hs: "HomeServer"): self.hs = hs self.wheel_timer: WheelTimer[str] = WheelTimer() self.notifier = hs.get_notifier() - self._presence_enabled = hs.config.server.use_presence federation_registry = hs.get_federation_registry() @@ -955,7 +954,7 @@ async def bump_presence_active_time(self, user: UserID) -> None: with the app. """ # If presence is disabled, no-op - if not self.hs.config.server.use_presence: + if not self._presence_enabled: return user_id = user.to_string() @@ -1227,7 +1226,7 @@ async def set_state( raise SynapseError(400, "Invalid presence state") # If presence is disabled, no-op - if not self.hs.config.server.use_presence: + if not self._presence_enabled: return user_id = target_user.to_string() From bb8b53f50a4970443cd93aaa403d87cc6031a1ac Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 9 Aug 2023 13:31:44 -0400 Subject: [PATCH 5/9] Remove unused code. --- synapse/handlers/presence.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/synapse/handlers/presence.py b/synapse/handlers/presence.py index b9361d0d35a4..f9624fe848d5 100644 --- a/synapse/handlers/presence.py +++ b/synapse/handlers/presence.py @@ -699,8 +699,6 @@ def __init__(self, hs: "HomeServer"): self._on_shutdown, ) - self._next_serial = 1 - # Keeps track of the number of *ongoing* syncs on this process. While # this is non zero a user will never go offline. self.user_to_num_current_syncs: Dict[str, int] = {} From c8a55bafbd36df166e33f77ad7a15dd50a409e5a Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 9 Aug 2023 13:34:43 -0400 Subject: [PATCH 6/9] Use wrap_as_background_process. --- synapse/handlers/presence.py | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/synapse/handlers/presence.py b/synapse/handlers/presence.py index f9624fe848d5..d527a4049d84 100644 --- a/synapse/handlers/presence.py +++ b/synapse/handlers/presence.py @@ -54,7 +54,10 @@ from synapse.events.presence_router import PresenceRouter from synapse.logging.context import run_in_background from synapse.metrics import LaterGauge -from synapse.metrics.background_process_metrics import run_as_background_process +from synapse.metrics.background_process_metrics import ( + run_as_background_process, + wrap_as_background_process, +) from synapse.replication.http.presence import ( ReplicationBumpPresenceActiveTime, ReplicationPresenceSetState, @@ -720,21 +723,16 @@ def __init__(self, hs: "HomeServer"): # Start a LoopingCall in 30s that fires every 5s. # The initial delay is to allow disconnected clients a chance to # reconnect before we treat them as offline. - def run_timeout_handler() -> Awaitable[None]: - return run_as_background_process( - "handle_presence_timeouts", self._handle_timeouts - ) - self.clock.call_later( - 30, self.clock.looping_call, run_timeout_handler, 5000 + 30, self.clock.looping_call, self._handle_timeouts, 5000 ) - def run_persister() -> Awaitable[None]: - return run_as_background_process( - "persist_presence_changes", self._persist_unpersisted_changes - ) - - self.clock.call_later(60, self.clock.looping_call, run_persister, 60 * 1000) + self.clock.call_later( + 60, + self.clock.looping_call, + self._persist_unpersisted_changes, + 60 * 1000, + ) LaterGauge( "synapse_handlers_presence_wheel_timer_size", @@ -780,6 +778,7 @@ async def _on_shutdown(self) -> None: ) logger.info("Finished _on_shutdown") + @wrap_as_background_process("persist_presence_changes") async def _persist_unpersisted_changes(self) -> None: """We periodically persist the unpersisted changes, as otherwise they may stack up and slow down shutdown times. @@ -895,6 +894,7 @@ async def _update_states( states, [destination] ) + @wrap_as_background_process("handle_presence_timeouts") async def _handle_timeouts(self) -> None: """Checks the presence of users that have timed out and updates as appropriate. From 5527f3c42b0ee84280be0a3952cfa864182ac9c9 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 9 Aug 2023 13:37:13 -0400 Subject: [PATCH 7/9] Newsfragment --- changelog.d/16092.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/16092.misc diff --git a/changelog.d/16092.misc b/changelog.d/16092.misc new file mode 100644 index 000000000000..b52080777105 --- /dev/null +++ b/changelog.d/16092.misc @@ -0,0 +1 @@ +Clean-up the presence code. From 27bb32a2b92547f68fa5e6de305c93c03b79eef4 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 9 Aug 2023 13:39:45 -0400 Subject: [PATCH 8/9] Lint --- synapse/handlers/presence.py | 1 - 1 file changed, 1 deletion(-) diff --git a/synapse/handlers/presence.py b/synapse/handlers/presence.py index d527a4049d84..ba3aa1b946b6 100644 --- a/synapse/handlers/presence.py +++ b/synapse/handlers/presence.py @@ -30,7 +30,6 @@ from typing import ( TYPE_CHECKING, Any, - Awaitable, Callable, Collection, Dict, From 37b1ec07dbc03313e13fef25c57bfd5bf4f64418 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 9 Aug 2023 14:28:29 -0400 Subject: [PATCH 9/9] More lint. --- synapse/handlers/presence.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/handlers/presence.py b/synapse/handlers/presence.py index ba3aa1b946b6..11dff724e665 100644 --- a/synapse/handlers/presence.py +++ b/synapse/handlers/presence.py @@ -153,7 +153,7 @@ def __init__(self, hs: "HomeServer"): self._busy_presence_enabled = hs.config.experimental.msc3026_enabled - self.VALID_PRESENCE = ( + self.VALID_PRESENCE: Tuple[str, ...] = ( PresenceState.ONLINE, PresenceState.UNAVAILABLE, PresenceState.OFFLINE,