Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Remove cache for get_shared_rooms_for_users #9416

Merged
merged 4 commits into from
Feb 22, 2021
Merged

Remove cache for get_shared_rooms_for_users #9416

merged 4 commits into from
Feb 22, 2021

Conversation

anoadragon453
Copy link
Member

@anoadragon453 anoadragon453 commented Feb 16, 2021 •
edited
Loading

This PR remove the cache for the get_shared_rooms_for_users storage method (the db method driving the experimental "what rooms do I share with this user?" feature: MSC2666). Currently subsequent requests to the endpoint will return the same result, even if your shared rooms with that user have changed.

The cache was added in #7785, but we forgot to ensure it was invalidated appropriately.

Upon attempting to invalidate it, I found that the cache had to be entirely invalidated whenever a user (remote or local) joined or left a room. This didn't make for a very useful cache, especially for a function that may or may not be called very often. Thus, I've opted to remove it instead of invalidating it.

If there's a way to partially invalidate this cache when a user leaves/joins a room, then that may be a case for bringing it back. But at the moment I don't think that's possible (especially if it requires a costly DB query just to do so).

@anoadragon453 anoadragon453 requested a review from a team February 16, 2021 12:40
clokep
clokep reviewed Feb 17, 2021
View reviewed changes
Copy link
Member

@clokep clokep left a comment

Choose a reason for hiding this comment

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

This PR looks fine, but it does seem like we're doing a lot of work to invalidate caches for something that may or may not be called frequently.

synapse/storage/databases/main/user_directory.py Outdated Show resolved Hide resolved
@anoadragon453
Copy link
Member Author

Yeah, after thinking about this a bit I think it's probably just best to lose the cache. We'd indeed spend so much work invalidating it that I don't think it's particularly enough to offset the benefit of keeping it.

That said, ideally we would rate-limit this endpoint, though the spec doesn't suggest to: matrix-org/matrix-spec-proposals#2666.

@anoadragon453
Drop cache for get_shared_rooms_for_users
46b58ac 
It's not clear that the benefit of caching this function outweighs
the performance cost of invalidating the cache when the underlying
tables change.
@anoadragon453
Fix docstring of add_users_in_public_rooms
9e3f535
@anoadragon453 anoadragon453 force-pushed the anoa/invalidate_user_tracking_caches branch from a588ee5 to 3787d8e Compare February 22, 2021 16:02
@anoadragon453 anoadragon453 changed the title Invalidate cache for get_shared_rooms_for_users when underlying tables change Remove cache for get_shared_rooms_for_users Feb 22, 2021
@anoadragon453
Copy link
Member Author

I've updated this PR to just remove the cache instead, as it seemed more costly to invalidate it.

@anoadragon453 anoadragon453 requested a review from a team February 22, 2021 16:06
@anoadragon453
Fix tests to check the shared_rooms endpoints multiple times
bfb7833 
Beforehand, with the cache added, multiple calls would give back the same
results, even if the users started sharing a new room in-between those calls.

Removing the cache should fix the problem, but it's a good idea to add tests
to cache the mistake in the future; in case we do decide to add a cache again.
@anoadragon453
Changelog
94022c4
@anoadragon453 anoadragon453 force-pushed the anoa/invalidate_user_tracking_caches branch from 3787d8e to 94022c4 Compare February 22, 2021 16:10
clokep
clokep approved these changes Feb 22, 2021
View reviewed changes
Copy link
Member

@clokep clokep left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me! 👍 The test changes just look like some code de-duplication.

