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

Return read-only collections from @cached methods #13755

Merged
merged 11 commits into from
Feb 10, 2023
Merged

Return read-only collections from @cached methods #13755

merged 11 commits into from
Feb 10, 2023

Conversation

squahtx
Copy link
Contributor

@squahtx squahtx commented Sep 8, 2022 •
edited
Loading

It's important that collections returned from @cached methods are not
modified, otherwise future retrievals from the cache will return the
modified collection.

This applies to the return values from @cached methods and the values
inside the dictionaries returned by @cachedList methods. It's not
necessary for the dictionaries returned by @cachedList methods
themselves to be read-only.

Signed-off-by: Sean Quah seanq@matrix.org

Return read-only collections from @cached methods
45821e1 
Signed-off-by: Sean Quah <seanq@matrix.org>
@squahtx squahtx requested a review from a team as a code owner September 8, 2022 15:16
squahtx
squahtx commented Sep 8, 2022
View reviewed changes
synapse/handlers/directory.py Outdated Show resolved Hide resolved
synapse/handlers/room.py Show resolved Hide resolved
synapse/handlers/sync.py
Comment on lines +1380 to 1382
unused_fallback_key_types = list(
await self.store.get_e2e_unused_fallback_key_types(user_id, device_id)
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This goes straight into the SyncResult and is pretty much only going to be JSON-serialized from here on.
Having to take a copy to make mypy happy is unfortunate.

Copy link
Contributor

Choose a reason for hiding this comment

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

At least it's a shallow copy 🤷

Copy link
Member

Choose a reason for hiding this comment

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

I'd be curious of the error here? Is something in our code unhappy or is it a type hint of like json.dump?

I think it'd be OK to add a cast with a comment explaining it is for performance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a type hint problem. Only Lists and Tuples are JSON-serializable, except they're mutable types. I would have updated the definition of SyncResult otherwise.

synapse/handlers/sync.py Outdated Show resolved Hide resolved
synapse/storage/databases/main/account_data.py Outdated Show resolved Hide resolved
synapse/storage/databases/main/devices.py Outdated Show resolved Hide resolved
DMRobertson
DMRobertson reviewed Oct 19, 2022
View reviewed changes
Copy link
Contributor

@DMRobertson DMRobertson left a comment

Choose a reason for hiding this comment

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

I haven't looked through the full changes (sorry), but I'd love to see us land some of this: particularly bugfixes or things that don't incur performance costs. (Though it's Python: if you're worried about performance costs, it could be argued you're doing something wrong.)

One thing I'm not clear on is enforcement. Is there some way we can annotate @cached or some mypy plugin witchcraft we can do to which will force us to only cache functions with immutable return values? (Possibly we could use Hashable as an approximation of an Immutable type?)

@DMRobertson DMRobertson mentioned this pull request Jan 25, 2023
Merged
2 tasks
@DMRobertson
Copy link
Contributor

Is there some way we can annotate @cached or some mypy plugin witchcraft we can do to which will force us to only cache functions with immutable return values? (Possibly we could use Hashable as an approximation of an Immutable type?)

I indulged myself. #14911 is a mypy plugin which hopefully detects a bunch of these.

@DMRobertson
Copy link
Contributor

I've tried to merge in develop to resolve the merge conflicts. (I'd like to see this land).

(And I also want to see what #14911 makes of it)

@DMRobertson
Copy link
Contributor

I've tried to merge in develop to resolve the merge conflicts. (I'd like to see this land).

