Skip to content

Sliding Sync: Make sure we get up-to-date information from get_sliding_sync_rooms_for_user(...)#17692

Merged
MadLittleMods merged 8 commits into
developfrom
madlittlemods/get_sliding_sync_rooms_for_user-bust-cache
Sep 11, 2024
Merged

Sliding Sync: Make sure we get up-to-date information from get_sliding_sync_rooms_for_user(...)#17692
MadLittleMods merged 8 commits into
developfrom
madlittlemods/get_sliding_sync_rooms_for_user-bust-cache

Conversation

@MadLittleMods

@MadLittleMods MadLittleMods commented Sep 10, 2024

Copy link
Copy Markdown
Contributor

Make sure we get up-to-date information from get_sliding_sync_rooms_for_user(...). We need to bust the get_sliding_sync_rooms_for_user cache when the room encryption is updated and any other field that is used in the query.

Follow-up to #17630

TODO

  • Bust cache for membership change (cross-reference get_rooms_for_user)
  • Bust cache for room encryption (cross-reference get_room_encryption)
  • Bust cache for forgotten (cross-reference did_forget/get_forgotten_rooms_for_user)

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
    • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
  • Code style is correct
    (run the linters)

Make sure we get up-to-date information from `get_sliding_sync_rooms_for_user(...)`
Comment thread synapse/storage/databases/main/cache.py Outdated
self._attempt_to_invalidate_cache("get_room_version_id", (room_id,))
self._attempt_to_invalidate_cache("get_room_type", (room_id,))
self._attempt_to_invalidate_cache("get_room_encryption", (room_id,))
self._attempt_to_invalidate_cache("get_sliding_sync_rooms_for_user", None)

@MadLittleMods MadLittleMods Sep 10, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

get_sliding_sync_rooms_for_user(...) doesn't cache very well because we have to bust it whenever state in any room changes. Is it even worth caching it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The reason I discovered this in the first place is because I tried adding COALESCE(j.event_stream_ordering, m.event_stream_ordering) for sorting and discovered that get_sliding_sync_rooms_for_user(...) was returning a bunch of cached values. And busting the cache for any event sent on the homeserver just doesn't make sense to do.

But busting the cache when any state across the homeserver seems just as bad.

)
self._attempt_to_invalidate_cache(
"get_sliding_sync_rooms_for_user", None
)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Busting the cache wherever we bust it for get_room_encryption already

)
self._invalidate_cache_and_stream(
txn, self.get_sliding_sync_rooms_for_user, (user_id,)
)

@MadLittleMods MadLittleMods Sep 10, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Busting the cache whenever we bust the cache for did_forget/get_forgotten_rooms_for_user

@MadLittleMods MadLittleMods changed the title Make sure we get up-to-date information from get_sliding_sync_rooms_for_user(...) Sliding Sync: Make sure we get up-to-date information from get_sliding_sync_rooms_for_user(...) Sep 10, 2024
},
}
}
response_body, from_token = self.do_sync(sync_body, tok=user1_tok)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Unrelated, just updating to use the do_sync helper

],
)

def test_filter_is_encrypted_up_to_date(self) -> None:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

New tests

@MadLittleMods MadLittleMods marked this pull request as ready for review September 10, 2024 23:17
@MadLittleMods MadLittleMods requested a review from a team as a code owner September 10, 2024 23:17
@MadLittleMods MadLittleMods merged commit e4a1f27 into develop Sep 11, 2024
@MadLittleMods MadLittleMods deleted the madlittlemods/get_sliding_sync_rooms_for_user-bust-cache branch September 11, 2024 17:13
@MadLittleMods

Copy link
Copy Markdown
Contributor Author

Thanks for the review @erikjohnston 🐕

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants