From 2623cec8746392067e781164e8ed4f2236b15bec Mon Sep 17 00:00:00 2001 From: Mark Haines Date: Mon, 25 Jul 2016 16:12:16 +0100 Subject: [PATCH 1/7] Don't add rejections to the state_group, persist all rejections --- synapse/storage/events.py | 9 +++++---- synapse/storage/state.py | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/synapse/storage/events.py b/synapse/storage/events.py index 6610549281..41c9b17d14 100644 --- a/synapse/storage/events.py +++ b/synapse/storage/events.py @@ -591,10 +591,11 @@ class EventsStore(SQLBaseStore): ], ) - if context.rejected: - self._store_rejections_txn( - txn, event.event_id, context.rejected - ) + for event, context in events_and_contexts: + if context.rejected: + self._store_rejections_txn( + txn, event.event_id, context.rejected + ) self._simple_insert_many_txn( txn, diff --git a/synapse/storage/state.py b/synapse/storage/state.py index 5b743db67a..cc1c7ec6a7 100644 --- a/synapse/storage/state.py +++ b/synapse/storage/state.py @@ -79,7 +79,7 @@ class StateStore(SQLBaseStore): state_events = dict(context.current_state) - if event.is_state(): + if event.is_state() and not context.rejected: state_events[(event.type, event.state_key)] = event state_group = context.new_state_group_id From 8f7f4cb92baa1eb8c772644e2567fe56d563b4b9 Mon Sep 17 00:00:00 2001 From: Mark Haines Date: Mon, 25 Jul 2016 17:13:37 +0100 Subject: [PATCH 2/7] Don't add the events to forward extremities if the event is rejected --- synapse/storage/events.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/synapse/storage/events.py b/synapse/storage/events.py index 41c9b17d14..201a4455fa 100644 --- a/synapse/storage/events.py +++ b/synapse/storage/events.py @@ -498,8 +498,8 @@ class EventsStore(SQLBaseStore): sql, (False, event.event_id,) ) - - self._update_extremeties(txn, [event]) + if not context.rejected: + self._update_extremeties(txn, [event]) events_and_contexts = [ ec for ec in events_and_contexts if ec[0] not in to_remove @@ -512,7 +512,10 @@ class EventsStore(SQLBaseStore): self._handle_mult_prev_events( txn, - events=[event for event, _ in events_and_contexts], + events=[ + event for event, context in events_and_contexts + if not context.rejected + ], ) for event, _ in events_and_contexts: From 1b3c3e6d68bf503bf09e046ecf57bb652669e637 Mon Sep 17 00:00:00 2001 From: Mark Haines Date: Mon, 25 Jul 2016 18:44:30 +0100 Subject: [PATCH 3/7] Only update the events and event_json tables for rejected events --- synapse/storage/events.py | 113 +++++++++++++++++++++----------------- synapse/storage/state.py | 2 +- 2 files changed, 63 insertions(+), 52 deletions(-) diff --git a/synapse/storage/events.py b/synapse/storage/events.py index 201a4455fa..c38a631081 100644 --- a/synapse/storage/events.py +++ b/synapse/storage/events.py @@ -407,21 +407,11 @@ class EventsStore(SQLBaseStore): event.room_id, event.internal_metadata.stream_ordering, ) - if not event.internal_metadata.is_outlier(): + if not event.internal_metadata.is_outlier() and not context.rejected: depth_updates[event.room_id] = max( event.depth, depth_updates.get(event.room_id, event.depth) ) - if context.push_actions: - self._set_push_actions_for_event_and_users_txn( - txn, event, context.push_actions - ) - - if event.type == EventTypes.Redaction and event.redacts is not None: - self._remove_push_actions_for_event_id_txn( - txn, event.room_id, event.redacts - ) - for room_id, depth in depth_updates.items(): self._update_min_depth_for_room_txn(txn, room_id, depth) @@ -431,6 +421,7 @@ class EventsStore(SQLBaseStore): ), [event.event_id for event, _ in events_and_contexts] ) + have_persisted = { event_id: outlier for event_id, outlier in txn.fetchall() @@ -442,6 +433,9 @@ class EventsStore(SQLBaseStore): # Handle the case of the list including the same event multiple # times. The tricky thing here is when they differ by whether # they are an outlier. + if context.rejected: + continue + if event.event_id in event_map: other = event_map[event.event_id] @@ -498,8 +492,8 @@ class EventsStore(SQLBaseStore): sql, (False, event.event_id,) ) - if not context.rejected: - self._update_extremeties(txn, [event]) + + self._update_extremeties(txn, [event]) events_and_contexts = [ ec for ec in events_and_contexts if ec[0] not in to_remove @@ -508,39 +502,8 @@ class EventsStore(SQLBaseStore): if not events_and_contexts: return - self._store_mult_state_groups_txn(txn, events_and_contexts) - - self._handle_mult_prev_events( - txn, - events=[ - event for event, context in events_and_contexts - if not context.rejected - ], - ) - - for event, _ in events_and_contexts: - if event.type == EventTypes.Name: - self._store_room_name_txn(txn, event) - elif event.type == EventTypes.Topic: - self._store_room_topic_txn(txn, event) - elif event.type == EventTypes.Message: - self._store_room_message_txn(txn, event) - elif event.type == EventTypes.Redaction: - self._store_redaction(txn, event) - elif event.type == EventTypes.RoomHistoryVisibility: - self._store_history_visibility_txn(txn, event) - elif event.type == EventTypes.GuestAccess: - self._store_guest_access_txn(txn, event) - - self._store_room_members_txn( - txn, - [ - event - for event, _ in events_and_contexts - if event.type == EventTypes.Member - ], - backfilled=backfilled, - ) + # From this point onwards the events are only events that we haven't + # seen before. def event_dict(event): return { @@ -594,11 +557,28 @@ class EventsStore(SQLBaseStore): ], ) + to_remove = set() for event, context in events_and_contexts: if context.rejected: self._store_rejections_txn( txn, event.event_id, context.rejected ) + to_remove.add(event.event_id) + + events_and_contexts = [ + ec for ec in events_and_contexts if ec[0].event_id not in to_remove + ] + + if not events_and_contexts: + return + + # From this point onwards the events are only ones that weren't rejected. + + for event, context in events_and_contexts: + if context.push_actions: + self._set_push_actions_for_event_and_users_txn( + txn, event, context.push_actions + ) self._simple_insert_many_txn( txn, @@ -614,6 +594,42 @@ class EventsStore(SQLBaseStore): ], ) + if event.type == EventTypes.Redaction and event.redacts is not None: + self._remove_push_actions_for_event_id_txn( + txn, event.room_id, event.redacts + ) + + self._store_mult_state_groups_txn(txn, events_and_contexts) + + self._handle_mult_prev_events( + txn, + events=[event for event, _ in events_and_contexts], + ) + + for event, _ in events_and_contexts: + if event.type == EventTypes.Name: + self._store_room_name_txn(txn, event) + elif event.type == EventTypes.Topic: + self._store_room_topic_txn(txn, event) + elif event.type == EventTypes.Message: + self._store_room_message_txn(txn, event) + elif event.type == EventTypes.Redaction: + self._store_redaction(txn, event) + elif event.type == EventTypes.RoomHistoryVisibility: + self._store_history_visibility_txn(txn, event) + elif event.type == EventTypes.GuestAccess: + self._store_guest_access_txn(txn, event) + + self._store_room_members_txn( + txn, + [ + event + for event, _ in events_and_contexts + if event.type == EventTypes.Member + ], + backfilled=backfilled, + ) + self._store_event_reference_hashes_txn( txn, [event for event, _ in events_and_contexts] ) @@ -670,11 +686,6 @@ class EventsStore(SQLBaseStore): # Outlier events shouldn't clobber the current state. continue - if context.rejected: - # If the event failed it's auth checks then it shouldn't - # clobbler the current state. - continue - txn.call_after( self._get_current_state_for_key.invalidate, (event.room_id, event.type, event.state_key,) diff --git a/synapse/storage/state.py b/synapse/storage/state.py index cc1c7ec6a7..5b743db67a 100644 --- a/synapse/storage/state.py +++ b/synapse/storage/state.py @@ -79,7 +79,7 @@ class StateStore(SQLBaseStore): state_events = dict(context.current_state) - if event.is_state() and not context.rejected: + if event.is_state(): state_events[(event.type, event.state_key)] = event state_group = context.new_state_group_id From efeb6176c169835465eeb6184ead940a89b93b4e Mon Sep 17 00:00:00 2001 From: Mark Haines Date: Tue, 26 Jul 2016 10:49:52 +0100 Subject: [PATCH 4/7] Don't add rejected events if we've seen them befrore. Add some comments to explain what the code is doing mechanically --- synapse/storage/events.py | 53 +++++++++++++++++++++++++++++++++++---- 1 file changed, 48 insertions(+), 5 deletions(-) diff --git a/synapse/storage/events.py b/synapse/storage/events.py index c38a631081..25a2be2795 100644 --- a/synapse/storage/events.py +++ b/synapse/storage/events.py @@ -397,6 +397,12 @@ class EventsStore(SQLBaseStore): @log_function def _persist_events_txn(self, txn, events_and_contexts, backfilled): + """Insert some number of room events into the necessary database tables. + + Rejected events are only inserted into the events table, the events_json table, + and the rejections table. Things reading from those table will need to check + whether the event was rejected. + """ depth_updates = {} for event, context in events_and_contexts: # Remove the any existing cache entries for the event_ids @@ -427,15 +433,21 @@ class EventsStore(SQLBaseStore): for event_id, outlier in txn.fetchall() } + # Remove the events that we've seen before. event_map = {} to_remove = set() for event, context in events_and_contexts: + if context.rejected: + # If the event is rejected then we don't care if the event + # was an outlier or not. + if event.event_id in have_persisted: + # If we have already seen the event then ignore it. + to_remove.add(event) + continue + # Handle the case of the list including the same event multiple # times. The tricky thing here is when they differ by whether # they are an outlier. - if context.rejected: - continue - if event.event_id in event_map: other = event_map[event.event_id] @@ -457,6 +469,12 @@ class EventsStore(SQLBaseStore): outlier_persisted = have_persisted[event.event_id] if not event.internal_metadata.is_outlier() and outlier_persisted: + # We received a copy of an event that we had already stored as + # an outlier in the database. We now have some state at that + # so we need to update the state_groups table with that state. + + # insert into the state_group, state_groups_state and + # event_to_state_groups tables. self._store_mult_state_groups_txn(txn, ((event, context),)) metadata_json = encode_json( @@ -472,6 +490,8 @@ class EventsStore(SQLBaseStore): (metadata_json, event.event_id,) ) + # Add an entry to the ex_outlier_stream table to replicate the + # change in outlier status to our workers. stream_order = event.internal_metadata.stream_ordering state_group_id = context.state_group or context.new_state_group_id self._simple_insert_txn( @@ -493,6 +513,8 @@ class EventsStore(SQLBaseStore): (False, event.event_id,) ) + # Update the event_backward_extremities table now that this + # event isn't an outlier any more. self._update_extremeties(txn, [event]) events_and_contexts = [ @@ -557,24 +579,30 @@ class EventsStore(SQLBaseStore): ], ) + # Remove the rejected events from the list now that we've added them + # to the events table and the events_json table. to_remove = set() for event, context in events_and_contexts: if context.rejected: + # Insert the event_id into the rejections table self._store_rejections_txn( txn, event.event_id, context.rejected ) - to_remove.add(event.event_id) + to_remove.add(event) events_and_contexts = [ - ec for ec in events_and_contexts if ec[0].event_id not in to_remove + ec for ec in events_and_contexts if ec[0] not in to_remove ] if not events_and_contexts: + # Make sure we don't pass an empty list to functions that expect to + # be storing at least one element. return # From this point onwards the events are only ones that weren't rejected. for event, context in events_and_contexts: + # Insert all the push actions into the event_push_actions table. if context.push_actions: self._set_push_actions_for_event_and_users_txn( txn, event, context.push_actions @@ -595,12 +623,18 @@ class EventsStore(SQLBaseStore): ) if event.type == EventTypes.Redaction and event.redacts is not None: + # Remove the entries in the event_push_actions table for the + # redacted event. self._remove_push_actions_for_event_id_txn( txn, event.room_id, event.redacts ) + # Insert into the state_groups, state_groups_state, and + # event_to_state_groups tables. self._store_mult_state_groups_txn(txn, events_and_contexts) + # Update the event_forward_extremities, event_backward_extremities and + # event_edges tables. self._handle_mult_prev_events( txn, events=[event for event, _ in events_and_contexts], @@ -608,18 +642,25 @@ class EventsStore(SQLBaseStore): for event, _ in events_and_contexts: if event.type == EventTypes.Name: + # Insert into the room_names and event_search tables. self._store_room_name_txn(txn, event) elif event.type == EventTypes.Topic: + # Insert into the topics table and event_search table. self._store_room_topic_txn(txn, event) elif event.type == EventTypes.Message: + # Insert into the event_search table. self._store_room_message_txn(txn, event) elif event.type == EventTypes.Redaction: + # Insert into the redactions table. self._store_redaction(txn, event) elif event.type == EventTypes.RoomHistoryVisibility: + # Insert into the event_search table. self._store_history_visibility_txn(txn, event) elif event.type == EventTypes.GuestAccess: + # Insert into the event_search table. self._store_guest_access_txn(txn, event) + # Insert into the room_memberships table. self._store_room_members_txn( txn, [ @@ -630,6 +671,7 @@ class EventsStore(SQLBaseStore): backfilled=backfilled, ) + # Insert event_reference_hashes table. self._store_event_reference_hashes_txn( txn, [event for event, _ in events_and_contexts] ) @@ -674,6 +716,7 @@ class EventsStore(SQLBaseStore): ], ) + # Prefil the event cache self._add_to_cache(txn, events_and_contexts) if backfilled: From a6f06ce3e280cfa18f51748a7d4327001658db40 Mon Sep 17 00:00:00 2001 From: Mark Haines Date: Tue, 26 Jul 2016 11:05:39 +0100 Subject: [PATCH 5/7] Fix how push_actions are redacted. --- synapse/storage/events.py | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/synapse/storage/events.py b/synapse/storage/events.py index 25a2be2795..c63ca36df6 100644 --- a/synapse/storage/events.py +++ b/synapse/storage/events.py @@ -522,6 +522,8 @@ class EventsStore(SQLBaseStore): ] if not events_and_contexts: + # Make sure we don't pass an empty list to functions that expect to + # be storing at least one element. return # From this point onwards the events are only events that we haven't @@ -608,6 +610,13 @@ class EventsStore(SQLBaseStore): txn, event, context.push_actions ) + if event.type == EventTypes.Redaction and event.redacts is not None: + # Remove the entries in the event_push_actions table for the + # redacted event. + self._remove_push_actions_for_event_id_txn( + txn, event.room_id, event.redacts + ) + self._simple_insert_many_txn( txn, table="event_auth", @@ -622,13 +631,6 @@ class EventsStore(SQLBaseStore): ], ) - if event.type == EventTypes.Redaction and event.redacts is not None: - # Remove the entries in the event_push_actions table for the - # redacted event. - self._remove_push_actions_for_event_id_txn( - txn, event.room_id, event.redacts - ) - # Insert into the state_groups, state_groups_state, and # event_to_state_groups tables. self._store_mult_state_groups_txn(txn, events_and_contexts) @@ -716,7 +718,7 @@ class EventsStore(SQLBaseStore): ], ) - # Prefil the event cache + # Prefill the event cache self._add_to_cache(txn, events_and_contexts) if backfilled: From db4f823d34c6ebe30ce8f4f957c20f6e0a627ecc Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Tue, 26 Jul 2016 11:49:40 +0100 Subject: [PATCH 6/7] Fix flake8 configuration Apparently flake8 v3 doesn't like trailing comments on config settings. Also remove the pep8 config, which didn't work (because it was missing W503) and duplicated the flake8 config. We don't use pep8 on its own, so the config was duplicative. --- setup.cfg | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/setup.cfg b/setup.cfg index 5ebce1c56b..da8eafbb39 100644 --- a/setup.cfg +++ b/setup.cfg @@ -16,7 +16,5 @@ ignore = [flake8] max-line-length = 90 -ignore = W503 ; W503 requires that binary operators be at the end, not start, of lines. Erik doesn't like it. - -[pep8] -max-line-length = 90 +# W503 requires that binary operators be at the end, not start, of lines. Erik doesn't like it. +ignore = W503 From 05e7e5e972446b639997f0ea461c2eea39617342 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Tue, 26 Jul 2016 11:59:08 +0100 Subject: [PATCH 7/7] Fix flake8 violation Apparently flake8 v3 puts the error on a different line to v2. Easiest way to make sure that happens is by putting the whole statement on one line :) --- synapse/app/__init__.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/synapse/app/__init__.py b/synapse/app/__init__.py index 1bc4279807..9c2b627590 100644 --- a/synapse/app/__init__.py +++ b/synapse/app/__init__.py @@ -16,13 +16,11 @@ import sys sys.dont_write_bytecode = True -from synapse.python_dependencies import ( - check_requirements, MissingRequirementError -) # NOQA +from synapse import python_dependencies # noqa: E402 try: - check_requirements() -except MissingRequirementError as e: + python_dependencies.check_requirements() +except python_dependencies.MissingRequirementError as e: message = "\n".join([ "Missing Requirement: %s" % (e.message,), "To install run:",