Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Ignore incoming events for rooms that we have left #2490

Merged
merged 5 commits into from Oct 10, 2017

Conversation

Projects
None yet
3 participants
Owner

erikjohnston commented Oct 3, 2017

When synapse receives an event for a room its not in over federation, it
double checks with the remote server to see if it is in fact in the
room. This is done so that if the server has forgotten about the room
(usually as a result of the database being dropped) it can recover from
it.

However, in the presence of state resets in large rooms, this can cause
a lot of work for servers that have legitimately left. As a hacky
solution that supports both cases we drop incoming events for rooms that
we have explicitly left.

This means that we no longer support the case of servers having
forgotten that they've rejoined a room, but that is sufficiently rare
that we're not going to support it for now.

Ignore incoming events for rooms that we have left
When synapse receives an event for a room its not in over federation, it
double checks with the remote server to see if it is in fact in the
room. This is done so that if the server has forgotten about the room
(usually as a result of the database being dropped) it can recover from
it.

However, in the presence of state resets in large rooms, this can cause
a lot of work for servers that have legitimately left. As a hacky
solution that supports both cases we drop incoming events for rooms that
we have explicitly left.

This means that we no longer support the case of servers having
forgotten that they've rejoined a room, but that is sufficiently rare
that we're not going to support it for now.
+ raise Exception("Invalid host name")
+
+ defer.returnValue(True)
+
@erikjohnston

erikjohnston Oct 3, 2017

Owner

This function is almost an exact copy of is_host_joined above.

synapse/handlers/federation.py
+ if not is_in_room:
+ was_in_room = yield self.store.was_host_joined(
+ pdu.room_id, self.server_name,
+
@ara4n

ara4n Oct 3, 2017

Owner

spurious LF?

Owner

ara4n commented Oct 3, 2017

lgtm. my gut is that the auto-rejoin-rooms stuff is too magical and complicated to preserve long-term, but given it can be done easily here... why not.

Owner

erikjohnston commented Oct 3, 2017

lgtm. my gut is that the auto-rejoin-rooms stuff is too magical and complicated to preserve long-term, but given it can be done easily here... why not.

Its worth noting that the actual number of lines to support this is relatively small by the looks of things, as it reuses existing code paths.

synapse/handlers/federation.py
@@ -125,6 +125,28 @@ def on_receive_pdu(self, origin, pdu, get_missing=True):
self.room_queues[pdu.room_id].append((pdu, origin))
return
+ # If we're no longer in the room just ditch the event entirely. This
+ # is probably an old server that has come back and thinks we're still
+ # in the room.
@richvdh

richvdh Oct 3, 2017

Member

... or we've been rejoined to the room by a state reset.

@@ -533,6 +533,38 @@ def is_host_joined(self, room_id, host):
defer.returnValue(True)
+ @cachedInlineCallbacks()
@richvdh

richvdh Oct 3, 2017

Member

erm; do we not need to arrange for this cache to be flushed?

synapse/storage/roommember.py
+ @cachedInlineCallbacks()
+ def was_host_joined(self, room_id, host):
+ """Check whether the server is or ever was in the room.
+ """
@richvdh

richvdh Oct 3, 2017

Member

document the arg types and return types please

@richvdh richvdh assigned erikjohnston and unassigned richvdh Oct 3, 2017

erikjohnston added some commits Oct 3, 2017

@erikjohnston erikjohnston assigned richvdh and unassigned erikjohnston Oct 3, 2017

synapse/storage/roommember.py
+ host (str)
+
+ Returns:
+ bool: whether the host is/was in the room or not
@richvdh

richvdh Oct 5, 2017

Member

nuh. it returns a deferred.

also: "True if the host is/was in the room. Otherwise False".

richvdh approved these changes Oct 5, 2017

lgtm other than comment nitpickery

@richvdh richvdh assigned erikjohnston and unassigned richvdh Oct 5, 2017

@erikjohnston erikjohnston merged commit 84e27a5 into develop Oct 10, 2017

6 of 8 checks passed

Sytest Dendron (Commit) Build #2723 origin/erikj/drop_left_room_events failed in 3 min 59 sec
Details
Sytest Dendron (Merged PR) Build finished.
Details
Sytest Postgres (Commit) Build #3556 origin/erikj/drop_left_room_events succeeded in 3 min 38 sec
Details
Sytest Postgres (Merged PR) Build finished.
Details
Sytest SQLite (Commit) Build #3651 origin/erikj/drop_left_room_events succeeded in 2 min 45 sec
Details
Sytest SQLite (Merged PR) Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@erikjohnston erikjohnston deleted the erikj/drop_left_room_events branch Oct 26, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment