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.

Sign up for GitHub

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

Jump to bottom

Sliding Sync: Make PerConnectionState immutable #17600

Merged
merged 9 commits into from
Aug 29, 2024
Merged

Sliding Sync: Make PerConnectionState immutable #17600

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e2ade85
Move sliding sync types
erikjohnston Aug 21, 2024
5b77f4a
Update mypy plugin to handle enums and typevars
erikjohnston Aug 22, 2024
7087c7c
Make RoomSyncConfig immutable
erikjohnston Aug 22, 2024
e34d634
Make PerConnectionState immutable
erikjohnston Aug 22, 2024
87d5336
Newsfile
erikjohnston Aug 22, 2024
5fe6466
Update synapse/types/handlers/sliding_sync.py
erikjohnston Aug 27, 2024
174e1ad
Merge remote-tracking branch 'origin/develop' into erikj/ss_immutable
erikjohnston Aug 29, 2024
088c6b2
Remove deep_copy
erikjohnston Aug 29, 2024
24b1e28
Merge remote-tracking branch 'origin/develop' into erikj/ss_immutable
erikjohnston Aug 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/17600.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Make the sliding sync `PerConnectionState` class immutable.
19 changes: 18 additions & 1 deletion scripts-dev/mypy_synapse_plugin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
NoneType,
TupleType,
TypeAliasType,
TypeVarType,
UninhabitedType,
UnionType,
)
Expand Down Expand Up @@ -233,6 +234,7 @@ def check_is_cacheable(
"synapse.synapse_rust.push.FilteredPushRules",
# This is technically not immutable, but close enough.
"signedjson.types.VerifyKey",
"synapse.types.StrCollection",
}

