From 773e1c6d68223c787bff1da78baef519a70f8c3d Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 3 Apr 2017 15:14:11 +0100 Subject: [PATCH 1/6] Remove spurious @preserve_fn decorators Remove `@preserve_fn` decorators on `on_new_room_event`, `_notify_pending_new_room_events`, `_on_new_room_event`, `on_new_event`, and `on_new_replication_data` - none of these functions return a deferred, and the decorator does nothing unless the wrapped function returns a deferred, so the decorator was a no-op. --- synapse/notifier.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/synapse/notifier.py b/synapse/notifier.py index 7eeba6d28..3b206bb96 100644 --- a/synapse/notifier.py +++ b/synapse/notifier.py @@ -202,7 +202,6 @@ class Notifier(object): lambda: len(self.user_to_user_stream), ) - @preserve_fn def on_new_room_event(self, event, room_stream_id, max_room_stream_id, extra_users=[]): """ Used by handlers to inform the notifier something has happened @@ -224,7 +223,6 @@ class Notifier(object): self.notify_replication() - @preserve_fn def _notify_pending_new_room_events(self, max_room_stream_id): """Notify for the room events that were queued waiting for a previous event to be persisted. @@ -242,7 +240,6 @@ class Notifier(object): else: self._on_new_room_event(event, room_stream_id, extra_users) - @preserve_fn def _on_new_room_event(self, event, room_stream_id, extra_users=[]): """Notify any user streams that are interested in this room event""" # poke any interested application service. @@ -260,7 +257,6 @@ class Notifier(object): rooms=[event.room_id], ) - @preserve_fn def on_new_event(self, stream_key, new_token, users=[], rooms=[]): """ Used to inform listeners that something has happend event wise. @@ -287,7 +283,6 @@ class Notifier(object): self.notify_replication() - @preserve_fn def on_new_replication_data(self): """Used to inform replication listeners that something has happend without waking up any of the normal user event streams""" From e2eebf16963d9580a581a15308d2771dce875a83 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 3 Apr 2017 15:38:02 +0100 Subject: [PATCH 2/6] Fix fixme in preserve_fn `preserve_fn` is no longer used as a decorator anywhere, so we can safely fix a fixme therein. --- synapse/util/logcontext.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/synapse/util/logcontext.py b/synapse/util/logcontext.py index 857afee7c..990216145 100644 --- a/synapse/util/logcontext.py +++ b/synapse/util/logcontext.py @@ -334,12 +334,8 @@ def preserve_fn(f): LoggingContext.set_current_context(LoggingContext.sentinel) return result - # XXX: why is this here rather than inside g? surely we want to preserve - # the context from the time the function was called, not when it was - # wrapped? - current = LoggingContext.current_context() - def g(*args, **kwargs): + current = LoggingContext.current_context() res = f(*args, **kwargs) if isinstance(res, defer.Deferred) and not res.called: # The function will have reset the context before returning, so From feb496056ee1a6d30174a2594dbe01e24dd4fb25 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 3 Apr 2017 15:41:17 +0100 Subject: [PATCH 3/6] preserve_fn some deferred-returning things In `Notifier._on_new_room_event`, `preserve_fn` around its subroutines which return deferreds, so that it is safe to call it with an active logcontext. --- synapse/notifier.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/synapse/notifier.py b/synapse/notifier.py index 3b206bb96..e8177452a 100644 --- a/synapse/notifier.py +++ b/synapse/notifier.py @@ -243,10 +243,13 @@ class Notifier(object): def _on_new_room_event(self, event, room_stream_id, extra_users=[]): """Notify any user streams that are interested in this room event""" # poke any interested application service. - self.appservice_handler.notify_interested_services(room_stream_id) + preserve_fn(self.appservice_handler.notify_interested_services)( + room_stream_id) if self.federation_sender: - self.federation_sender.notify_new_events(room_stream_id) + preserve_fn(self.federation_sender.notify_new_events)( + room_stream_id + ) if event.type == EventTypes.Member and event.membership == Membership.JOIN: self._user_joined_room(event.state_key, event.room_id) From 65e1683680b656accd46f531e00d69b68a09c49e Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 3 Apr 2017 15:42:38 +0100 Subject: [PATCH 4/6] Remove spurious PreserveLoggingContext In `on_new_room_event`, remove `PreserveLoggingContext` - we can call its subroutines with the logcontext set. --- synapse/notifier.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/synapse/notifier.py b/synapse/notifier.py index e8177452a..57d6a8cfe 100644 --- a/synapse/notifier.py +++ b/synapse/notifier.py @@ -215,13 +215,12 @@ class Notifier(object): until all previous events have been persisted before notifying the client streams. """ - with PreserveLoggingContext(): - self.pending_new_room_events.append(( - room_stream_id, event, extra_users - )) - self._notify_pending_new_room_events(max_room_stream_id) + self.pending_new_room_events.append(( + room_stream_id, event, extra_users + )) + self._notify_pending_new_room_events(max_room_stream_id) - self.notify_replication() + self.notify_replication() def _notify_pending_new_room_events(self, max_room_stream_id): """Notify for the room events that were queued waiting for a previous From 0b08c48fc5269a04325791c96ddd389a7cfe502a Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 3 Apr 2017 15:43:37 +0100 Subject: [PATCH 5/6] Remove more spurious `PreserveLoggingContext`s Remove `PreserveLoggingContext` around calls to `Notifier.on_new_room_event`; there is no problem if the logcontext is set when calling it. --- synapse/handlers/federation.py | 43 +++++++++++++++------------------- 1 file changed, 19 insertions(+), 24 deletions(-) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 888dd0124..737e2f716 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -28,7 +28,7 @@ from synapse.api.constants import EventTypes, Membership, RejectedReason from synapse.events.validator import EventValidator from synapse.util import unwrapFirstError from synapse.util.logcontext import ( - PreserveLoggingContext, preserve_fn, preserve_context_over_deferred + preserve_fn, preserve_context_over_deferred ) from synapse.util.metrics import measure_func from synapse.util.logutils import log_function @@ -394,11 +394,10 @@ class FederationHandler(BaseHandler): target_user = UserID.from_string(target_user_id) extra_users.append(target_user) - with PreserveLoggingContext(): - self.notifier.on_new_room_event( - event, event_stream_id, max_stream_id, - extra_users=extra_users - ) + self.notifier.on_new_room_event( + event, event_stream_id, max_stream_id, + extra_users=extra_users + ) if event.type == EventTypes.Member: if event.membership == Membership.JOIN: @@ -916,11 +915,10 @@ class FederationHandler(BaseHandler): origin, auth_chain, state, event ) - with PreserveLoggingContext(): - self.notifier.on_new_room_event( - event, event_stream_id, max_stream_id, - extra_users=[joinee] - ) + self.notifier.on_new_room_event( + event, event_stream_id, max_stream_id, + extra_users=[joinee] + ) logger.debug("Finished joining %s to %s", joinee, room_id) finally: @@ -1025,10 +1023,9 @@ class FederationHandler(BaseHandler): target_user = UserID.from_string(target_user_id) extra_users.append(target_user) - with PreserveLoggingContext(): - self.notifier.on_new_room_event( - event, event_stream_id, max_stream_id, extra_users=extra_users - ) + self.notifier.on_new_room_event( + event, event_stream_id, max_stream_id, extra_users=extra_users + ) if event.type == EventTypes.Member: if event.content["membership"] == Membership.JOIN: @@ -1074,11 +1071,10 @@ class FederationHandler(BaseHandler): ) target_user = UserID.from_string(event.state_key) - with PreserveLoggingContext(): - self.notifier.on_new_room_event( - event, event_stream_id, max_stream_id, - extra_users=[target_user], - ) + self.notifier.on_new_room_event( + event, event_stream_id, max_stream_id, + extra_users=[target_user], + ) defer.returnValue(event) @@ -1236,10 +1232,9 @@ class FederationHandler(BaseHandler): target_user = UserID.from_string(target_user_id) extra_users.append(target_user) - with PreserveLoggingContext(): - self.notifier.on_new_room_event( - event, event_stream_id, max_stream_id, extra_users=extra_users - ) + self.notifier.on_new_room_event( + event, event_stream_id, max_stream_id, extra_users=extra_users + ) defer.returnValue(None) From 7eb9f34cc3c2845a0ef35c9524f7ecc14339f7c1 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 3 Apr 2017 15:44:19 +0100 Subject: [PATCH 6/6] Remove spurious yield In `MessageHandler`, remove `yield` on call to `Notifier.on_new_room_event`: it doesn't return anything anyway. --- synapse/handlers/message.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index 7a498af5a..348056add 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -612,7 +612,7 @@ class MessageHandler(BaseHandler): @defer.inlineCallbacks def _notify(): yield run_on_reactor() - yield self.notifier.on_new_room_event( + self.notifier.on_new_room_event( event, event_stream_id, max_stream_id, extra_users=extra_users )