(And I also want to see what #14911 makes of it)

Against develop: 181 errors
With this PR: 72 errors (some of which may be my hacky plugin).

@DMRobertson DMRobertson requested a review from a team February 1, 2023 01:04
@DMRobertson DMRobertson mentioned this pull request Feb 8, 2023
Merged
DMRobertson
DMRobertson reviewed Feb 9, 2023
View reviewed changes
Copy link
Contributor

@DMRobertson DMRobertson left a comment

Choose a reason for hiding this comment

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

Huge thanks for this Sean. I think this is a great change and am looking forward to being shot at in the foot less. Just a few things to check:

This was referenced Feb 9, 2023
Merged
Merged
@squahtx squahtx mentioned this pull request Feb 10, 2023
Merged
DMRobertson
DMRobertson approved these changes Feb 10, 2023
View reviewed changes
Copy link
Contributor

@DMRobertson DMRobertson left a comment

Choose a reason for hiding this comment

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

LGTM!

@squahtx
Copy link
Contributor Author

squahtx commented Feb 10, 2023

I had to fix some merge conflicts.

@squahtx squahtx merged commit d0c713c into develop Feb 10, 2023
@squahtx squahtx deleted the squah/read_only_collection_type_hints_2 branch February 10, 2023 23:29
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Mar 5, 2023
@Midar
Update chat/matrix-synapse to 1.78.0
4fc59f4 
Synapse 1.78.0 (2023-02-28)
===========================

Bugfixes
--------

- Fix a bug introduced in Synapse 1.76 where 5s delays would occasionally occur in deployments using workers. ([\#15150](matrix-org/synapse#15150))


Synapse 1.78.0rc1 (2023-02-21)
==============================

Features
--------

- Implement the experimental `exact_event_match` push rule condition from [MSC3758](matrix-org/matrix-spec-proposals#3758). ([\#14964](matrix-org/synapse#14964))
- Add account data to the command line [user data export tool](https://matrix-org.github.io/synapse/v1.78/usage/administration/admin_faq.html#how-can-i-export-user-data). ([\#14969](matrix-org/synapse#14969))
- Implement [MSC3873](matrix-org/matrix-spec-proposals#3873) to disambiguate push rule keys with dots in them. ([\#15004](matrix-org/synapse#15004))
- Allow Synapse to use a specific Redis [logical database](https://redis.io/commands/select/) in worker-mode deployments. ([\#15034](matrix-org/synapse#15034))
- Tag opentracing spans for federation requests with the name of the worker serving the request. ([\#15042](matrix-org/synapse#15042))
- Implement the experimental `exact_event_property_contains` push rule condition from [MSC3966](matrix-org/matrix-spec-proposals#3966). ([\#15045](matrix-org/synapse#15045))
- Remove spurious `dont_notify` action from the defaults for the `.m.rule.reaction` pushrule. ([\#15073](matrix-org/synapse#15073))
- Update the error code returned when user sends a duplicate annotation. ([\#15075](matrix-org/synapse#15075))


Bugfixes
--------

- Prevent clients from reporting nonexistent events. ([\#13779](matrix-org/synapse#13779))
- Return spec-compliant JSON errors when unknown endpoints are requested. ([\#14605](matrix-org/synapse#14605))
- Fix a long-standing bug where the room aliases returned could be corrupted. ([\#15038](matrix-org/synapse#15038))
- Fix a bug introduced in Synapse 1.76.0 where partially-joined rooms could not be deleted using the [purge room API](https://matrix-org.github.io/synapse/latest/admin_api/rooms.html#delete-room-api). ([\#15068](matrix-org/synapse#15068))
- Fix a long-standing bug where federated joins would fail if the first server in the list of servers to try is not in the room. ([\#15074](matrix-org/synapse#15074))
- Fix a bug introduced in Synapse v1.74.0 where searching with colons when using ICU for search term tokenisation would fail with an error. ([\#15079](matrix-org/synapse#15079))
- Reduce the likelihood of a rare race condition where rejoining a restricted room over federation would fail. ([\#15080](matrix-org/synapse#15080))
- Fix a bug introduced in Synapse 1.76 where workers would fail to start if the `health` listener was configured. ([\#15096](matrix-org/synapse#15096))
- Fix a bug introduced in Synapse 1.75 where the [portdb script](https://matrix-org.github.io/synapse/release-v1.78/postgres.html#porting-from-sqlite) would fail to run after a room had been faster-joined. ([\#15108](matrix-org/synapse#15108))


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

- Document how to start Synapse with Poetry. Contributed by @thezaidbintariq. ([\#14892](matrix-org/synapse#14892), [\#15022](matrix-org/synapse#15022))
- Update delegation documentation to clarify that SRV DNS delegation does not eliminate all needs to serve files from .well-known locations. Contributed by @williamkray. ([\#14959](matrix-org/synapse#14959))
- Fix a mistake in registration_shared_secret_path docs. ([\#15078](matrix-org/synapse#15078))
- Refer to a more recent blog post on the [Database Maintenance Tools](https://matrix-org.github.io/synapse/latest/usage/administration/database_maintenance_tools.html) page. Contributed by @jahway603. ([\#15083](matrix-org/synapse#15083))


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

- Re-type hint some collections as read-only. ([\#13755](matrix-org/synapse#13755))
- Faster joins: don't stall when another user joins during a partial-state room resync. ([\#14606](matrix-org/synapse#14606))
- Add a class `UnpersistedEventContext` to allow for the batching up of storing state groups. ([\#14675](matrix-org/synapse#14675))
- Add a check to ensure that locked dependencies have source distributions available. ([\#14742](matrix-org/synapse#14742))
- Tweak comment on `_is_local_room_accessible` as part of room visibility in `/hierarchy` to clarify the condition for a room being visible. ([\#14834](matrix-org/synapse#14834))
- Prevent `WARNING: there is already a transaction in progress` lines appearing in PostgreSQL's logs on some occasions. ([\#14840](matrix-org/synapse#14840))
- Use `StrCollection` to avoid potential bugs with `Collection[str]`. ([\#14929](matrix-org/synapse#14929))
- Improve performance of `/sync` in a few situations. ([\#14973](matrix-org/synapse#14973))
- Limit concurrent event creation for a room to avoid state resolution when sending bursts of events to a local room. ([\#14977](matrix-org/synapse#14977))
- Skip calculating unread push actions in /sync when enable_push is false. ([\#14980](matrix-org/synapse#14980))
- Add a schema dump symlinks inside `contrib`, to make it easier for IDEs to interrogate Synapse's database schema. ([\#14982](matrix-org/synapse#14982))
- Improve type hints. ([\#15008](matrix-org/synapse#15008), [\#15026](matrix-org/synapse#15026), [\#15027](matrix-org/synapse#15027), [\#15028](matrix-org/synapse#15028), [\#15031](matrix-org/synapse#15031), [\#15035](matrix-org/synapse#15035), [\#15052](matrix-org/synapse#15052), [\#15072](matrix-org/synapse#15072), [\#15084](matrix-org/synapse#15084))
- Update [MSC3952](matrix-org/matrix-spec-proposals#3952) support based on changes to the MSC. ([\#15037](matrix-org/synapse#15037))
- Avoid mutating a cached value in `get_user_devices_from_cache`. ([\#15040](matrix-org/synapse#15040))
- Fix a rare exception in logs on start up. ([\#15041](matrix-org/synapse#15041))
- Update pyo3-log to v0.8.1. ([\#15043](matrix-org/synapse#15043))
- Avoid mutating cached values in `_generate_sync_entry_for_account_data`. ([\#15047](matrix-org/synapse#15047))
- Refactor arguments of `try_unbind_threepid` and `_try_unbind_threepid_with_id_server` to not use dictionaries. ([\#15053](matrix-org/synapse#15053))
- Merge debug logging from the hotfixes branch. ([\#15054](matrix-org/synapse#15054))
- Faster joins: omit device list updates originating from partial state rooms in /sync responses without lazy loading of members enabled. ([\#15069](matrix-org/synapse#15069))
- Fix clashing database transaction name. ([\#15070](matrix-org/synapse#15070))
- Upper-bound frozendict dependency. This works around us being unable to test installing our wheels against Python 3.11 in CI. ([\#15114](matrix-org/synapse#15114))
- Tweak logging for when a worker waits for its view of a replication stream to catch up. ([\#15120](matrix-org/synapse#15120))

<details><summary>Locked dependency updates</summary>

- Bump bleach from 5.0.1 to 6.0.0. ([\#15059](matrix-org/synapse#15059))
- Bump cryptography from 38.0.4 to 39.0.1. ([\#15020](matrix-org/synapse#15020))
- Bump ruff version from 0.0.230 to 0.0.237. ([\#15033](matrix-org/synapse#15033))
- Bump dtolnay/rust-toolchain from 9cd00a88a73addc8617065438eff914dd08d0955 to 25dc93b901a87e864900a8aec6c12e9aa794c0c3. ([\#15060](matrix-org/synapse#15060))
- Bump systemd-python from 234 to 235. ([\#15061](matrix-org/synapse#15061))
- Bump serde_json from 1.0.92 to 1.0.93. ([\#15062](matrix-org/synapse#15062))
- Bump types-requests from 2.28.11.8 to 2.28.11.12. ([\#15063](matrix-org/synapse#15063))
- Bump types-pillow from 9.4.0.5 to 9.4.0.10. ([\#15064](matrix-org/synapse#15064))
- Bump sentry-sdk from 1.13.0 to 1.15.0. ([\#15065](matrix-org/synapse#15065))
- Bump types-jsonschema from 4.17.0.3 to 4.17.0.5. ([\#15099](matrix-org/synapse#15099))
- Bump types-bleach from 5.0.3.1 to 6.0.0.0. ([\#15100](matrix-org/synapse#15100))
- Bump dtolnay/rust-toolchain from 25dc93b901a87e864900a8aec6c12e9aa794c0c3 to e12eda571dc9a5ee5d58eecf4738ec291c66f295. ([\#15101](matrix-org/synapse#15101))
- Bump dawidd6/action-download-artifact from 2.24.3 to 2.25.0. ([\#15102](matrix-org/synapse#15102))
- Bump types-pillow from 9.4.0.10 to 9.4.0.13. ([\#15104](matrix-org/synapse#15104))
- Bump types-setuptools from 67.1.0.0 to 67.3.0.1. ([\#15105](matrix-org/synapse#15105))


</details>
@DMRobertson DMRobertson mentioned this pull request Sep 12, 2023
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@clokep clokep clokep left review comments

@DMRobertson DMRobertson DMRobertson 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.
3 participants
@squahtx @DMRobertson @clokep