# Immutable containers only if the values are also immutable.
Expand Down Expand Up @@ -298,7 +300,7 @@ def is_cacheable(

elif rt.type.fullname in MUTABLE_CONTAINER_TYPES:
# Mutable containers are mutable regardless of their underlying type.
return False, None
return False, f"container {rt.type.fullname} is mutable"

elif "attrs" in rt.type.metadata:
# attrs classes are only cachable iff it is frozen (immutable itself)
Expand All @@ -318,6 +320,9 @@ def is_cacheable(
else:
return False, "non-frozen attrs class"

elif rt.type.is_enum:
MadLittleMods marked this conversation as resolved.
Show resolved Hide resolved
# We assume Enum values are immutable
Copy link
Contributor

Choose a reason for hiding this comment

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

We could check if the enum member values are all immutable to be sure.

Copy link
Member Author

Choose a reason for hiding this comment

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

If I could figure out how I would, but I couldn't :(

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work?

        elif rt.type.is_enum:
            for enum_member_name in rt.get_enum_values():
                enum_member_type = rt.type.names[enum_member_name].type
                # Enum members should have types
                assert enum_member_type is not None
                ok, note = is_cacheable(enum_member_type, signature, verbose)
                if not ok:
                    return (
                        False,
                        f"enum member {enum_member_name} value of type {enum_member_type} not cacheable: {note}",
                    )

            # All Enum member values are immutable
            return True, None

It hasn't been tested as this code doesn't seem to matter anymore in this PR? I commented this whole enum logic branch out and mypy still passes.

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 had no idea that get enum values was a thing. I'll have a try and see if it works! Very good catch either way

Yeah, turns out this matters for the next PR where we add a cache on this value. Not sure why I added this to this pr.

return True, None
else:
# Ensure we fail for unknown types, these generally means that the
# above code is not complete.
Expand All @@ -326,6 +331,18 @@ def is_cacheable(
f"Don't know how to handle {rt.type.fullname} return type instance",
)

elif isinstance(rt, TypeVarType):
# We consider TypeVars immutable if they are bound to a set of immutable
# types.
if rt.values:
for value in rt.values:
ok, note = is_cacheable(value, signature, verbose)
if not ok:
return False, f"TypeVar bound not cacheable {value}"
return True, None

return False, "TypeVar is unbound"

elif isinstance(rt, NoneType):
# None is cachable.
return True, None
Expand Down
16 changes: 7 additions & 9 deletions synapse/handlers/sliding_sync/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,6 @@
_RoomMembershipForUser,
)
from synapse.handlers.sliding_sync.store import SlidingSyncConnectionStore
from synapse.handlers.sliding_sync.types import (
HaveSentRoomFlag,
MutablePerConnectionState,
PerConnectionState,
RoomSyncConfig,
StateValues,
)
from synapse.logging.opentracing import (
SynapseTags,
log_kv,
Expand All @@ -57,10 +50,15 @@
StreamKeyType,
StreamToken,
)
from synapse.types.handlers import (
SLIDING_SYNC_DEFAULT_BUMP_EVENT_TYPES,
from synapse.types.handlers import SLIDING_SYNC_DEFAULT_BUMP_EVENT_TYPES
from synapse.types.handlers.sliding_sync import (
HaveSentRoomFlag,
MutablePerConnectionState,
PerConnectionState,
RoomSyncConfig,
SlidingSyncConfig,
SlidingSyncResult,
StateValues,
)
from synapse.types.state import StateFilter
from synapse.util.async_helpers import concurrently_execute
Expand Down
14 changes: 8 additions & 6 deletions synapse/handlers/sliding_sync/extensions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,6 @@

from synapse.api.constants import AccountDataTypes, EduTypes
from synapse.handlers.receipts import ReceiptEventSource
from synapse.handlers.sliding_sync.types import (
HaveSentRoomFlag,
MutablePerConnectionState,
PerConnectionState,
)
from synapse.logging.opentracing import trace
from synapse.storage.databases.main.receipts import ReceiptInRoom
from synapse.types import (
Expand All @@ -35,7 +30,14 @@
StrCollection,
StreamToken,
)
from synapse.types.handlers import OperationType, SlidingSyncConfig, SlidingSyncResult
from synapse.types.handlers.sliding_sync import (
HaveSentRoomFlag,
MutablePerConnectionState,
OperationType,
PerConnectionState,
SlidingSyncConfig,
SlidingSyncResult,
)

if TYPE_CHECKING:
from synapse.server import HomeServer
Expand Down
34 changes: 17 additions & 17 deletions synapse/handlers/sliding_sync/room_lists.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,6 @@
)
from synapse.events import StrippedStateEvent
from synapse.events.utils import parse_stripped_state_event
from synapse.handlers.sliding_sync.types import (
HaveSentRoomFlag,
PerConnectionState,
RoomSyncConfig,
)
from synapse.logging.opentracing import start_active_span, trace
from synapse.storage.databases.main.state import (
ROOM_UNKNOWN_SENTINEL,
Expand All @@ -61,7 +56,14 @@
StreamToken,
UserID,
)
from synapse.types.handlers import OperationType, SlidingSyncConfig, SlidingSyncResult
from synapse.types.handlers.sliding_sync import (
HaveSentRoomFlag,
OperationType,
PerConnectionState,
RoomSyncConfig,
SlidingSyncConfig,
SlidingSyncResult,
)
from synapse.types.state import StateFilter

if TYPE_CHECKING:
Expand Down Expand Up @@ -279,15 +281,11 @@ async def compute_interested_rooms(
room_id
)
if existing_room_sync_config is not None:
existing_room_sync_config.combine_room_sync_config(
room_sync_config = existing_room_sync_config.combine_room_sync_config(
room_sync_config
)
else:
# Make a copy so if we modify it later, it doesn't
# affect all references.
relevant_room_map[room_id] = (
room_sync_config.deep_copy()
)

relevant_room_map[room_id] = room_sync_config

room_ids_in_list.append(room_id)

Expand Down Expand Up @@ -351,11 +349,13 @@ async def compute_interested_rooms(
# and need to fetch more info about.
existing_room_sync_config = relevant_room_map.get(room_id)
if existing_room_sync_config is not None:
existing_room_sync_config.combine_room_sync_config(
room_sync_config
room_sync_config = (
existing_room_sync_config.combine_room_sync_config(
room_sync_config
)
)
else:
relevant_room_map[room_id] = room_sync_config

relevant_room_map[room_id] = room_sync_config

# Filtered subset of `relevant_room_map` for rooms that may have updates
# (in the event stream)
Expand Down
8 changes: 4 additions & 4 deletions synapse/handlers/sliding_sync/store.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,13 @@
import attr

from synapse.api.errors import SlidingSyncUnknownPosition
from synapse.handlers.sliding_sync.types import (
from synapse.logging.opentracing import trace
from synapse.types import SlidingSyncStreamToken
from synapse.types.handlers.sliding_sync import (
MutablePerConnectionState,
PerConnectionState,
SlidingSyncConfig,
)
from synapse.logging.opentracing import trace
from synapse.types import SlidingSyncStreamToken
from synapse.types.handlers import SlidingSyncConfig

if TYPE_CHECKING:
pass
Expand Down
Loading
Loading