From 47abebfd6d0ea56d7ac7a565f359992fde323177 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 17 Aug 2015 09:50:50 +0100 Subject: [PATCH 1/6] Add batched version of store.get_presence_state --- synapse/storage/presence.py | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/synapse/storage/presence.py b/synapse/storage/presence.py index 4f91a2b87c..f351b76a70 100644 --- a/synapse/storage/presence.py +++ b/synapse/storage/presence.py @@ -14,7 +14,7 @@ # limitations under the License. from ._base import SQLBaseStore -from synapse.util.caches.descriptors import cached +from synapse.util.caches.descriptors import cached, cachedList from twisted.internet import defer @@ -36,6 +36,7 @@ class PresenceStore(SQLBaseStore): desc="has_presence_state", ) + @cached() def get_presence_state(self, user_localpart): return self._simple_select_one( table="presence", @@ -44,6 +45,23 @@ class PresenceStore(SQLBaseStore): desc="get_presence_state", ) + @cachedList(get_presence_state.cache, list_name="user_localparts") + def get_presence_states(self, user_localparts): + def f(txn): + results = {} + for user_localpart in user_localparts: + results[user_localpart] = self._simple_select_one_txn( + txn, + table="presence", + keyvalues={"user_id": user_localpart}, + retcols=["state", "status_msg", "mtime"], + desc="get_presence_state", + ) + + return results + + return self.runInteraction("get_presence_states", f) + def set_presence_state(self, user_localpart, new_state): return self._simple_update_one( table="presence", From 1a9510bb84d79f6ff78d32390195bc97ed9a439e Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 17 Aug 2015 10:40:23 +0100 Subject: [PATCH 2/6] Implement a batched presence_handler.get_state and use it --- synapse/handlers/message.py | 18 ++++------- synapse/handlers/presence.py | 63 ++++++++++++++++++++++++++++++++++++ synapse/storage/presence.py | 6 ++-- 3 files changed, 73 insertions(+), 14 deletions(-) diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index 29e81085d1..f12465fa2c 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -460,20 +460,14 @@ class MessageHandler(BaseHandler): @defer.inlineCallbacks def get_presence(): - presence_defs = yield defer.DeferredList( - [ - presence_handler.get_state( - target_user=UserID.from_string(m.user_id), - auth_user=auth_user, - as_event=True, - check_auth=False, - ) - for m in room_members - ], - consumeErrors=True, + states = yield presence_handler.get_states( + target_users=[UserID.from_string(m.user_id) for m in room_members], + auth_user=auth_user, + as_event=True, + check_auth=False, ) - defer.returnValue([p for success, p in presence_defs if success]) + defer.returnValue(states.values()) receipts_handler = self.hs.get_handlers().receipts_handler diff --git a/synapse/handlers/presence.py b/synapse/handlers/presence.py index 341a516da2..33d76efe08 100644 --- a/synapse/handlers/presence.py +++ b/synapse/handlers/presence.py @@ -232,6 +232,69 @@ class PresenceHandler(BaseHandler): else: defer.returnValue(state) + @defer.inlineCallbacks + def get_states(self, target_users, auth_user, as_event=False, check_auth=True): + try: + local_users, remote_users = partitionbool( + target_users, + lambda u: self.hs.is_mine(u) + ) + + if check_auth: + for u in local_users: + visible = yield self.is_presence_visible( + observer_user=auth_user, + observed_user=u + ) + + if not visible: + raise SynapseError(404, "Presence information not visible") + + results = {} + if local_users: + for u in local_users: + if u in self._user_cachemap: + results[u] = self._user_cachemap[u].get_state() + + local_to_user = {u.localpart: u for u in local_users} + + states = yield self.store.get_presence_states( + [u.localpart for u in local_users if u not in results] + ) + + for local_part, state in states.items(): + res = {"presence": state["state"]} + if "status_msg" in state and state["status_msg"]: + res["status_msg"] = state["status_msg"] + results[local_to_user[local_part]] = res + + for u in remote_users: + # TODO(paul): Have remote server send us permissions set + results[u] = self._get_or_offline_usercache(u).get_state() + + for state in results.values(): + if "last_active" in state: + state["last_active_ago"] = int( + self.clock.time_msec() - state.pop("last_active") + ) + + if as_event: + for user, state in results.items(): + content = state + content["user_id"] = user.to_string() + + if "last_active" in content: + content["last_active_ago"] = int( + self._clock.time_msec() - content.pop("last_active") + ) + + results[user] = {"type": "m.presence", "content": content} + except: + logger.exception(":(") + raise + + defer.returnValue(results) + @defer.inlineCallbacks @log_function def set_state(self, target_user, auth_user, state): diff --git a/synapse/storage/presence.py b/synapse/storage/presence.py index f351b76a70..15d98198e2 100644 --- a/synapse/storage/presence.py +++ b/synapse/storage/presence.py @@ -50,13 +50,15 @@ class PresenceStore(SQLBaseStore): def f(txn): results = {} for user_localpart in user_localparts: - results[user_localpart] = self._simple_select_one_txn( + res = self._simple_select_one_txn( txn, table="presence", keyvalues={"user_id": user_localpart}, retcols=["state", "status_msg", "mtime"], - desc="get_presence_state", + allow_none=True, ) + if res: + results[user_localpart] = res return results From 2d97e65558f37fa0fbdd8d06545c32b410f1b5ed Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 17 Aug 2015 10:46:55 +0100 Subject: [PATCH 3/6] Remember to invalidate caches --- synapse/storage/presence.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/synapse/storage/presence.py b/synapse/storage/presence.py index 15d98198e2..9b136f3119 100644 --- a/synapse/storage/presence.py +++ b/synapse/storage/presence.py @@ -21,12 +21,15 @@ from twisted.internet import defer class PresenceStore(SQLBaseStore): def create_presence(self, user_localpart): - return self._simple_insert( + res = self._simple_insert( table="presence", values={"user_id": user_localpart}, desc="create_presence", ) + self.get_presence_state.invalidate((user_localpart,)) + return res + def has_presence_state(self, user_localpart): return self._simple_select_one( table="presence", @@ -65,7 +68,7 @@ class PresenceStore(SQLBaseStore): return self.runInteraction("get_presence_states", f) def set_presence_state(self, user_localpart, new_state): - return self._simple_update_one( + res = self._simple_update_one( table="presence", keyvalues={"user_id": user_localpart}, updatevalues={"state": new_state["state"], @@ -74,6 +77,9 @@ class PresenceStore(SQLBaseStore): desc="set_presence_state", ) + self.get_presence_state.invalidate((user_localpart,)) + return res + def allow_presence_visible(self, observed_localpart, observer_userid): return self._simple_insert( table="presence_allow_inbound", From f72ed6c6a353bad4a54cb695eae12d39fd41ad24 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 18 Aug 2015 10:29:49 +0100 Subject: [PATCH 4/6] Remove debug try/catch --- synapse/handlers/presence.py | 98 +++++++++++++++++------------------- 1 file changed, 47 insertions(+), 51 deletions(-) diff --git a/synapse/handlers/presence.py b/synapse/handlers/presence.py index 33d76efe08..b7664e30f8 100644 --- a/synapse/handlers/presence.py +++ b/synapse/handlers/presence.py @@ -234,64 +234,60 @@ class PresenceHandler(BaseHandler): @defer.inlineCallbacks def get_states(self, target_users, auth_user, as_event=False, check_auth=True): - try: - local_users, remote_users = partitionbool( - target_users, - lambda u: self.hs.is_mine(u) - ) + local_users, remote_users = partitionbool( + target_users, + lambda u: self.hs.is_mine(u) + ) - if check_auth: - for u in local_users: - visible = yield self.is_presence_visible( - observer_user=auth_user, - observed_user=u - ) - - if not visible: - raise SynapseError(404, "Presence information not visible") - - results = {} - if local_users: - for u in local_users: - if u in self._user_cachemap: - results[u] = self._user_cachemap[u].get_state() - - local_to_user = {u.localpart: u for u in local_users} - - states = yield self.store.get_presence_states( - [u.localpart for u in local_users if u not in results] + if check_auth: + for u in local_users: + visible = yield self.is_presence_visible( + observer_user=auth_user, + observed_user=u ) - for local_part, state in states.items(): - res = {"presence": state["state"]} - if "status_msg" in state and state["status_msg"]: - res["status_msg"] = state["status_msg"] - results[local_to_user[local_part]] = res + if not visible: + raise SynapseError(404, "Presence information not visible") - for u in remote_users: - # TODO(paul): Have remote server send us permissions set - results[u] = self._get_or_offline_usercache(u).get_state() + results = {} + if local_users: + for u in local_users: + if u in self._user_cachemap: + results[u] = self._user_cachemap[u].get_state() - for state in results.values(): - if "last_active" in state: - state["last_active_ago"] = int( - self.clock.time_msec() - state.pop("last_active") + local_to_user = {u.localpart: u for u in local_users} + + states = yield self.store.get_presence_states( + [u.localpart for u in local_users if u not in results] + ) + + for local_part, state in states.items(): + res = {"presence": state["state"]} + if "status_msg" in state and state["status_msg"]: + res["status_msg"] = state["status_msg"] + results[local_to_user[local_part]] = res + + for u in remote_users: + # TODO(paul): Have remote server send us permissions set + results[u] = self._get_or_offline_usercache(u).get_state() + + for state in results.values(): + if "last_active" in state: + state["last_active_ago"] = int( + self.clock.time_msec() - state.pop("last_active") + ) + + if as_event: + for user, state in results.items(): + content = state + content["user_id"] = user.to_string() + + if "last_active" in content: + content["last_active_ago"] = int( + self._clock.time_msec() - content.pop("last_active") ) - if as_event: - for user, state in results.items(): - content = state - content["user_id"] = user.to_string() - - if "last_active" in content: - content["last_active_ago"] = int( - self._clock.time_msec() - content.pop("last_active") - ) - - results[user] = {"type": "m.presence", "content": content} - except: - logger.exception(":(") - raise + results[user] = {"type": "m.presence", "content": content} defer.returnValue(results) From 776ee6d92b8672c36723b0b8dc9ae3467f34c08f Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 18 Aug 2015 10:30:07 +0100 Subject: [PATCH 5/6] Doc strings --- synapse/handlers/presence.py | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/synapse/handlers/presence.py b/synapse/handlers/presence.py index b7664e30f8..1177cbe51b 100644 --- a/synapse/handlers/presence.py +++ b/synapse/handlers/presence.py @@ -192,6 +192,20 @@ class PresenceHandler(BaseHandler): @defer.inlineCallbacks def get_state(self, target_user, auth_user, as_event=False, check_auth=True): + """Get the current presence state of the given user. + + Args: + target_user (UserID): The user whose presence we want + auth_user (UserID): The user requesting the presence, used for + checking if said user is allowed to see the persence of the + `target_user` + as_event (bool): Format the return as an event or not? + check_auth (bool): Perform the auth checks or not? + + Returns: + dict: The presence state of the `target_user`, whose format depends + on the `as_event` argument. + """ if self.hs.is_mine(target_user): if check_auth: visible = yield self.is_presence_visible( @@ -234,6 +248,20 @@ class PresenceHandler(BaseHandler): @defer.inlineCallbacks def get_states(self, target_users, auth_user, as_event=False, check_auth=True): + """A batched version of the `get_state` method that accepts a list of + `target_users` + + Args: + target_users (list): The list of UserID's whose presence we want + auth_user (UserID): The user requesting the presence, used for + checking if said user is allowed to see the persence of the + `target_users` + as_event (bool): Format the return as an event or not? + check_auth (bool): Perform the auth checks or not? + + Returns: + dict: A mapping from user -> presence_state + """ local_users, remote_users = partitionbool( target_users, lambda u: self.hs.is_mine(u) From 83eb627b5a50ef7cd8803f6c6fae9c5f5271bcb1 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 18 Aug 2015 10:33:11 +0100 Subject: [PATCH 6/6] More helpful variable names --- synapse/handlers/presence.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/synapse/handlers/presence.py b/synapse/handlers/presence.py index 1177cbe51b..2b103b48bb 100644 --- a/synapse/handlers/presence.py +++ b/synapse/handlers/presence.py @@ -268,10 +268,10 @@ class PresenceHandler(BaseHandler): ) if check_auth: - for u in local_users: + for user in local_users: visible = yield self.is_presence_visible( observer_user=auth_user, - observed_user=u + observed_user=user ) if not visible: @@ -279,9 +279,9 @@ class PresenceHandler(BaseHandler): results = {} if local_users: - for u in local_users: - if u in self._user_cachemap: - results[u] = self._user_cachemap[u].get_state() + for user in local_users: + if user in self._user_cachemap: + results[user] = self._user_cachemap[user].get_state() local_to_user = {u.localpart: u for u in local_users} @@ -295,9 +295,9 @@ class PresenceHandler(BaseHandler): res["status_msg"] = state["status_msg"] results[local_to_user[local_part]] = res - for u in remote_users: + for user in remote_users: # TODO(paul): Have remote server send us permissions set - results[u] = self._get_or_offline_usercache(u).get_state() + results[user] = self._get_or_offline_usercache(user).get_state() for state in results.values(): if "last_active" in state: