Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lazy loading: use sync token for /members?at=... request, as synapse expects it now #686

Merged

Conversation

bwindels
Copy link
Contributor

No description provided.

@bwindels bwindels requested a review from a team August 13, 2018 13:23
Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this safe if the sync returns with a member event before the response to /members comes back?

@bwindels
Copy link
Contributor Author

@dbkr Yes, this is handled by marking the RoomMember::supersedesOutOfBand. When the /members request comes back it doesn't override anything with that flag set. I was thinking about possibles races with this change here before, and there seemed to be some danger of the opposite: as processing the /sync response is not synchronous due to e2e being async, it could happen that:

  1. /sync comes back
  2. we save the next sync token
  3. /members?at=synctokensavedatstep2 is requested
  4. /members comes back with "user A" and mark it as out-of-band.
  5. we asynchronously process a membership event for "user A"

"User a" could be marked as out-of-band when it really shouldn't be (which would be sub-optimal). But this also won't happen as any update to RoomMember through setMembershipEvent will clear the out-of-band flag (step 5).

@bwindels bwindels force-pushed the bwindels/feature_lazyloading branch 2 times, most recently from 6d7a626 to 7f2885c Compare August 14, 2018 08:42
Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok - sounds like we know what the edge cases are then

@dbkr dbkr merged commit 801b240 into bwindels/feature_lazyloading Aug 15, 2018
@t3chguy t3chguy deleted the bwindels/ll_members_at_param_sync_token branch May 10, 2022 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants