From 07d404170934db8bc3aa3ae8ac89ceb25cd2e9a1 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 8 Apr 2015 13:27:36 +0100 Subject: [PATCH 1/5] Fix bug where we didn't inform the NotificataionListeners about new rooms they have been subscribed to. This meant that the listeners didn't clean themselves up fully from all the dicts --- synapse/notifier.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/synapse/notifier.py b/synapse/notifier.py index 7121d659d0..b12b54353e 100644 --- a/synapse/notifier.py +++ b/synapse/notifier.py @@ -427,3 +427,6 @@ class Notifier(object): listeners = self.room_to_listeners.setdefault(room_id, set()) listeners |= new_listeners + + for l in new_listeners: + l.rooms.add(room_id) From 65f5e4e3e43ee471ee0a8c6989bbf60cb3be2c95 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 8 Apr 2015 13:31:06 +0100 Subject: [PATCH 2/5] Add paranoia checks to make sure that we evict stale NotificationListeners when we are about to process them --- synapse/notifier.py | 36 +++++++++++++++++++++++++++++++----- 1 file changed, 31 insertions(+), 5 deletions(-) diff --git a/synapse/notifier.py b/synapse/notifier.py index b12b54353e..ce9b0d2187 100644 --- a/synapse/notifier.py +++ b/synapse/notifier.py @@ -62,7 +62,8 @@ class _NotificationListener(object): self.rooms = rooms - self.pending_notifications = [] + def notified(self): + return self.deferred.called def notify(self, notifier, events, start_token, end_token): """ Inform whoever is listening about the new events. This will @@ -78,11 +79,15 @@ class _NotificationListener(object): except defer.AlreadyCalledError: pass + # Should the following be done be using intrusively linked lists? + # -- erikj + for room in self.rooms: lst = notifier.room_to_listeners.get(room, set()) lst.discard(self) notifier.user_to_listeners.get(self.user, set()).discard(self) + if self.appservice: notifier.appservice_to_listeners.get( self.appservice, set() @@ -161,10 +166,24 @@ class Notifier(object): room_source = self.event_sources.sources["room"] - listeners = self.room_to_listeners.get(room_id, set()).copy() + room_listeners = self.room_to_listeners.get(room_id, set()) + + # Remove any 'stale' listeners. + for l in room_listeners.copy(): + if l.notified(): + room_listeners.discard(l) + + listeners = room_listeners.copy() for user in extra_users: - listeners |= self.user_to_listeners.get(user, set()).copy() + user_listeners = self.user_to_listeners.get(user, set()) + + # Remove any 'stale' listeners. + for l in user_listeners.copy(): + if l.notified(): + user_listeners.discard(l) + + listeners |= user_listeners for appservice in self.appservice_to_listeners: # TODO (kegan): Redundant appservice listener checks? @@ -173,9 +192,16 @@ class Notifier(object): # receive *invites* for users they are interested in. Does this # make the room_to_listeners check somewhat obselete? if appservice.is_interested(event): - listeners |= self.appservice_to_listeners.get( + app_listeners = self.appservice_to_listeners.get( appservice, set() - ).copy() + ) + + # Remove any 'stale' listeners. + for l in app_listeners.copy(): + if l.notified(): + app_listeners.discard(l) + + listeners |= app_listeners logger.debug("on_new_room_event listeners %s", listeners) From 830d07db8278d773338fee94eb269eafd6b1b7fc Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 8 Apr 2015 13:40:20 +0100 Subject: [PATCH 3/5] Also perform paranoia checks in 'on_new_user_event' --- synapse/notifier.py | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/synapse/notifier.py b/synapse/notifier.py index ce9b0d2187..be78082021 100644 --- a/synapse/notifier.py +++ b/synapse/notifier.py @@ -252,10 +252,24 @@ class Notifier(object): listeners = set() for user in users: - listeners |= self.user_to_listeners.get(user, set()).copy() + user_listeners = self.user_to_listeners.get(user, set()) + + # Remove any 'stale' listeners. + for l in user_listeners.copy(): + if l.notified(): + user_listeners.discard(l) + + listeners |= user_listeners for room in rooms: - listeners |= self.room_to_listeners.get(room, set()).copy() + room_listeners = self.room_to_listeners.get(room, set()) + + # Remove any 'stale' listeners. + for l in room_listeners.copy(): + if l.notified(): + room_listeners.discard(l) + + listeners |= room_listeners @defer.inlineCallbacks def notify(listener): From 638be5a6b971bf961ee030d96245f296eb83e612 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 8 Apr 2015 13:58:32 +0100 Subject: [PATCH 4/5] Factor out loops into '_discard_if_notified' --- synapse/notifier.py | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/synapse/notifier.py b/synapse/notifier.py index be78082021..754569ebd2 100644 --- a/synapse/notifier.py +++ b/synapse/notifier.py @@ -169,9 +169,7 @@ class Notifier(object): room_listeners = self.room_to_listeners.get(room_id, set()) # Remove any 'stale' listeners. - for l in room_listeners.copy(): - if l.notified(): - room_listeners.discard(l) + _discard_if_notified(room_listeners) listeners = room_listeners.copy() @@ -179,9 +177,7 @@ class Notifier(object): user_listeners = self.user_to_listeners.get(user, set()) # Remove any 'stale' listeners. - for l in user_listeners.copy(): - if l.notified(): - user_listeners.discard(l) + _discard_if_notified(user_listeners) listeners |= user_listeners @@ -197,9 +193,7 @@ class Notifier(object): ) # Remove any 'stale' listeners. - for l in app_listeners.copy(): - if l.notified(): - app_listeners.discard(l) + _discard_if_notified(app_listeners) listeners |= app_listeners @@ -255,9 +249,7 @@ class Notifier(object): user_listeners = self.user_to_listeners.get(user, set()) # Remove any 'stale' listeners. - for l in user_listeners.copy(): - if l.notified(): - user_listeners.discard(l) + _discard_if_notified(user_listeners) listeners |= user_listeners @@ -265,9 +257,7 @@ class Notifier(object): room_listeners = self.room_to_listeners.get(room, set()) # Remove any 'stale' listeners. - for l in room_listeners.copy(): - if l.notified(): - room_listeners.discard(l) + _discard_if_notified(room_listeners) listeners |= room_listeners @@ -470,3 +460,12 @@ class Notifier(object): for l in new_listeners: l.rooms.add(room_id) + + +def _discard_if_notified(listener_set): + to_discard = set() + for l in listener_set: + if l.notified(): + to_discard.add(l) + + listener_set -= to_discard From 5bc41fe9f8d40ecf4070c7ffb8df635dcccb4efe Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 8 Apr 2015 14:01:22 +0100 Subject: [PATCH 5/5] Move comment into docstring --- synapse/notifier.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/synapse/notifier.py b/synapse/notifier.py index 754569ebd2..12573f3f59 100644 --- a/synapse/notifier.py +++ b/synapse/notifier.py @@ -168,7 +168,6 @@ class Notifier(object): room_listeners = self.room_to_listeners.get(room_id, set()) - # Remove any 'stale' listeners. _discard_if_notified(room_listeners) listeners = room_listeners.copy() @@ -176,7 +175,6 @@ class Notifier(object): for user in extra_users: user_listeners = self.user_to_listeners.get(user, set()) - # Remove any 'stale' listeners. _discard_if_notified(user_listeners) listeners |= user_listeners @@ -192,7 +190,6 @@ class Notifier(object): appservice, set() ) - # Remove any 'stale' listeners. _discard_if_notified(app_listeners) listeners |= app_listeners @@ -248,7 +245,6 @@ class Notifier(object): for user in users: user_listeners = self.user_to_listeners.get(user, set()) - # Remove any 'stale' listeners. _discard_if_notified(user_listeners) listeners |= user_listeners @@ -256,7 +252,6 @@ class Notifier(object): for room in rooms: room_listeners = self.room_to_listeners.get(room, set()) - # Remove any 'stale' listeners. _discard_if_notified(room_listeners) listeners |= room_listeners @@ -463,6 +458,8 @@ class Notifier(object): def _discard_if_notified(listener_set): + """Remove any 'stale' listeners from the given set. + """ to_discard = set() for l in listener_set: if l.notified():