Sliding Sync: Make PerConnectionState immutable

synapse/handlers/sliding_sync/__init__.py

@@ -432,15 +432,11 @@ async def current_sync_for_user(
                                     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)
 
@@ -505,11 +501,13 @@ async def current_sync_for_user(
                     # 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
 
         # Fetch room data
         rooms: Dict[str, SlidingSyncResult.RoomResult] = {}

synapse/types/handlers/sliding_sync.py

@@ -18,6 +18,7 @@
 from enum import Enum
 from typing import (
     TYPE_CHECKING,
+    AbstractSet,
     Callable,
     Dict,
     Final,
@@ -411,7 +412,7 @@ class StateValues:
 
 # We can't freeze this class because we want to update it in place with the
 # de-duplicated data.
-@attr.s(slots=True, auto_attribs=True)
+@attr.s(slots=True, auto_attribs=True, frozen=True)
 class RoomSyncConfig:
     """
     Holds the config for what data we should fetch for a room in the sync response.
@@ -425,7 +426,7 @@ class RoomSyncConfig:
     """
 
     timeline_limit: int
-    required_state_map: Dict[str, Set[str]]
+    required_state_map: Mapping[str, AbstractSet[str]]
 
     @classmethod
     def from_room_config(
@@ -499,7 +500,7 @@ def from_room_config(
 
     def deep_copy(self) -> "RoomSyncConfig":
         required_state_map: Dict[str, Set[str]] = {
-            state_type: state_key_set.copy()
+            state_type: set(state_key_set)
             for state_type, state_key_set in self.required_state_map.items()
         }
 
@@ -510,14 +511,20 @@ def deep_copy(self) -> "RoomSyncConfig":
 
     def combine_room_sync_config(
         self, other_room_sync_config: "RoomSyncConfig"
-    ) -> None:
+    ) -> "RoomSyncConfig":
         """
-        Combine this `RoomSyncConfig` with another `RoomSyncConfig` and take the
+        Combine this `RoomSyncConfig` with another `RoomSyncConfig` and return the
         superset union of the two.
         """
+        timeline_limit = self.timeline_limit
+        required_state_map = {
+            event_type: set(state_keys)
+            for event_type, state_keys in self.required_state_map.items()
+        }
+
         # Take the highest timeline limit
         if self.timeline_limit < other_room_sync_config.timeline_limit:
-            self.timeline_limit = other_room_sync_config.timeline_limit
+            timeline_limit = other_room_sync_config.timeline_limit
 
         # Union the required state
         for (
@@ -526,14 +533,14 @@ def combine_room_sync_config(
         ) in other_room_sync_config.required_state_map.items():
             # If we already have a wildcard for everything, we don't need to add
             # anything else
-            if StateValues.WILDCARD in self.required_state_map.get(
+            if StateValues.WILDCARD in required_state_map.get(
                 StateValues.WILDCARD, set()
             ):
                 break
 
             # If we already have a wildcard `state_key` for this `state_type`, we don't need
             # to add anything else
-            if StateValues.WILDCARD in self.required_state_map.get(state_type, set()):
+            if StateValues.WILDCARD in required_state_map.get(state_type, set()):
                 continue
 
             # If we're getting wildcards for the `state_type` and `state_key`, that's
@@ -542,16 +549,14 @@ def combine_room_sync_config(
                 state_type == StateValues.WILDCARD
                 and StateValues.WILDCARD in state_key_set
             ):
-                self.required_state_map = {state_type: {StateValues.WILDCARD}}
+                required_state_map = {state_type: {StateValues.WILDCARD}}
                 # We can break, since we don't need to add anything else
                 break
 
             for state_key in state_key_set:
                 # If we already have a wildcard for this specific `state_key`, we don't need
                 # to add it since the wildcard already covers it.
-                if state_key in self.required_state_map.get(
-                    StateValues.WILDCARD, set()
-                ):
+                if state_key in required_state_map.get(StateValues.WILDCARD, set()):
                     continue
 
                 # If we're getting a wildcard for the `state_type`, get rid of any other
@@ -562,7 +567,7 @@ def combine_room_sync_config(
                     # Make a copy so we don't run into an error: `dictionary changed size
                     # during iteration`, when we remove items
                     for existing_state_type, existing_state_key_set in list(
-                        self.required_state_map.items()
+                        required_state_map.items()
                     ):
                         # Make a copy so we don't run into an error: `Set changed size during
                         # iteration`, when we filter out and remove items
@@ -572,19 +577,21 @@ def combine_room_sync_config(
 
                         # If we've the left the `set()` empty, remove it from the map
                         if existing_state_key_set == set():
-                            self.required_state_map.pop(existing_state_type, None)
+                            required_state_map.pop(existing_state_type, None)
 
                 # If we're getting a wildcard `state_key`, get rid of any other state_keys
                 # for this `state_type` since the wildcard will cover it already.
                 if state_key == StateValues.WILDCARD:
-                    self.required_state_map[state_type] = {state_key}
+                    required_state_map[state_type] = {state_key}
                     break
                 # Otherwise, just add it to the set
                 else:
-                    if self.required_state_map.get(state_type) is None:
-                        self.required_state_map[state_type] = {state_key}
+                    if required_state_map.get(state_type) is None:
+                        required_state_map[state_type] = {state_key}
                     else:
-                        self.required_state_map[state_type].add(state_key)
+                        required_state_map[state_type].add(state_key)
+
+        return RoomSyncConfig(timeline_limit, required_state_map)
 
     def must_await_full_state(
         self,

tests/handlers/test_sliding_sync.py

@@ -18,7 +18,6 @@
 #
 #
 import logging
-from copy import deepcopy
 from typing import Dict, List, Optional
 from unittest.mock import patch
 
@@ -566,23 +565,11 @@ def test_combine_room_sync_config(
         """
         Combine A into B and B into A to make sure we get the same result.
         """
-        # Since we're mutating these in place, make a copy for each of our trials
-        room_sync_config_a = deepcopy(a)
-        room_sync_config_b = deepcopy(b)
+        combined_config = a.combine_room_sync_config(b)
+        self._assert_room_config_equal(combined_config, expected, "B into A")
 
-        # Combine B into A
-        room_sync_config_a.combine_room_sync_config(room_sync_config_b)
-
-        self._assert_room_config_equal(room_sync_config_a, expected, "B into A")
-
-        # Since we're mutating these in place, make a copy for each of our trials
-        room_sync_config_a = deepcopy(a)
-        room_sync_config_b = deepcopy(b)
-
-        # Combine A into B
-        room_sync_config_b.combine_room_sync_config(room_sync_config_a)
-
-        self._assert_room_config_equal(room_sync_config_b, expected, "A into B")
+        combined_config = a.combine_room_sync_config(b)
+        self._assert_room_config_equal(combined_config, expected, "A into B")
 
 
 class GetRoomMembershipForUserAtToTokenTestCase(HomeserverTestCase):