synapse/storage/databases/main/user_directory.py
@@ -497,8 +497,7 @@ async def add_users_who_share_private_room(
async def add_users_in_public_rooms(
self, room_id: str, user_ids: Iterable[str]
) -> None:
"""Insert entries into the users_who_share_private_rooms table. The first
user should be a local user.
"""Insert entries into the users_in_public_rooms table.
Copy link
Member

Choose a reason for hiding this comment

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

Was this comment just stale?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it was a copy/paste error from the method above (add_users_who_share_private_room).

@anoadragon453
Copy link
Member Author

The test changes just look like some code de-duplication.

Code de-duping and making sure a second call happens to the shared_rooms endpoint after some change occurred.

@anoadragon453 anoadragon453 merged commit 0a363f9 into develop Feb 22, 2021
@anoadragon453 anoadragon453 deleted the anoa/invalidate_user_tracking_caches branch February 22, 2021 16:52
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Mar 14, 2021
@Midar
Update chat/matrix-synapse to 1.29.0
d5b1cc5 
Synapse 1.29.0 (2021-03-08)
===========================

Note that synapse now expects an `X-Forwarded-Proto` header when used with a reverse proxy. Please see [UPGRADE.rst](UPGRADE.rst#upgrading-to-v1290) for more details on this change.


No significant changes.


Synapse 1.29.0rc1 (2021-03-04)
==============================

Features
--------

- Add rate limiters to cross-user key sharing requests. ([\#8957](matrix-org/synapse#8957))
- Add `order_by` to the admin API `GET /_synapse/admin/v1/users/<user_id>/media`. Contributed by @dklimpel. ([\#8978](matrix-org/synapse#8978))
- Add some configuration settings to make users' profile data more private. ([\#9203](matrix-org/synapse#9203))
- The `no_proxy` and `NO_PROXY` environment variables are now respected in proxied HTTP clients with the lowercase form taking precedence if both are present. Additionally, the lowercase `https_proxy` environment variable is now respected in proxied HTTP clients on top of existing support for the uppercase `HTTPS_PROXY` form and takes precedence if both are present. Contributed by Timothy Leung. ([\#9372](matrix-org/synapse#9372))
- Add a configuration option, `user_directory.prefer_local_users`, which when enabled will make it more likely for users on the same server as you to appear above other users. ([\#9383](matrix-org/synapse#9383), [\#9385](matrix-org/synapse#9385))
- Add support for regenerating thumbnails if they have been deleted but the original image is still stored. ([\#9438](matrix-org/synapse#9438))
- Add support for `X-Forwarded-Proto` header when using a reverse proxy. ([\#9472](matrix-org/synapse#9472), [\#9501](matrix-org/synapse#9501), [\#9512](matrix-org/synapse#9512), [\#9539](matrix-org/synapse#9539))


Bugfixes
--------

- Fix a bug where users' pushers were not all deleted when they deactivated their account. ([\#9285](matrix-org/synapse#9285), [\#9516](matrix-org/synapse#9516))
- Fix a bug where a lot of unnecessary presence updates were sent when joining a room. ([\#9402](matrix-org/synapse#9402))
- Fix a bug that caused multiple calls to the experimental `shared_rooms` endpoint to return stale results. ([\#9416](matrix-org/synapse#9416))
- Fix a bug in single sign-on which could cause a "No session cookie found" error. ([\#9436](matrix-org/synapse#9436))
- Fix bug introduced in v1.27.0 where allowing a user to choose their own username when logging in via single sign-on did not work unless an `idp_icon` was defined. ([\#9440](matrix-org/synapse#9440))
- Fix a bug introduced in v1.26.0 where some sequences were not properly configured when running `synapse_port_db`. ([\#9449](matrix-org/synapse#9449))
- Fix deleting pushers when using sharded pushers. ([\#9465](matrix-org/synapse#9465), [\#9466](matrix-org/synapse#9466), [\#9479](matrix-org/synapse#9479), [\#9536](matrix-org/synapse#9536))
- Fix missing startup checks for the consistency of certain PostgreSQL sequences. ([\#9470](matrix-org/synapse#9470))
- Fix a long-standing bug where the media repository could leak file descriptors while previewing media. ([\#9497](matrix-org/synapse#9497))
- Properly purge the event chain cover index when purging history. ([\#9498](matrix-org/synapse#9498))
- Fix missing chain cover index due to a schema delta not being applied correctly. Only affected servers that ran development versions. ([\#9503](matrix-org/synapse#9503))
- Fix a bug introduced in v1.25.0 where `/_synapse/admin/join/` would fail when given a room alias. ([\#9506](matrix-org/synapse#9506))
- Prevent presence background jobs from running when presence is disabled. ([\#9530](matrix-org/synapse#9530))
- Fix rare edge case that caused a background update to fail if the server had rejected an event that had duplicate auth events. ([\#9537](matrix-org/synapse#9537))


Improved Documentation
----------------------

- Update the example systemd config to propagate reloads to individual units. ([\#9463](matrix-org/synapse#9463))


Internal Changes
----------------

- Add documentation and type hints to `parse_duration`. ([\#9432](matrix-org/synapse#9432))
- Remove vestiges of `uploads_path` configuration setting. ([\#9462](matrix-org/synapse#9462))
- Add a comment about systemd-python. ([\#9464](matrix-org/synapse#9464))
- Test that we require validated email for email pushers. ([\#9496](matrix-org/synapse#9496))
- Allow python to generate bytecode for synapse. ([\#9502](matrix-org/synapse#9502))
- Fix incorrect type hints. ([\#9515](matrix-org/synapse#9515), [\#9518](matrix-org/synapse#9518))
- Add type hints to device and event report admin API. ([\#9519](matrix-org/synapse#9519))
- Add type hints to user admin API. ([\#9521](matrix-org/synapse#9521))
- Bump the versions of mypy and mypy-zope used for static type checking. ([\#9529](matrix-org/synapse#9529))
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@clokep clokep clokep approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@anoadragon453 @clokep