Fix race in sync when joining room #2944
Conversation
synapse/handlers/sync.py
Outdated
| joined_room_ids.add(room_id) | ||
| continue | ||
|
|
||
| logger.info("SH joined_room_ids membership after current token") |
There was a problem hiding this comment.
Oops, forgot about this debug logging. Perhaps we want to keep something like it here? WDYT?
synapse/handlers/sync.py
Outdated
| joined_room_ids.add(room_id) | ||
| continue | ||
|
|
||
| logger.info("SH joined_room_ids membership after current token") |
There was a problem hiding this comment.
Yeah, sorry, as I mentioned in a comment this was debug logging I forgot to remove, (SH was just a way for me to search in logs for). Do you think it'd be useful to keep this log line? This case should be hit rarely.
synapse/handlers/sync.py
Outdated
| # If the membership's stream ordering is after the given stream | ||
| # ordering, we need to go and work out if the user was in the room | ||
| # before. | ||
| for room_id, membeship_stream_ordering in joined_rooms: |
synapse/handlers/sync.py
Outdated
| user_id = sync_config.user.to_string() | ||
| app_service = self.store.get_app_service_by_user_id(user_id) | ||
| if app_service: | ||
| rooms = yield self.store.get_app_service_rooms(app_service) |
There was a problem hiding this comment.
do we not need to worry about the same thing here?
There was a problem hiding this comment.
Yeeeeeeeeeeeeeeeeeees, though I really want to burn this code path with fire since its so inefficient and nothing should really be using it. Looking at the code again it actually may not be as bad to fix as I initially thought
There was a problem hiding this comment.
Hmm, though I think this code paths is actually broken, as it tries to instantiate RoomsForUser with too few arguments.
There was a problem hiding this comment.
i.e., I'm wondering if we can just stub it out with a NotImplementedError or something
The race happens when the user joins a room at the same time as doing a sync. We fetch the current token and then get the rooms the user is in. If the join happens after the current token, but before we get the rooms we end up sending down a partial room entry in the sync. This is fixed by looking at the stream ordering of the membership returned by get_rooms_for_user, and handling the case when that stream ordering is after the current token.
erikjohnston
force-pushed
the
erikj/fix_sync_race
branch
from
6629b8e to
a56d54d
Compare
|
(Sorry for rebasing, I did it on autopilot forgetting you'd already reviewed) |
The race happens when the user joins a room at the same time as doing a
sync. We fetch the current token and then get the rooms the user is in.
If the join happens after the current token, but before we get the rooms
we end up sending down a partial room entry in the sync.
This is fixed by looking at the stream ordering of the membership
returned by get_rooms_for_user, and handling the case when that stream
ordering is after the current token.