From cdc02594491b9410f250f0adc4ea6d223aa3de7f Mon Sep 17 00:00:00 2001 From: jejo86 <28619134+jejo86@users.noreply.github.com> Date: Wed, 29 Jun 2022 11:24:10 +0200 Subject: [PATCH 01/35] Document the `--report-stats` argument (#13029) Signed-off-by: jejo86 <28619134+jejo86@users.noreply.github.com> --- changelog.d/13029.doc | 1 + docs/setup/installation.md | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-) create mode 100644 changelog.d/13029.doc diff --git a/changelog.d/13029.doc b/changelog.d/13029.doc new file mode 100644 index 0000000000..d398f0fdbe --- /dev/null +++ b/changelog.d/13029.doc @@ -0,0 +1 @@ +Add an explanation of the `--report-stats` argument to the docs. diff --git a/docs/setup/installation.md b/docs/setup/installation.md index 5bdefe2bc1..1580529fd1 100644 --- a/docs/setup/installation.md +++ b/docs/setup/installation.md @@ -232,7 +232,9 @@ python -m synapse.app.homeserver \ --report-stats=[yes|no] ``` -... substituting an appropriate value for `--server-name`. +... substituting an appropriate value for `--server-name` and choosing whether +or not to report usage statistics (hostname, Synapse version, uptime, total +users, etc.) to the developers via the `--report-stats` argument. This command will generate you a config file that you can then customise, but it will also generate a set of keys for you. These keys will allow your homeserver to From 92a0c18ef0f42b80e382667141e6593ab30e3776 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 29 Jun 2022 11:32:38 +0100 Subject: [PATCH 02/35] Improve performance of getting unread counts in rooms (#13119) --- changelog.d/13119.misc | 1 + synapse/_scripts/synapse_port_db.py | 3 +++ synapse/storage/databases/main/__init__.py | 2 +- .../databases/main/event_push_actions.py | 16 ++++++++++++--- synapse/storage/databases/main/stream.py | 20 +++++++++++++++++++ tests/storage/test_event_push_actions.py | 2 ++ 6 files changed, 40 insertions(+), 4 deletions(-) create mode 100644 changelog.d/13119.misc diff --git a/changelog.d/13119.misc b/changelog.d/13119.misc new file mode 100644 index 0000000000..3bb51962e7 --- /dev/null +++ b/changelog.d/13119.misc @@ -0,0 +1 @@ +Reduce DB usage of `/sync` when a large number of unread messages have recently been sent in a room. diff --git a/synapse/_scripts/synapse_port_db.py b/synapse/_scripts/synapse_port_db.py index 9c06c837dc..f3f9c6d54c 100755 --- a/synapse/_scripts/synapse_port_db.py +++ b/synapse/_scripts/synapse_port_db.py @@ -270,6 +270,9 @@ class MockHomeserver: def get_instance_name(self) -> str: return "master" + def should_send_federation(self) -> bool: + return False + class Porter: def __init__( diff --git a/synapse/storage/databases/main/__init__.py b/synapse/storage/databases/main/__init__.py index 57aaf778ec..a3d31d3737 100644 --- a/synapse/storage/databases/main/__init__.py +++ b/synapse/storage/databases/main/__init__.py @@ -87,7 +87,6 @@ class DataStore( RoomStore, RoomBatchStore, RegistrationStore, - StreamWorkerStore, ProfileStore, PresenceStore, TransactionWorkerStore, @@ -112,6 +111,7 @@ class DataStore( SearchStore, TagsStore, AccountDataStore, + StreamWorkerStore, OpenIdStore, ClientIpWorkerStore, DeviceStore, diff --git a/synapse/storage/databases/main/event_push_actions.py b/synapse/storage/databases/main/event_push_actions.py index 80ca2fd0b6..eae41d7484 100644 --- a/synapse/storage/databases/main/event_push_actions.py +++ b/synapse/storage/databases/main/event_push_actions.py @@ -25,8 +25,8 @@ from synapse.storage.database import ( LoggingDatabaseConnection, LoggingTransaction, ) -from synapse.storage.databases.main.events_worker import EventsWorkerStore from synapse.storage.databases.main.receipts import ReceiptsWorkerStore +from synapse.storage.databases.main.stream import StreamWorkerStore from synapse.util import json_encoder from synapse.util.caches.descriptors import cached @@ -122,7 +122,7 @@ def _deserialize_action(actions: str, is_highlight: bool) -> List[Union[dict, st return DEFAULT_NOTIF_ACTION -class EventPushActionsWorkerStore(ReceiptsWorkerStore, EventsWorkerStore, SQLBaseStore): +class EventPushActionsWorkerStore(ReceiptsWorkerStore, StreamWorkerStore, SQLBaseStore): def __init__( self, database: DatabasePool, @@ -218,7 +218,7 @@ class EventPushActionsWorkerStore(ReceiptsWorkerStore, EventsWorkerStore, SQLBas retcol="event_id", ) - stream_ordering = self.get_stream_id_for_event_txn(txn, event_id) # type: ignore[attr-defined] + stream_ordering = self.get_stream_id_for_event_txn(txn, event_id) return self._get_unread_counts_by_pos_txn( txn, room_id, user_id, stream_ordering @@ -307,12 +307,22 @@ class EventPushActionsWorkerStore(ReceiptsWorkerStore, EventsWorkerStore, SQLBas actions that have been deleted from `event_push_actions` table. """ + # If there have been no events in the room since the stream ordering, + # there can't be any push actions either. + if not self._events_stream_cache.has_entity_changed(room_id, stream_ordering): + return 0, 0 + clause = "" args = [user_id, room_id, stream_ordering] if max_stream_ordering is not None: clause = "AND ea.stream_ordering <= ?" args.append(max_stream_ordering) + # If the max stream ordering is less than the min stream ordering, + # then obviously there are zero push actions in that range. + if max_stream_ordering <= stream_ordering: + return 0, 0 + sql = f""" SELECT COUNT(CASE WHEN notif = 1 THEN 1 END), diff --git a/synapse/storage/databases/main/stream.py b/synapse/storage/databases/main/stream.py index 8e88784d3c..3a1df7776c 100644 --- a/synapse/storage/databases/main/stream.py +++ b/synapse/storage/databases/main/stream.py @@ -46,10 +46,12 @@ from typing import ( Set, Tuple, cast, + overload, ) import attr from frozendict import frozendict +from typing_extensions import Literal from twisted.internet import defer @@ -795,6 +797,24 @@ class StreamWorkerStore(EventsWorkerStore, SQLBaseStore): ) return RoomStreamToken(topo, stream_ordering) + @overload + def get_stream_id_for_event_txn( + self, + txn: LoggingTransaction, + event_id: str, + allow_none: Literal[False] = False, + ) -> int: + ... + + @overload + def get_stream_id_for_event_txn( + self, + txn: LoggingTransaction, + event_id: str, + allow_none: bool = False, + ) -> Optional[int]: + ... + def get_stream_id_for_event_txn( self, txn: LoggingTransaction, diff --git a/tests/storage/test_event_push_actions.py b/tests/storage/test_event_push_actions.py index ef069a8110..a5a2dab21c 100644 --- a/tests/storage/test_event_push_actions.py +++ b/tests/storage/test_event_push_actions.py @@ -86,6 +86,8 @@ class EventPushActionsStoreTestCase(HomeserverTestCase): event.internal_metadata.is_outlier.return_value = False event.depth = stream + self.store._events_stream_cache.entity_has_changed(room_id, stream) + self.get_success( self.store.db_pool.simple_insert( table="events", From e714b8a057f65fe07b4f3939e018e57862980cdc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Moritz=20St=C3=BCckler?= Date: Wed, 29 Jun 2022 18:41:39 +0200 Subject: [PATCH 03/35] Fix documentation header for `allow_public_rooms_over_federation` (#13116) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Moritz Stückler Co-authored-by: Patrick Cloke --- changelog.d/13116.doc | 1 + docs/usage/configuration/config_documentation.md | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) create mode 100644 changelog.d/13116.doc diff --git a/changelog.d/13116.doc b/changelog.d/13116.doc new file mode 100644 index 0000000000..f99be50f44 --- /dev/null +++ b/changelog.d/13116.doc @@ -0,0 +1 @@ +Fix wrong section header for `allow_public_rooms_over_federation` in the homeserver config documentation. diff --git a/docs/usage/configuration/config_documentation.md b/docs/usage/configuration/config_documentation.md index 58a74ace48..19eb504496 100644 --- a/docs/usage/configuration/config_documentation.md +++ b/docs/usage/configuration/config_documentation.md @@ -317,7 +317,7 @@ Example configuration: allow_public_rooms_without_auth: true ``` --- -### `allow_public_rooms_without_auth` +### `allow_public_rooms_over_federation` If set to true, allows any other homeserver to fetch the server's public rooms directory via federation. Defaults to false. From 13e359aec8ae8be8dc56a036ae6d9f2bc1d07385 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Brandner?= Date: Wed, 29 Jun 2022 19:12:45 +0200 Subject: [PATCH 04/35] Implement MSC3827: Filtering of `/publicRooms` by room type (#13031) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Šimon Brandner --- changelog.d/13031.feature | 1 + synapse/api/constants.py | 10 ++ synapse/config/experimental.py | 3 + synapse/handlers/room_list.py | 23 +++- synapse/handlers/stats.py | 3 + synapse/rest/client/versions.py | 2 + synapse/storage/databases/main/room.py | 126 +++++++++++++++++- synapse/storage/databases/main/stats.py | 10 +- .../72/01add_room_type_to_state_stats.sql | 19 +++ tests/rest/client/test_rooms.py | 92 ++++++++++++- tests/storage/databases/main/test_room.py | 69 ++++++++++ 11 files changed, 345 insertions(+), 13 deletions(-) create mode 100644 changelog.d/13031.feature create mode 100644 synapse/storage/schema/main/delta/72/01add_room_type_to_state_stats.sql diff --git a/changelog.d/13031.feature b/changelog.d/13031.feature new file mode 100644 index 0000000000..fee8e9d1ff --- /dev/null +++ b/changelog.d/13031.feature @@ -0,0 +1 @@ +Implement [MSC3827](https://github.com/matrix-org/matrix-spec-proposals/pull/3827): Filtering of /publicRooms by room type. diff --git a/synapse/api/constants.py b/synapse/api/constants.py index e1d31cabed..2653764119 100644 --- a/synapse/api/constants.py +++ b/synapse/api/constants.py @@ -259,3 +259,13 @@ class ReceiptTypes: READ: Final = "m.read" READ_PRIVATE: Final = "org.matrix.msc2285.read.private" FULLY_READ: Final = "m.fully_read" + + +class PublicRoomsFilterFields: + """Fields in the search filter for `/publicRooms` that we understand. + + As defined in https://spec.matrix.org/v1.3/client-server-api/#post_matrixclientv3publicrooms + """ + + GENERIC_SEARCH_TERM: Final = "generic_search_term" + ROOM_TYPES: Final = "org.matrix.msc3827.room_types" diff --git a/synapse/config/experimental.py b/synapse/config/experimental.py index 0a285dba31..ee443cea00 100644 --- a/synapse/config/experimental.py +++ b/synapse/config/experimental.py @@ -87,3 +87,6 @@ class ExperimentalConfig(Config): # MSC3715: dir param on /relations. self.msc3715_enabled: bool = experimental.get("msc3715_enabled", False) + + # MSC3827: Filtering of /publicRooms by room type + self.msc3827_enabled: bool = experimental.get("msc3827_enabled", False) diff --git a/synapse/handlers/room_list.py b/synapse/handlers/room_list.py index 183d4ae3c4..29868eb743 100644 --- a/synapse/handlers/room_list.py +++ b/synapse/handlers/room_list.py @@ -25,6 +25,7 @@ from synapse.api.constants import ( GuestAccess, HistoryVisibility, JoinRules, + PublicRoomsFilterFields, ) from synapse.api.errors import ( Codes, @@ -181,6 +182,7 @@ class RoomListHandler: == HistoryVisibility.WORLD_READABLE, "guest_can_join": room["guest_access"] == "can_join", "join_rule": room["join_rules"], + "org.matrix.msc3827.room_type": room["room_type"], } # Filter out Nones – rather omit the field altogether @@ -239,7 +241,9 @@ class RoomListHandler: response["chunk"] = results response["total_room_count_estimate"] = await self.store.count_public_rooms( - network_tuple, ignore_non_federatable=from_federation + network_tuple, + ignore_non_federatable=from_federation, + search_filter=search_filter, ) return response @@ -508,8 +512,21 @@ class RoomListNextBatch: def _matches_room_entry(room_entry: JsonDict, search_filter: dict) -> bool: - if search_filter and search_filter.get("generic_search_term", None): - generic_search_term = search_filter["generic_search_term"].upper() + """Determines whether the given search filter matches a room entry returned over + federation. + + Only used if the remote server does not support MSC2197 remote-filtered search, and + hence does not support MSC3827 filtering of `/publicRooms` by room type either. + + In this case, we cannot apply the `room_type` filter since no `room_type` field is + returned. + """ + if search_filter and search_filter.get( + PublicRoomsFilterFields.GENERIC_SEARCH_TERM, None + ): + generic_search_term = search_filter[ + PublicRoomsFilterFields.GENERIC_SEARCH_TERM + ].upper() if generic_search_term in room_entry.get("name", "").upper(): return True elif generic_search_term in room_entry.get("topic", "").upper(): diff --git a/synapse/handlers/stats.py b/synapse/handlers/stats.py index f45e06eb0e..5c01482acf 100644 --- a/synapse/handlers/stats.py +++ b/synapse/handlers/stats.py @@ -271,6 +271,9 @@ class StatsHandler: room_state["is_federatable"] = ( event_content.get(EventContentFields.FEDERATE, True) is True ) + room_type = event_content.get(EventContentFields.ROOM_TYPE) + if isinstance(room_type, str): + room_state["room_type"] = room_type elif typ == EventTypes.JoinRules: room_state["join_rules"] = event_content.get("join_rule") elif typ == EventTypes.RoomHistoryVisibility: diff --git a/synapse/rest/client/versions.py b/synapse/rest/client/versions.py index c1bd775fec..f4f06563dd 100644 --- a/synapse/rest/client/versions.py +++ b/synapse/rest/client/versions.py @@ -95,6 +95,8 @@ class VersionsRestServlet(RestServlet): "org.matrix.msc3026.busy_presence": self.config.experimental.msc3026_enabled, # Supports receiving private read receipts as per MSC2285 "org.matrix.msc2285": self.config.experimental.msc2285_enabled, + # Supports filtering of /publicRooms by room type MSC3827 + "org.matrix.msc3827": self.config.experimental.msc3827_enabled, # Adds support for importing historical messages as per MSC2716 "org.matrix.msc2716": self.config.experimental.msc2716_enabled, # Adds support for jump to date endpoints (/timestamp_to_event) as per MSC3030 diff --git a/synapse/storage/databases/main/room.py b/synapse/storage/databases/main/room.py index 5760d3428e..d8026e3fac 100644 --- a/synapse/storage/databases/main/room.py +++ b/synapse/storage/databases/main/room.py @@ -32,12 +32,17 @@ from typing import ( import attr -from synapse.api.constants import EventContentFields, EventTypes, JoinRules +from synapse.api.constants import ( + EventContentFields, + EventTypes, + JoinRules, + PublicRoomsFilterFields, +) from synapse.api.errors import StoreError from synapse.api.room_versions import RoomVersion, RoomVersions from synapse.config.homeserver import HomeServerConfig from synapse.events import EventBase -from synapse.storage._base import SQLBaseStore, db_to_json +from synapse.storage._base import SQLBaseStore, db_to_json, make_in_list_sql_clause from synapse.storage.database import ( DatabasePool, LoggingDatabaseConnection, @@ -199,10 +204,29 @@ class RoomWorkerStore(CacheInvalidationWorkerStore): desc="get_public_room_ids", ) + def _construct_room_type_where_clause( + self, room_types: Union[List[Union[str, None]], None] + ) -> Tuple[Union[str, None], List[str]]: + if not room_types or not self.config.experimental.msc3827_enabled: + return None, [] + else: + # We use None when we want get rooms without a type + is_null_clause = "" + if None in room_types: + is_null_clause = "OR room_type IS NULL" + room_types = [value for value in room_types if value is not None] + + list_clause, args = make_in_list_sql_clause( + self.database_engine, "room_type", room_types + ) + + return f"({list_clause} {is_null_clause})", args + async def count_public_rooms( self, network_tuple: Optional[ThirdPartyInstanceID], ignore_non_federatable: bool, + search_filter: Optional[dict], ) -> int: """Counts the number of public rooms as tracked in the room_stats_current and room_stats_state table. @@ -210,11 +234,20 @@ class RoomWorkerStore(CacheInvalidationWorkerStore): Args: network_tuple ignore_non_federatable: If true filters out non-federatable rooms + search_filter """ def _count_public_rooms_txn(txn: LoggingTransaction) -> int: query_args = [] + room_type_clause, args = self._construct_room_type_where_clause( + search_filter.get(PublicRoomsFilterFields.ROOM_TYPES, None) + if search_filter + else None + ) + room_type_clause = f" AND {room_type_clause}" if room_type_clause else "" + query_args += args + if network_tuple: if network_tuple.appservice_id: published_sql = """ @@ -249,6 +282,7 @@ class RoomWorkerStore(CacheInvalidationWorkerStore): OR join_rules = '{JoinRules.KNOCK_RESTRICTED}' OR history_visibility = 'world_readable' ) + {room_type_clause} AND joined_members > 0 """ @@ -347,8 +381,12 @@ class RoomWorkerStore(CacheInvalidationWorkerStore): if ignore_non_federatable: where_clauses.append("is_federatable") - if search_filter and search_filter.get("generic_search_term", None): - search_term = "%" + search_filter["generic_search_term"] + "%" + if search_filter and search_filter.get( + PublicRoomsFilterFields.GENERIC_SEARCH_TERM, None + ): + search_term = ( + "%" + search_filter[PublicRoomsFilterFields.GENERIC_SEARCH_TERM] + "%" + ) where_clauses.append( """ @@ -365,6 +403,15 @@ class RoomWorkerStore(CacheInvalidationWorkerStore): search_term.lower(), ] + room_type_clause, args = self._construct_room_type_where_clause( + search_filter.get(PublicRoomsFilterFields.ROOM_TYPES, None) + if search_filter + else None + ) + if room_type_clause: + where_clauses.append(room_type_clause) + query_args += args + where_clause = "" if where_clauses: where_clause = " AND " + " AND ".join(where_clauses) @@ -373,7 +420,7 @@ class RoomWorkerStore(CacheInvalidationWorkerStore): sql = f""" SELECT room_id, name, topic, canonical_alias, joined_members, - avatar, history_visibility, guest_access, join_rules + avatar, history_visibility, guest_access, join_rules, room_type FROM ( {published_sql} ) published @@ -1166,6 +1213,7 @@ class _BackgroundUpdates: POPULATE_ROOM_DEPTH_MIN_DEPTH2 = "populate_room_depth_min_depth2" REPLACE_ROOM_DEPTH_MIN_DEPTH = "replace_room_depth_min_depth" POPULATE_ROOMS_CREATOR_COLUMN = "populate_rooms_creator_column" + ADD_ROOM_TYPE_COLUMN = "add_room_type_column" _REPLACE_ROOM_DEPTH_SQL_COMMANDS = ( @@ -1200,6 +1248,11 @@ class RoomBackgroundUpdateStore(SQLBaseStore): self._background_add_rooms_room_version_column, ) + self.db_pool.updates.register_background_update_handler( + _BackgroundUpdates.ADD_ROOM_TYPE_COLUMN, + self._background_add_room_type_column, + ) + # BG updates to change the type of room_depth.min_depth self.db_pool.updates.register_background_update_handler( _BackgroundUpdates.POPULATE_ROOM_DEPTH_MIN_DEPTH2, @@ -1569,6 +1622,69 @@ class RoomBackgroundUpdateStore(SQLBaseStore): return batch_size + async def _background_add_room_type_column( + self, progress: JsonDict, batch_size: int + ) -> int: + """Background update to go and add room_type information to `room_stats_state` + table from `event_json` table. + """ + + last_room_id = progress.get("room_id", "") + + def _background_add_room_type_column_txn( + txn: LoggingTransaction, + ) -> bool: + sql = """ + SELECT state.room_id, json FROM event_json + INNER JOIN current_state_events AS state USING (event_id) + WHERE state.room_id > ? AND type = 'm.room.create' + ORDER BY state.room_id + LIMIT ? + """ + + txn.execute(sql, (last_room_id, batch_size)) + room_id_to_create_event_results = txn.fetchall() + + new_last_room_id = None + for room_id, event_json in room_id_to_create_event_results: + event_dict = db_to_json(event_json) + + room_type = event_dict.get("content", {}).get( + EventContentFields.ROOM_TYPE, None + ) + if isinstance(room_type, str): + self.db_pool.simple_update_txn( + txn, + table="room_stats_state", + keyvalues={"room_id": room_id}, + updatevalues={"room_type": room_type}, + ) + + new_last_room_id = room_id + + if new_last_room_id is None: + return True + + self.db_pool.updates._background_update_progress_txn( + txn, + _BackgroundUpdates.ADD_ROOM_TYPE_COLUMN, + {"room_id": new_last_room_id}, + ) + + return False + + end = await self.db_pool.runInteraction( + "_background_add_room_type_column", + _background_add_room_type_column_txn, + ) + + if end: + await self.db_pool.updates._end_background_update( + _BackgroundUpdates.ADD_ROOM_TYPE_COLUMN + ) + + return batch_size + class RoomStore(RoomBackgroundUpdateStore, RoomWorkerStore): def __init__( diff --git a/synapse/storage/databases/main/stats.py b/synapse/storage/databases/main/stats.py index 82851ffa95..b4c652acf3 100644 --- a/synapse/storage/databases/main/stats.py +++ b/synapse/storage/databases/main/stats.py @@ -16,7 +16,7 @@ import logging from enum import Enum from itertools import chain -from typing import TYPE_CHECKING, Any, Dict, List, Optional, Tuple, cast +from typing import TYPE_CHECKING, Any, Dict, List, Optional, Tuple, Union, cast from typing_extensions import Counter @@ -238,6 +238,7 @@ class StatsStore(StateDeltasStore): * avatar * canonical_alias * guest_access + * room_type A is_federatable key can also be included with a boolean value. @@ -263,6 +264,7 @@ class StatsStore(StateDeltasStore): "avatar", "canonical_alias", "guest_access", + "room_type", ): field = fields.get(col, sentinel) if field is not sentinel and (not isinstance(field, str) or "\0" in field): @@ -572,7 +574,7 @@ class StatsStore(StateDeltasStore): state_event_map = await self.get_events(event_ids, get_prev_content=False) # type: ignore[attr-defined] - room_state = { + room_state: Dict[str, Union[None, bool, str]] = { "join_rules": None, "history_visibility": None, "encryption": None, @@ -581,6 +583,7 @@ class StatsStore(StateDeltasStore): "avatar": None, "canonical_alias": None, "is_federatable": True, + "room_type": None, } for event in state_event_map.values(): @@ -604,6 +607,9 @@ class StatsStore(StateDeltasStore): room_state["is_federatable"] = ( event.content.get(EventContentFields.FEDERATE, True) is True ) + room_type = event.content.get(EventContentFields.ROOM_TYPE) + if isinstance(room_type, str): + room_state["room_type"] = room_type await self.update_room_state(room_id, room_state) diff --git a/synapse/storage/schema/main/delta/72/01add_room_type_to_state_stats.sql b/synapse/storage/schema/main/delta/72/01add_room_type_to_state_stats.sql new file mode 100644 index 0000000000..d5e0765471 --- /dev/null +++ b/synapse/storage/schema/main/delta/72/01add_room_type_to_state_stats.sql @@ -0,0 +1,19 @@ +/* Copyright 2022 The Matrix.org Foundation C.I.C + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +ALTER TABLE room_stats_state ADD room_type TEXT; + +INSERT INTO background_updates (update_name, progress_json) + VALUES ('add_room_type_column', '{}'); diff --git a/tests/rest/client/test_rooms.py b/tests/rest/client/test_rooms.py index 35c59ee9e0..1ccd96a207 100644 --- a/tests/rest/client/test_rooms.py +++ b/tests/rest/client/test_rooms.py @@ -18,7 +18,7 @@ """Tests REST events for /rooms paths.""" import json -from typing import Any, Dict, Iterable, List, Optional, Union +from typing import Any, Dict, Iterable, List, Optional, Tuple, Union from unittest.mock import Mock, call from urllib import parse as urlparse @@ -33,7 +33,9 @@ from synapse.api.constants import ( EventContentFields, EventTypes, Membership, + PublicRoomsFilterFields, RelationTypes, + RoomTypes, ) from synapse.api.errors import Codes, HttpResponseException from synapse.handlers.pagination import PurgeStatus @@ -1858,6 +1860,90 @@ class PublicRoomsRestrictedTestCase(unittest.HomeserverTestCase): self.assertEqual(channel.code, 200, channel.result) +class PublicRoomsRoomTypeFilterTestCase(unittest.HomeserverTestCase): + + servlets = [ + synapse.rest.admin.register_servlets_for_client_rest_resource, + room.register_servlets, + login.register_servlets, + ] + + def make_homeserver(self, reactor: MemoryReactor, clock: Clock) -> HomeServer: + + config = self.default_config() + config["allow_public_rooms_without_auth"] = True + config["experimental_features"] = {"msc3827_enabled": True} + self.hs = self.setup_test_homeserver(config=config) + self.url = b"/_matrix/client/r0/publicRooms" + + return self.hs + + def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: + user = self.register_user("alice", "pass") + self.token = self.login(user, "pass") + + # Create a room + self.helper.create_room_as( + user, + is_public=True, + extra_content={"visibility": "public"}, + tok=self.token, + ) + # Create a space + self.helper.create_room_as( + user, + is_public=True, + extra_content={ + "visibility": "public", + "creation_content": {EventContentFields.ROOM_TYPE: RoomTypes.SPACE}, + }, + tok=self.token, + ) + + def make_public_rooms_request( + self, room_types: Union[List[Union[str, None]], None] + ) -> Tuple[List[Dict[str, Any]], int]: + channel = self.make_request( + "POST", + self.url, + {"filter": {PublicRoomsFilterFields.ROOM_TYPES: room_types}}, + self.token, + ) + chunk = channel.json_body["chunk"] + count = channel.json_body["total_room_count_estimate"] + + self.assertEqual(len(chunk), count) + + return chunk, count + + def test_returns_both_rooms_and_spaces_if_no_filter(self) -> None: + chunk, count = self.make_public_rooms_request(None) + + self.assertEqual(count, 2) + + def test_returns_only_rooms_based_on_filter(self) -> None: + chunk, count = self.make_public_rooms_request([None]) + + self.assertEqual(count, 1) + self.assertEqual(chunk[0].get("org.matrix.msc3827.room_type", None), None) + + def test_returns_only_space_based_on_filter(self) -> None: + chunk, count = self.make_public_rooms_request(["m.space"]) + + self.assertEqual(count, 1) + self.assertEqual(chunk[0].get("org.matrix.msc3827.room_type", None), "m.space") + + def test_returns_both_rooms_and_space_based_on_filter(self) -> None: + chunk, count = self.make_public_rooms_request(["m.space", None]) + + self.assertEqual(count, 2) + + def test_returns_both_rooms_and_spaces_if_array_is_empty(self) -> None: + chunk, count = self.make_public_rooms_request([]) + + self.assertEqual(count, 2) + + class PublicRoomsTestRemoteSearchFallbackTestCase(unittest.HomeserverTestCase): """Test that we correctly fallback to local filtering if a remote server doesn't support search. @@ -1882,7 +1968,7 @@ class PublicRoomsTestRemoteSearchFallbackTestCase(unittest.HomeserverTestCase): "Simple test for searching rooms over federation" self.federation_client.get_public_rooms.return_value = make_awaitable({}) # type: ignore[attr-defined] - search_filter = {"generic_search_term": "foobar"} + search_filter = {PublicRoomsFilterFields.GENERIC_SEARCH_TERM: "foobar"} channel = self.make_request( "POST", @@ -1911,7 +1997,7 @@ class PublicRoomsTestRemoteSearchFallbackTestCase(unittest.HomeserverTestCase): make_awaitable({}), ) - search_filter = {"generic_search_term": "foobar"} + search_filter = {PublicRoomsFilterFields.GENERIC_SEARCH_TERM: "foobar"} channel = self.make_request( "POST", diff --git a/tests/storage/databases/main/test_room.py b/tests/storage/databases/main/test_room.py index 9abd0cb446..1edb619630 100644 --- a/tests/storage/databases/main/test_room.py +++ b/tests/storage/databases/main/test_room.py @@ -12,6 +12,9 @@ # See the License for the specific language governing permissions and # limitations under the License. +import json + +from synapse.api.constants import RoomTypes from synapse.rest import admin from synapse.rest.client import login, room from synapse.storage.databases.main.room import _BackgroundUpdates @@ -91,3 +94,69 @@ class RoomBackgroundUpdateStoreTestCase(HomeserverTestCase): ) ) self.assertEqual(room_creator_after, self.user_id) + + def test_background_add_room_type_column(self): + """Test that the background update to populate the `room_type` column in + `room_stats_state` works properly. + """ + + # Create a room without a type + room_id = self._generate_room() + + # Get event_id of the m.room.create event + event_id = self.get_success( + self.store.db_pool.simple_select_one_onecol( + table="current_state_events", + keyvalues={ + "room_id": room_id, + "type": "m.room.create", + }, + retcol="event_id", + ) + ) + + # Fake a room creation event with a room type + event = { + "content": { + "creator": "@user:server.org", + "room_version": "9", + "type": RoomTypes.SPACE, + }, + "type": "m.room.create", + } + self.get_success( + self.store.db_pool.simple_update( + table="event_json", + keyvalues={"event_id": event_id}, + updatevalues={"json": json.dumps(event)}, + desc="test", + ) + ) + + # Insert and run the background update + self.get_success( + self.store.db_pool.simple_insert( + "background_updates", + { + "update_name": _BackgroundUpdates.ADD_ROOM_TYPE_COLUMN, + "progress_json": "{}", + }, + ) + ) + + # ... and tell the DataStore that it hasn't finished all updates yet + self.store.db_pool.updates._all_done = False + + # Now let's actually drive the updates to completion + self.wait_for_background_updates() + + # Make sure the background update filled in the room type + room_type_after = self.get_success( + self.store.db_pool.simple_select_one_onecol( + table="room_stats_state", + keyvalues={"room_id": room_id}, + retcol="room_type", + allow_none=True, + ) + ) + self.assertEqual(room_type_after, RoomTypes.SPACE) From 4d3b8fb23f0c9288b311efd7def83b641bda82b8 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Thu, 30 Jun 2022 10:43:24 +0200 Subject: [PATCH 05/35] Don't actually one-line the SQL statements we send to the DB (#13129) --- changelog.d/13129.misc | 1 + synapse/storage/database.py | 7 ++++--- 2 files changed, 5 insertions(+), 3 deletions(-) create mode 100644 changelog.d/13129.misc diff --git a/changelog.d/13129.misc b/changelog.d/13129.misc new file mode 100644 index 0000000000..4c2dbb7057 --- /dev/null +++ b/changelog.d/13129.misc @@ -0,0 +1 @@ +Only one-line SQL statements for logging and tracing. diff --git a/synapse/storage/database.py b/synapse/storage/database.py index e8c63cf567..e21ab08515 100644 --- a/synapse/storage/database.py +++ b/synapse/storage/database.py @@ -366,10 +366,11 @@ class LoggingTransaction: *args: P.args, **kwargs: P.kwargs, ) -> R: - sql = self._make_sql_one_line(sql) + # Generate a one-line version of the SQL to better log it. + one_line_sql = self._make_sql_one_line(sql) # TODO(paul): Maybe use 'info' and 'debug' for values? - sql_logger.debug("[SQL] {%s} %s", self.name, sql) + sql_logger.debug("[SQL] {%s} %s", self.name, one_line_sql) sql = self.database_engine.convert_param_style(sql) if args: @@ -389,7 +390,7 @@ class LoggingTransaction: "db.query", tags={ opentracing.tags.DATABASE_TYPE: "sql", - opentracing.tags.DATABASE_STATEMENT: sql, + opentracing.tags.DATABASE_STATEMENT: one_line_sql, }, ): return func(sql, *args, **kwargs) From 80c7a06777507beb5401718dd07fbcb1cd377de1 Mon Sep 17 00:00:00 2001 From: David Teller Date: Thu, 30 Jun 2022 11:44:47 +0200 Subject: [PATCH 06/35] Rate limiting invites per issuer (#13125) Co-authored-by: reivilibre --- changelog.d/13125.feature | 1 + synapse/config/ratelimiting.py | 5 +++++ synapse/handlers/room_member.py | 20 ++++++++++++++++++-- 3 files changed, 24 insertions(+), 2 deletions(-) create mode 100644 changelog.d/13125.feature diff --git a/changelog.d/13125.feature b/changelog.d/13125.feature new file mode 100644 index 0000000000..9b0f609541 --- /dev/null +++ b/changelog.d/13125.feature @@ -0,0 +1 @@ +Add a rate limit for local users sending invites. \ No newline at end of file diff --git a/synapse/config/ratelimiting.py b/synapse/config/ratelimiting.py index d4090a1f9a..4fc1784efe 100644 --- a/synapse/config/ratelimiting.py +++ b/synapse/config/ratelimiting.py @@ -136,6 +136,11 @@ class RatelimitConfig(Config): defaults={"per_second": 0.003, "burst_count": 5}, ) + self.rc_invites_per_issuer = RateLimitConfig( + config.get("rc_invites", {}).get("per_issuer", {}), + defaults={"per_second": 0.3, "burst_count": 10}, + ) + self.rc_third_party_invite = RateLimitConfig( config.get("rc_third_party_invite", {}), defaults={ diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index bf6bae1232..5648ab4bf4 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -101,19 +101,33 @@ class RoomMemberHandler(metaclass=abc.ABCMeta): burst_count=hs.config.ratelimiting.rc_joins_remote.burst_count, ) + # Ratelimiter for invites, keyed by room (across all issuers, all + # recipients). self._invites_per_room_limiter = Ratelimiter( store=self.store, clock=self.clock, rate_hz=hs.config.ratelimiting.rc_invites_per_room.per_second, burst_count=hs.config.ratelimiting.rc_invites_per_room.burst_count, ) - self._invites_per_user_limiter = Ratelimiter( + + # Ratelimiter for invites, keyed by recipient (across all rooms, all + # issuers). + self._invites_per_recipient_limiter = Ratelimiter( store=self.store, clock=self.clock, rate_hz=hs.config.ratelimiting.rc_invites_per_user.per_second, burst_count=hs.config.ratelimiting.rc_invites_per_user.burst_count, ) + # Ratelimiter for invites, keyed by issuer (across all rooms, all + # recipients). + self._invites_per_issuer_limiter = Ratelimiter( + store=self.store, + clock=self.clock, + rate_hz=hs.config.ratelimiting.rc_invites_per_issuer.per_second, + burst_count=hs.config.ratelimiting.rc_invites_per_issuer.burst_count, + ) + self._third_party_invite_limiter = Ratelimiter( store=self.store, clock=self.clock, @@ -258,7 +272,9 @@ class RoomMemberHandler(metaclass=abc.ABCMeta): if room_id: await self._invites_per_room_limiter.ratelimit(requester, room_id) - await self._invites_per_user_limiter.ratelimit(requester, invitee_user_id) + await self._invites_per_recipient_limiter.ratelimit(requester, invitee_user_id) + if requester is not None: + await self._invites_per_issuer_limiter.ratelimit(requester) async def _local_membership_update( self, From 09f6e430254889a8633787a09f154075a17aff23 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 30 Jun 2022 11:45:47 +0200 Subject: [PATCH 07/35] Actually typecheck `tests.test_server` (#13135) --- changelog.d/13135.misc | 1 + mypy.ini | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) create mode 100644 changelog.d/13135.misc diff --git a/changelog.d/13135.misc b/changelog.d/13135.misc new file mode 100644 index 0000000000..f096dd8749 --- /dev/null +++ b/changelog.d/13135.misc @@ -0,0 +1 @@ +Enforce type annotations for `tests.test_server`. diff --git a/mypy.ini b/mypy.ini index 4b08f45c6d..e062cf43a2 100644 --- a/mypy.ini +++ b/mypy.ini @@ -56,7 +56,6 @@ exclude = (?x) |tests/server.py |tests/server_notices/test_resource_limits_server_notices.py |tests/test_metrics.py - |tests/test_server.py |tests/test_state.py |tests/test_terms_auth.py |tests/util/caches/test_cached_call.py From 9667bad55d8b50fe08990a8cfd2ac82c8540bcc1 Mon Sep 17 00:00:00 2001 From: reivilibre Date: Thu, 30 Jun 2022 12:58:12 +0100 Subject: [PATCH 08/35] Improve startup times in Complement test runs against workers, particularly in CPU-constrained environments. (#13127) Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- .github/workflows/tests.yml | 49 +---- changelog.d/13127.misc | 1 + .../complement/conf/start_for_complement.sh | 3 + .../conf-workers/synapse.supervisord.conf.j2 | 26 ++- docker/conf/log.config | 4 + docker/configure_workers_and_start.py | 7 + docker/start.py | 6 +- synapse/app/_base.py | 8 +- synapse/app/complement_fork_starter.py | 190 ++++++++++++++++++ 9 files changed, 243 insertions(+), 51 deletions(-) create mode 100644 changelog.d/13127.misc create mode 100644 synapse/app/complement_fork_starter.py diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 2e4ee723d3..a775f70c4e 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -328,51 +328,8 @@ jobs: - arrangement: monolith database: Postgres - steps: - # The path is set via a file given by $GITHUB_PATH. We need both Go 1.17 and GOPATH on the path to run Complement. - # See https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions#adding-a-system-path - - name: "Set Go Version" - run: | - # Add Go 1.17 to the PATH: see https://github.com/actions/virtual-environments/blob/main/images/linux/Ubuntu2004-Readme.md#environment-variables-2 - echo "$GOROOT_1_17_X64/bin" >> $GITHUB_PATH - # Add the Go path to the PATH: We need this so we can call gotestfmt - echo "~/go/bin" >> $GITHUB_PATH - - - name: "Install Complement Dependencies" - run: | - sudo apt-get update && sudo apt-get install -y libolm3 libolm-dev - go get -v github.com/haveyoudebuggedit/gotestfmt/v2/cmd/gotestfmt@latest - - - name: Run actions/checkout@v2 for synapse - uses: actions/checkout@v2 - with: - path: synapse - - - name: "Install custom gotestfmt template" - run: | - mkdir .gotestfmt/github -p - cp synapse/.ci/complement_package.gotpl .gotestfmt/github/package.gotpl - - # Attempt to check out the same branch of Complement as the PR. If it - # doesn't exist, fallback to HEAD. - - name: Checkout complement - run: synapse/.ci/scripts/checkout_complement.sh - - - run: | - set -o pipefail - POSTGRES=${{ (matrix.database == 'Postgres') && 1 || '' }} COMPLEMENT_DIR=`pwd`/complement synapse/scripts-dev/complement.sh -json 2>&1 | gotestfmt - shell: bash - name: Run Complement Tests - - # We only run the workers tests on `develop` for now, because they're too slow to wait for on PRs. - # Sadly, you can't have an `if` condition on the value of a matrix, so this is a temporary, separate job for now. - # GitHub Actions doesn't support YAML anchors, so it's full-on duplication for now. - complement-developonly: - if: "${{ !failure() && !cancelled() && (github.ref == 'refs/heads/develop') }}" - needs: linting-done - runs-on: ubuntu-latest - - name: "Complement Workers (develop only)" + - arrangement: workers + database: Postgres steps: # The path is set via a file given by $GITHUB_PATH. We need both Go 1.17 and GOPATH on the path to run Complement. @@ -406,7 +363,7 @@ jobs: - run: | set -o pipefail - WORKERS=1 COMPLEMENT_DIR=`pwd`/complement synapse/scripts-dev/complement.sh -json 2>&1 | gotestfmt + POSTGRES=${{ (matrix.database == 'Postgres') && 1 || '' }} WORKERS=${{ (matrix.arrangement == 'workers') && 1 || '' }} COMPLEMENT_DIR=`pwd`/complement synapse/scripts-dev/complement.sh -json 2>&1 | gotestfmt shell: bash name: Run Complement Tests diff --git a/changelog.d/13127.misc b/changelog.d/13127.misc new file mode 100644 index 0000000000..1414811e0a --- /dev/null +++ b/changelog.d/13127.misc @@ -0,0 +1 @@ +Improve startup times in Complement test runs against workers, particularly in CPU-constrained environments. \ No newline at end of file diff --git a/docker/complement/conf/start_for_complement.sh b/docker/complement/conf/start_for_complement.sh index 773c7db22f..cc6482f763 100755 --- a/docker/complement/conf/start_for_complement.sh +++ b/docker/complement/conf/start_for_complement.sh @@ -59,6 +59,9 @@ if [[ -n "$SYNAPSE_COMPLEMENT_USE_WORKERS" ]]; then synchrotron, \ appservice, \ pusher" + + # Improve startup times by using a launcher based on fork() + export SYNAPSE_USE_EXPERIMENTAL_FORKING_LAUNCHER=1 else # Empty string here means 'main process only' export SYNAPSE_WORKER_TYPES="" diff --git a/docker/conf-workers/synapse.supervisord.conf.j2 b/docker/conf-workers/synapse.supervisord.conf.j2 index 6443450491..481eb4fc92 100644 --- a/docker/conf-workers/synapse.supervisord.conf.j2 +++ b/docker/conf-workers/synapse.supervisord.conf.j2 @@ -1,3 +1,24 @@ +{% if use_forking_launcher %} +[program:synapse_fork] +command=/usr/local/bin/python -m synapse.app.complement_fork_starter + {{ main_config_path }} + synapse.app.homeserver + --config-path="{{ main_config_path }}" + --config-path=/conf/workers/shared.yaml + {%- for worker in workers %} + -- {{ worker.app }} + --config-path="{{ main_config_path }}" + --config-path=/conf/workers/shared.yaml + --config-path=/conf/workers/{{ worker.name }}.yaml + {%- endfor %} +stdout_logfile=/dev/stdout +stdout_logfile_maxbytes=0 +stderr_logfile=/dev/stderr +stderr_logfile_maxbytes=0 +autorestart=unexpected +exitcodes=0 + +{% else %} [program:synapse_main] command=/usr/local/bin/prefix-log /usr/local/bin/python -m synapse.app.homeserver --config-path="{{ main_config_path }}" @@ -13,7 +34,7 @@ autorestart=unexpected exitcodes=0 -{% for worker in workers %} + {% for worker in workers %} [program:synapse_{{ worker.name }}] command=/usr/local/bin/prefix-log /usr/local/bin/python -m {{ worker.app }} --config-path="{{ main_config_path }}" @@ -27,4 +48,5 @@ stdout_logfile_maxbytes=0 stderr_logfile=/dev/stderr stderr_logfile_maxbytes=0 -{% endfor %} + {% endfor %} +{% endif %} diff --git a/docker/conf/log.config b/docker/conf/log.config index dc8c70befd..d9e85aa533 100644 --- a/docker/conf/log.config +++ b/docker/conf/log.config @@ -2,7 +2,11 @@ version: 1 formatters: precise: + {% if include_worker_name_in_log_line %} + format: '{{ worker_name }} | %(asctime)s - %(name)s - %(lineno)d - %(levelname)s - %(request)s - %(message)s' + {% else %} format: '%(asctime)s - %(name)s - %(lineno)d - %(levelname)s - %(request)s - %(message)s' + {% endif %} handlers: {% if LOG_FILE_PATH %} diff --git a/docker/configure_workers_and_start.py b/docker/configure_workers_and_start.py index 2134b648d5..4521f99eb4 100755 --- a/docker/configure_workers_and_start.py +++ b/docker/configure_workers_and_start.py @@ -26,6 +26,9 @@ # * SYNAPSE_TLS_CERT: Path to a TLS certificate in PEM format. # * SYNAPSE_TLS_KEY: Path to a TLS key. If this and SYNAPSE_TLS_CERT are specified, # Nginx will be configured to serve TLS on port 8448. +# * SYNAPSE_USE_EXPERIMENTAL_FORKING_LAUNCHER: Whether to use the forking launcher, +# only intended for usage in Complement at the moment. +# No stability guarantees are provided. # # NOTE: According to Complement's ENTRYPOINT expectations for a homeserver image (as defined # in the project's README), this script may be run multiple times, and functionality should @@ -525,6 +528,7 @@ def generate_worker_files( "/etc/supervisor/conf.d/synapse.conf", workers=worker_descriptors, main_config_path=config_path, + use_forking_launcher=environ.get("SYNAPSE_USE_EXPERIMENTAL_FORKING_LAUNCHER"), ) # healthcheck config @@ -560,6 +564,9 @@ def generate_worker_log_config( log_config_filepath, worker_name=worker_name, **extra_log_template_args, + include_worker_name_in_log_line=environ.get( + "SYNAPSE_USE_EXPERIMENTAL_FORKING_LAUNCHER" + ), ) return log_config_filepath diff --git a/docker/start.py b/docker/start.py index 4ac8f03477..5a98dce551 100755 --- a/docker/start.py +++ b/docker/start.py @@ -110,7 +110,11 @@ def generate_config_from_template( log_config_file = environ["SYNAPSE_LOG_CONFIG"] log("Generating log config file " + log_config_file) - convert("/conf/log.config", log_config_file, environ) + convert( + "/conf/log.config", + log_config_file, + {**environ, "include_worker_name_in_log_line": False}, + ) # Hopefully we already have a signing key, but generate one if not. args = [ diff --git a/synapse/app/_base.py b/synapse/app/_base.py index 363ac98ea9..923891ae0d 100644 --- a/synapse/app/_base.py +++ b/synapse/app/_base.py @@ -106,7 +106,9 @@ def register_sighup(func: Callable[P, None], *args: P.args, **kwargs: P.kwargs) def start_worker_reactor( appname: str, config: HomeServerConfig, - run_command: Callable[[], None] = reactor.run, + # Use a lambda to avoid binding to a given reactor at import time. + # (needed when synapse.app.complement_fork_starter is being used) + run_command: Callable[[], None] = lambda: reactor.run(), ) -> None: """Run the reactor in the main process @@ -141,7 +143,9 @@ def start_reactor( daemonize: bool, print_pidfile: bool, logger: logging.Logger, - run_command: Callable[[], None] = reactor.run, + # Use a lambda to avoid binding to a given reactor at import time. + # (needed when synapse.app.complement_fork_starter is being used) + run_command: Callable[[], None] = lambda: reactor.run(), ) -> None: """Run the reactor in the main process diff --git a/synapse/app/complement_fork_starter.py b/synapse/app/complement_fork_starter.py new file mode 100644 index 0000000000..89eb07df27 --- /dev/null +++ b/synapse/app/complement_fork_starter.py @@ -0,0 +1,190 @@ +# Copyright 2022 The Matrix.org Foundation C.I.C. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +# ## What this script does +# +# This script spawns multiple workers, whilst only going through the code loading +# process once. The net effect is that start-up time for a swarm of workers is +# reduced, particularly in CPU-constrained environments. +# +# Before the workers are spawned, the database is prepared in order to avoid the +# workers racing. +# +# ## Stability +# +# This script is only intended for use within the Synapse images for the +# Complement test suite. +# There are currently no stability guarantees whatsoever; especially not about: +# - whether it will continue to exist in future versions; +# - the format of its command-line arguments; or +# - any details about its behaviour or principles of operation. +# +# ## Usage +# +# The first argument should be the path to the database configuration, used to +# set up the database. The rest of the arguments are used as follows: +# Each worker is specified as an argument group (each argument group is +# separated by '--'). +# The first argument in each argument group is the Python module name of the application +# to start. Further arguments are then passed to that module as-is. +# +# ## Example +# +# python -m synapse.app.complement_fork_starter path_to_db_config.yaml \ +# synapse.app.homeserver [args..] -- \ +# synapse.app.generic_worker [args..] -- \ +# ... +# synapse.app.generic_worker [args..] +# +import argparse +import importlib +import itertools +import multiprocessing +import sys +from typing import Any, Callable, List + +from twisted.internet.main import installReactor + + +class ProxiedReactor: + """ + Twisted tracks the 'installed' reactor as a global variable. + (Actually, it does some module trickery, but the effect is similar.) + + The default EpollReactor is buggy if it's created before a process is + forked, then used in the child. + See https://twistedmatrix.com/trac/ticket/4759#comment:17. + + However, importing certain Twisted modules will automatically create and + install a reactor if one hasn't already been installed. + It's not normally possible to re-install a reactor. + + Given the goal of launching workers with fork() to only import the code once, + this presents a conflict. + Our work around is to 'install' this ProxiedReactor which prevents Twisted + from creating and installing one, but which lets us replace the actual reactor + in use later on. + """ + + def __init__(self) -> None: + self.___reactor_target: Any = None + + def _install_real_reactor(self, new_reactor: Any) -> None: + """ + Install a real reactor for this ProxiedReactor to forward lookups onto. + + This method is specific to our ProxiedReactor and should not clash with + any names used on an actual Twisted reactor. + """ + self.___reactor_target = new_reactor + + def __getattr__(self, attr_name: str) -> Any: + return getattr(self.___reactor_target, attr_name) + + +def _worker_entrypoint( + func: Callable[[], None], proxy_reactor: ProxiedReactor, args: List[str] +) -> None: + """ + Entrypoint for a forked worker process. + + We just need to set up the command-line arguments, create our real reactor + and then kick off the worker's main() function. + """ + + sys.argv = args + + from twisted.internet.epollreactor import EPollReactor + + proxy_reactor._install_real_reactor(EPollReactor()) + func() + + +def main() -> None: + """ + Entrypoint for the forking launcher. + """ + parser = argparse.ArgumentParser() + parser.add_argument("db_config", help="Path to database config file") + parser.add_argument( + "args", + nargs="...", + help="Argument groups separated by `--`. " + "The first argument of each group is a Synapse app name. " + "Subsequent arguments are passed through.", + ) + ns = parser.parse_args() + + # Split up the subsequent arguments into each workers' arguments; + # `--` is our delimiter of choice. + args_by_worker: List[List[str]] = [ + list(args) + for cond, args in itertools.groupby(ns.args, lambda ele: ele != "--") + if cond and args + ] + + # Prevent Twisted from installing a shared reactor that all the workers will + # inherit when we fork(), by installing our own beforehand. + proxy_reactor = ProxiedReactor() + installReactor(proxy_reactor) + + # Import the entrypoints for all the workers. + worker_functions = [] + for worker_args in args_by_worker: + worker_module = importlib.import_module(worker_args[0]) + worker_functions.append(worker_module.main) + + # We need to prepare the database first as otherwise all the workers will + # try to create a schema version table and some will crash out. + from synapse._scripts import update_synapse_database + + update_proc = multiprocessing.Process( + target=_worker_entrypoint, + args=( + update_synapse_database.main, + proxy_reactor, + [ + "update_synapse_database", + "--database-config", + ns.db_config, + "--run-background-updates", + ], + ), + ) + print("===== PREPARING DATABASE =====", file=sys.stderr) + update_proc.start() + update_proc.join() + print("===== PREPARED DATABASE =====", file=sys.stderr) + + # At this point, we've imported all the main entrypoints for all the workers. + # Now we basically just fork() out to create the workers we need. + # Because we're using fork(), all the workers get a clone of this launcher's + # memory space and don't need to repeat the work of loading the code! + # Instead of using fork() directly, we use the multiprocessing library, + # which uses fork() on Unix platforms. + processes = [] + for (func, worker_args) in zip(worker_functions, args_by_worker): + process = multiprocessing.Process( + target=_worker_entrypoint, args=(func, proxy_reactor, worker_args) + ) + process.start() + processes.append(process) + + # Be a good parent and wait for our children to die before exiting. + for process in processes: + process.join() + + +if __name__ == "__main__": + main() From 6ad012ef89c966cbb3616c1be63d964db48d49ca Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 30 Jun 2022 09:05:06 -0400 Subject: [PATCH 09/35] More type hints for `synapse.logging` (#13103) Completes type hints for synapse.logging.scopecontextmanager and (partially) for synapse.logging.opentracing. --- changelog.d/13103.misc | 1 + mypy.ini | 3 -- synapse/logging/opentracing.py | 61 +++++++++++++++----------- synapse/logging/scopecontextmanager.py | 35 ++++++++------- tests/logging/test_opentracing.py | 2 +- 5 files changed, 56 insertions(+), 46 deletions(-) create mode 100644 changelog.d/13103.misc diff --git a/changelog.d/13103.misc b/changelog.d/13103.misc new file mode 100644 index 0000000000..4de5f9e905 --- /dev/null +++ b/changelog.d/13103.misc @@ -0,0 +1 @@ +Add missing type hints to `synapse.logging`. diff --git a/mypy.ini b/mypy.ini index e062cf43a2..b9b16860db 100644 --- a/mypy.ini +++ b/mypy.ini @@ -88,9 +88,6 @@ disallow_untyped_defs = False [mypy-synapse.logging.opentracing] disallow_untyped_defs = False -[mypy-synapse.logging.scopecontextmanager] -disallow_untyped_defs = False - [mypy-synapse.metrics._reactor_metrics] disallow_untyped_defs = False # This module imports select.epoll. That exists on Linux, but doesn't on macOS. diff --git a/synapse/logging/opentracing.py b/synapse/logging/opentracing.py index 903ec40c86..50c57940f9 100644 --- a/synapse/logging/opentracing.py +++ b/synapse/logging/opentracing.py @@ -164,6 +164,7 @@ Gotchas with an active span? """ import contextlib +import enum import inspect import logging import re @@ -268,7 +269,7 @@ try: _reporter: Reporter = attr.Factory(Reporter) - def set_process(self, *args, **kwargs): + def set_process(self, *args: Any, **kwargs: Any) -> None: return self._reporter.set_process(*args, **kwargs) def report_span(self, span: "opentracing.Span") -> None: @@ -319,7 +320,11 @@ _homeserver_whitelist: Optional[Pattern[str]] = None # Util methods -Sentinel = object() + +class _Sentinel(enum.Enum): + # defining a sentinel in this way allows mypy to correctly handle the + # type of a dictionary lookup. + sentinel = object() P = ParamSpec("P") @@ -339,12 +344,12 @@ def only_if_tracing(func: Callable[P, R]) -> Callable[P, Optional[R]]: return _only_if_tracing_inner -def ensure_active_span(message, ret=None): +def ensure_active_span(message: str, ret=None): """Executes the operation only if opentracing is enabled and there is an active span. If there is no active span it logs message at the error level. Args: - message (str): Message which fills in "There was no active span when trying to %s" + message: Message which fills in "There was no active span when trying to %s" in the error log if there is no active span and opentracing is enabled. ret (object): return value if opentracing is None or there is no active span. @@ -402,7 +407,7 @@ def init_tracer(hs: "HomeServer") -> None: config = JaegerConfig( config=hs.config.tracing.jaeger_config, service_name=f"{hs.config.server.server_name} {hs.get_instance_name()}", - scope_manager=LogContextScopeManager(hs.config), + scope_manager=LogContextScopeManager(), metrics_factory=PrometheusMetricsFactory(), ) @@ -451,15 +456,15 @@ def whitelisted_homeserver(destination: str) -> bool: # Could use kwargs but I want these to be explicit def start_active_span( - operation_name, - child_of=None, - references=None, - tags=None, - start_time=None, - ignore_active_span=False, - finish_on_close=True, + operation_name: str, + child_of: Optional[Union["opentracing.Span", "opentracing.SpanContext"]] = None, + references: Optional[List["opentracing.Reference"]] = None, + tags: Optional[Dict[str, str]] = None, + start_time: Optional[float] = None, + ignore_active_span: bool = False, + finish_on_close: bool = True, *, - tracer=None, + tracer: Optional["opentracing.Tracer"] = None, ): """Starts an active opentracing span. @@ -493,11 +498,11 @@ def start_active_span( def start_active_span_follows_from( operation_name: str, contexts: Collection, - child_of=None, + child_of: Optional[Union["opentracing.Span", "opentracing.SpanContext"]] = None, start_time: Optional[float] = None, *, - inherit_force_tracing=False, - tracer=None, + inherit_force_tracing: bool = False, + tracer: Optional["opentracing.Tracer"] = None, ): """Starts an active opentracing span, with additional references to previous spans @@ -540,7 +545,7 @@ def start_active_span_from_edu( edu_content: Dict[str, Any], operation_name: str, references: Optional[List["opentracing.Reference"]] = None, - tags: Optional[Dict] = None, + tags: Optional[Dict[str, str]] = None, start_time: Optional[float] = None, ignore_active_span: bool = False, finish_on_close: bool = True, @@ -617,23 +622,27 @@ def set_operation_name(operation_name: str) -> None: @only_if_tracing -def force_tracing(span=Sentinel) -> None: +def force_tracing( + span: Union["opentracing.Span", _Sentinel] = _Sentinel.sentinel +) -> None: """Force sampling for the active/given span and its children. Args: span: span to force tracing for. By default, the active span. """ - if span is Sentinel: - span = opentracing.tracer.active_span - if span is None: + if isinstance(span, _Sentinel): + span_to_trace = opentracing.tracer.active_span + else: + span_to_trace = span + if span_to_trace is None: logger.error("No active span in force_tracing") return - span.set_tag(opentracing.tags.SAMPLING_PRIORITY, 1) + span_to_trace.set_tag(opentracing.tags.SAMPLING_PRIORITY, 1) # also set a bit of baggage, so that we have a way of figuring out if # it is enabled later - span.set_baggage_item(SynapseBaggage.FORCE_TRACING, "1") + span_to_trace.set_baggage_item(SynapseBaggage.FORCE_TRACING, "1") def is_context_forced_tracing( @@ -789,7 +798,7 @@ def extract_text_map(carrier: Dict[str, str]) -> Optional["opentracing.SpanConte # Tracing decorators -def trace(func=None, opname=None): +def trace(func=None, opname: Optional[str] = None): """ Decorator to trace a function. Sets the operation name to that of the function's or that given @@ -822,11 +831,11 @@ def trace(func=None, opname=None): result = func(*args, **kwargs) if isinstance(result, defer.Deferred): - def call_back(result): + def call_back(result: R) -> R: scope.__exit__(None, None, None) return result - def err_back(result): + def err_back(result: R) -> R: scope.__exit__(None, None, None) return result diff --git a/synapse/logging/scopecontextmanager.py b/synapse/logging/scopecontextmanager.py index a26a1a58e7..10877bdfc5 100644 --- a/synapse/logging/scopecontextmanager.py +++ b/synapse/logging/scopecontextmanager.py @@ -16,11 +16,15 @@ import logging from types import TracebackType from typing import Optional, Type -from opentracing import Scope, ScopeManager +from opentracing import Scope, ScopeManager, Span import twisted -from synapse.logging.context import current_context, nested_logging_context +from synapse.logging.context import ( + LoggingContext, + current_context, + nested_logging_context, +) logger = logging.getLogger(__name__) @@ -35,11 +39,11 @@ class LogContextScopeManager(ScopeManager): but currently that doesn't work due to https://twistedmatrix.com/trac/ticket/10301. """ - def __init__(self, config): + def __init__(self) -> None: pass @property - def active(self): + def active(self) -> Optional[Scope]: """ Returns the currently active Scope which can be used to access the currently active Scope.span. @@ -48,19 +52,18 @@ class LogContextScopeManager(ScopeManager): Tracer.start_active_span() time. Return: - (Scope) : the Scope that is active, or None if not - available. + The Scope that is active, or None if not available. """ ctx = current_context() return ctx.scope - def activate(self, span, finish_on_close): + def activate(self, span: Span, finish_on_close: bool) -> Scope: """ Makes a Span active. Args - span (Span): the span that should become active. - finish_on_close (Boolean): whether Span should be automatically - finished when Scope.close() is called. + span: the span that should become active. + finish_on_close: whether Span should be automatically finished when + Scope.close() is called. Returns: Scope to control the end of the active period for @@ -112,8 +115,8 @@ class _LogContextScope(Scope): def __init__( self, manager: LogContextScopeManager, - span, - logcontext, + span: Span, + logcontext: LoggingContext, enter_logcontext: bool, finish_on_close: bool, ): @@ -121,13 +124,13 @@ class _LogContextScope(Scope): Args: manager: the manager that is responsible for this scope. - span (Span): + span: the opentracing span which this scope represents the local lifetime for. - logcontext (LogContext): - the logcontext to which this scope is attached. + logcontext: + the log context to which this scope is attached. enter_logcontext: - if True the logcontext will be exited when the scope is finished + if True the log context will be exited when the scope is finished finish_on_close: if True finish the span when the scope is closed """ diff --git a/tests/logging/test_opentracing.py b/tests/logging/test_opentracing.py index e430941d27..40148d503c 100644 --- a/tests/logging/test_opentracing.py +++ b/tests/logging/test_opentracing.py @@ -50,7 +50,7 @@ class LogContextScopeManagerTestCase(TestCase): # global variables that power opentracing. We create our own tracer instance # and test with it. - scope_manager = LogContextScopeManager({}) + scope_manager = LogContextScopeManager() config = jaeger_client.config.Config( config={}, service_name="test", scope_manager=scope_manager ) From 0ceb3af10b88f9f195fd42db12d33dafda8e6261 Mon Sep 17 00:00:00 2001 From: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com> Date: Thu, 30 Jun 2022 15:59:11 +0100 Subject: [PATCH 10/35] Add a link to the configuration manual from the homeserver sample config documentation page (#13139) --- changelog.d/13139.doc | 1 + docs/usage/configuration/homeserver_sample_config.md | 3 +++ 2 files changed, 4 insertions(+) create mode 100644 changelog.d/13139.doc diff --git a/changelog.d/13139.doc b/changelog.d/13139.doc new file mode 100644 index 0000000000..f5d99d461a --- /dev/null +++ b/changelog.d/13139.doc @@ -0,0 +1 @@ +Add a link to the configuration manual from the homeserver sample config documentation. diff --git a/docs/usage/configuration/homeserver_sample_config.md b/docs/usage/configuration/homeserver_sample_config.md index 11e806998d..2dbfb35baa 100644 --- a/docs/usage/configuration/homeserver_sample_config.md +++ b/docs/usage/configuration/homeserver_sample_config.md @@ -9,6 +9,9 @@ a real homeserver.yaml. Instead, if you are starting from scratch, please genera a fresh config using Synapse by following the instructions in [Installation](../../setup/installation.md). +Documentation for all configuration options can be found in the +[Configuration Manual](./config_documentation.md). + ```yaml {{#include ../../sample_config.yaml}} ``` From 8330fc9953032f21eb4c7d5f0627c1e6aba2459c Mon Sep 17 00:00:00 2001 From: Shay Date: Thu, 30 Jun 2022 09:21:39 -0700 Subject: [PATCH 11/35] Cleanup references to sample config in the docs and redirect users to configuration manual (#13077) --- changelog.d/13077.doc | 3 + docs/admin_api/user_admin_api.md | 5 +- docs/code_style.md | 93 +++++++------------ docs/jwt.md | 5 +- docs/manhole.md | 6 +- docs/message_retention_policies.md | 18 ++-- docs/openid.md | 4 +- docs/setup/forward_proxy.md | 4 +- docs/setup/installation.md | 14 +-- .../configuration/config_documentation.md | 2 +- .../user_authentication/single_sign_on/cas.md | 4 +- synapse/config/emailconfig.py | 2 +- 12 files changed, 72 insertions(+), 88 deletions(-) create mode 100644 changelog.d/13077.doc diff --git a/changelog.d/13077.doc b/changelog.d/13077.doc new file mode 100644 index 0000000000..502f2d059e --- /dev/null +++ b/changelog.d/13077.doc @@ -0,0 +1,3 @@ +Clean up references to sample configuration and redirect users to the configuration manual instead. + + diff --git a/docs/admin_api/user_admin_api.md b/docs/admin_api/user_admin_api.md index 62f89e8cba..1235f1cb95 100644 --- a/docs/admin_api/user_admin_api.md +++ b/docs/admin_api/user_admin_api.md @@ -124,9 +124,8 @@ Body parameters: - `address` - string. Value of third-party ID. belonging to a user. - `external_ids` - array, optional. Allow setting the identifier of the external identity - provider for SSO (Single sign-on). Details in - [Sample Configuration File](../usage/configuration/homeserver_sample_config.html) - section `sso` and `oidc_providers`. + provider for SSO (Single sign-on). Details in the configuration manual under the + sections [sso](../usage/configuration/config_documentation.md#sso) and [oidc_providers](../usage/configuration/config_documentation.md#oidc_providers). - `auth_provider` - string. ID of the external identity provider. Value of `idp_id` in the homeserver configuration. Note that no error is raised if the provided value is not in the homeserver configuration. diff --git a/docs/code_style.md b/docs/code_style.md index db7edcd76b..d65fda62d1 100644 --- a/docs/code_style.md +++ b/docs/code_style.md @@ -70,82 +70,61 @@ on save as they take a while and can be very resource intensive. - Avoid wildcard imports (`from synapse.types import *`) and relative imports (`from .types import UserID`). -## Configuration file format +## Configuration code and documentation format -The [sample configuration file](./sample_config.yaml) acts as a +When adding a configuration option to the code, if several settings are grouped into a single dict, ensure that your code +correctly handles the top-level option being set to `None` (as it will be if no sub-options are enabled). + +The [configuration manual](usage/configuration/config_documentation.md) acts as a reference to Synapse's configuration options for server administrators. Remember that many readers will be unfamiliar with YAML and server -administration in general, so that it is important that the file be as -easy to understand as possible, which includes following a consistent -format. +administration in general, so it is important that when you add +a configuration option the documentation be as easy to understand as possible, which +includes following a consistent format. Some guidelines follow: -- Sections should be separated with a heading consisting of a single - line prefixed and suffixed with `##`. There should be **two** blank - lines before the section header, and **one** after. -- Each option should be listed in the file with the following format: - - A comment describing the setting. Each line of this comment - should be prefixed with a hash (`#`) and a space. +- Each option should be listed in the config manual with the following format: + + - The name of the option, prefixed by `###`. - The comment should describe the default behaviour (ie, what + - A comment which describes the default behaviour (i.e. what happens if the setting is omitted), as well as what the effect will be if the setting is changed. - - Often, the comment end with something like "uncomment the - following to ". - - - A line consisting of only `#`. - - A commented-out example setting, prefixed with only `#`. + - An example setting, using backticks to define the code block For boolean (on/off) options, convention is that this example - should be the *opposite* to the default (so the comment will end - with "Uncomment the following to enable [or disable] - ." For other options, the example should give some - non-default value which is likely to be useful to the reader. + should be the *opposite* to the default. For other options, the example should give + some non-default value which is likely to be useful to the reader. -- There should be a blank line between each option. -- Where several settings are grouped into a single dict, *avoid* the - convention where the whole block is commented out, resulting in - comment lines starting `# #`, as this is hard to read and confusing - to edit. Instead, leave the top-level config option uncommented, and - follow the conventions above for sub-options. Ensure that your code - correctly handles the top-level option being set to `None` (as it - will be if no sub-options are enabled). -- Lines should be wrapped at 80 characters. -- Use two-space indents. -- `true` and `false` are spelt thus (as opposed to `True`, etc.) -- Use single quotes (`'`) rather than double-quotes (`"`) or backticks - (`` ` ``) to refer to configuration options. +- There should be a horizontal rule between each option, which can be achieved by adding `---` before and + after the option. +- `true` and `false` are spelt thus (as opposed to `True`, etc.) Example: +--- +### `modules` + +Use the `module` sub-option to add a module under `modules` to extend functionality. +The `module` setting then has a sub-option, `config`, which can be used to define some configuration +for the `module`. + +Defaults to none. + +Example configuration: ```yaml -## Frobnication ## - -# The frobnicator will ensure that all requests are fully frobnicated. -# To enable it, uncomment the following. -# -#frobnicator_enabled: true - -# By default, the frobnicator will frobnicate with the default frobber. -# The following will make it use an alternative frobber. -# -#frobincator_frobber: special_frobber - -# Settings for the frobber -# -frobber: - # frobbing speed. Defaults to 1. - # - #speed: 10 - - # frobbing distance. Defaults to 1000. - # - #distance: 100 +modules: + - module: my_super_module.MySuperClass + config: + do_thing: true + - module: my_other_super_module.SomeClass + config: {} ``` +--- Note that the sample configuration is generated from the synapse code and is maintained by a script, `scripts-dev/generate_sample_config.sh`. Making sure that the output from this script matches the desired format is left as an exercise for the reader! + diff --git a/docs/jwt.md b/docs/jwt.md index 8f859d59a6..2e262583a7 100644 --- a/docs/jwt.md +++ b/docs/jwt.md @@ -49,9 +49,8 @@ as follows: * For other installation mechanisms, see the documentation provided by the maintainer. -To enable the JSON web token integration, you should then add a `jwt_config` section -to your configuration file (or uncomment the `enabled: true` line in the -existing section). See [sample_config.yaml](./sample_config.yaml) for some +To enable the JSON web token integration, you should then add a `jwt_config` option +to your configuration file. See the [configuration manual](usage/configuration/config_documentation.md#jwt_config) for some sample settings. ## How to test JWT as a developer diff --git a/docs/manhole.md b/docs/manhole.md index a82fad0f0f..4e5bf833ce 100644 --- a/docs/manhole.md +++ b/docs/manhole.md @@ -13,8 +13,10 @@ environments where untrusted users have shell access. ## Configuring the manhole -To enable it, first uncomment the `manhole` listener configuration in -`homeserver.yaml`. The configuration is slightly different if you're using docker. +To enable it, first add the `manhole` listener configuration in your +`homeserver.yaml`. You can find information on how to do that +in the [configuration manual](usage/configuration/config_documentation.md#manhole_settings). +The configuration is slightly different if you're using docker. #### Docker config diff --git a/docs/message_retention_policies.md b/docs/message_retention_policies.md index b52c4aaa24..8c88f93935 100644 --- a/docs/message_retention_policies.md +++ b/docs/message_retention_policies.md @@ -49,9 +49,9 @@ clients. ## Server configuration -Support for this feature can be enabled and configured in the -`retention` section of the Synapse configuration file (see the -[sample file](https://github.com/matrix-org/synapse/blob/v1.36.0/docs/sample_config.yaml#L451-L518)). +Support for this feature can be enabled and configured by adding a the +`retention` in the Synapse configuration file (see +[configuration manual](usage/configuration/config_documentation.md#retention)). To enable support for message retention policies, set the setting `enabled` in this section to `true`. @@ -65,8 +65,8 @@ message retention policy configured in its state. This allows server admins to ensure that messages are never kept indefinitely in a server's database. -A default policy can be defined as such, in the `retention` section of -the configuration file: +A default policy can be defined as such, by adding the `retention` option in +the configuration file and adding these sub-options: ```yaml default_policy: @@ -86,8 +86,8 @@ Purge jobs are the jobs that Synapse runs in the background to purge expired events from the database. They are only run if support for message retention policies is enabled in the server's configuration. If no configuration for purge jobs is configured by the server admin, -Synapse will use a default configuration, which is described in the -[sample configuration file](https://github.com/matrix-org/synapse/blob/v1.36.0/docs/sample_config.yaml#L451-L518). +Synapse will use a default configuration, which is described here in the +[configuration manual](usage/configuration/config_documentation.md#retention). Some server admins might want a finer control on when events are removed depending on an event's room's policy. This can be done by setting the @@ -137,8 +137,8 @@ the server's database. ### Lifetime limits Server admins can set limits on the values of `max_lifetime` to use when -purging old events in a room. These limits can be defined as such in the -`retention` section of the configuration file: +purging old events in a room. These limits can be defined under the +`retention` option in the configuration file: ```yaml allowed_lifetime_min: 1d diff --git a/docs/openid.md b/docs/openid.md index 9d615a5737..d0ccf36f71 100644 --- a/docs/openid.md +++ b/docs/openid.md @@ -45,8 +45,8 @@ as follows: maintainer. To enable the OpenID integration, you should then add a section to the `oidc_providers` -setting in your configuration file (or uncomment one of the existing examples). -See [sample_config.yaml](./sample_config.yaml) for some sample settings, as well as +setting in your configuration file. +See the [configuration manual](usage/configuration/config_documentation.md#oidc_providers) for some sample settings, as well as the text below for example configurations for specific providers. ## Sample configs diff --git a/docs/setup/forward_proxy.md b/docs/setup/forward_proxy.md index 494c14893b..3482691f83 100644 --- a/docs/setup/forward_proxy.md +++ b/docs/setup/forward_proxy.md @@ -66,8 +66,8 @@ in Synapse can be deactivated. **NOTE**: This has an impact on security and is for testing purposes only! -To deactivate the certificate validation, the following setting must be made in -[homserver.yaml](../usage/configuration/homeserver_sample_config.md). +To deactivate the certificate validation, the following setting must be added to +your [homserver.yaml](../usage/configuration/homeserver_sample_config.md). ```yaml use_insecure_ssl_client_just_for_testing_do_not_use: true diff --git a/docs/setup/installation.md b/docs/setup/installation.md index 1580529fd1..260e50577b 100644 --- a/docs/setup/installation.md +++ b/docs/setup/installation.md @@ -407,11 +407,11 @@ The recommended way to do so is to set up a reverse proxy on port Alternatively, you can configure Synapse to expose an HTTPS port. To do so, you will need to edit `homeserver.yaml`, as follows: -- First, under the `listeners` section, uncomment the configuration for the - TLS-enabled listener. (Remove the hash sign (`#`) at the start of - each line). The relevant lines are like this: +- First, under the `listeners` option, add the configuration for the + TLS-enabled listener like so: ```yaml +listeners: - port: 8448 type: http tls: true @@ -419,9 +419,11 @@ so, you will need to edit `homeserver.yaml`, as follows: - names: [client, federation] ``` -- You will also need to uncomment the `tls_certificate_path` and - `tls_private_key_path` lines under the `TLS` section. You will need to manage - provisioning of these certificates yourself. +- You will also need to add the options `tls_certificate_path` and + `tls_private_key_path`. to your configuration file. You will need to manage provisioning of + these certificates yourself. +- You can find more information about these options as well as how to configure synapse in the + [configuration manual](../usage/configuration/config_documentation.md). If you are using your own certificate, be sure to use a `.pem` file that includes the full certificate chain including any intermediate certificates diff --git a/docs/usage/configuration/config_documentation.md b/docs/usage/configuration/config_documentation.md index 19eb504496..82edd53e36 100644 --- a/docs/usage/configuration/config_documentation.md +++ b/docs/usage/configuration/config_documentation.md @@ -2999,7 +2999,7 @@ This setting has the following sub-options: * `localdb_enabled`: Set to false to disable authentication against the local password database. This is ignored if `enabled` is false, and is only useful if you have other `password_providers`. Defaults to true. -* `pepper`: Set the value here to a secret random string for extra security. # Uncomment and change to a secret random string for extra security. +* `pepper`: Set the value here to a secret random string for extra security. DO NOT CHANGE THIS AFTER INITIAL SETUP! * `policy`: Define and enforce a password policy, such as minimum lengths for passwords, etc. Each parameter is optional. This is an implementation of MSC2000. Parameters are as follows: diff --git a/docs/usage/configuration/user_authentication/single_sign_on/cas.md b/docs/usage/configuration/user_authentication/single_sign_on/cas.md index 3bac1b29f0..899face876 100644 --- a/docs/usage/configuration/user_authentication/single_sign_on/cas.md +++ b/docs/usage/configuration/user_authentication/single_sign_on/cas.md @@ -4,5 +4,5 @@ Synapse supports authenticating users via the [Central Authentication Service protocol](https://en.wikipedia.org/wiki/Central_Authentication_Service) (CAS) natively. -Please see the `cas_config` and `sso` sections of the [Synapse configuration -file](../../../configuration/homeserver_sample_config.md) for more details. \ No newline at end of file +Please see the [cas_config](../../../configuration/config_documentation.md#cas_config) and [sso](../../../configuration/config_documentation.md#sso) +sections of the configuration manual for more details. \ No newline at end of file diff --git a/synapse/config/emailconfig.py b/synapse/config/emailconfig.py index c82f3ee7a3..6e11fbdb9a 100644 --- a/synapse/config/emailconfig.py +++ b/synapse/config/emailconfig.py @@ -145,7 +145,7 @@ class EmailConfig(Config): raise ConfigError( 'The config option "trust_identity_server_for_password_resets" ' 'has been replaced by "account_threepid_delegate". ' - "Please consult the sample config at docs/sample_config.yaml for " + "Please consult the configuration manual at docs/usage/configuration/config_documentation.md for " "details and update your config file." ) From 046a6513bcad2f7111e12e3b750eb798466731da Mon Sep 17 00:00:00 2001 From: Shay Date: Thu, 30 Jun 2022 09:22:40 -0700 Subject: [PATCH 12/35] Don't process /send requests for users who have hit their ratelimit (#13134) --- changelog.d/13134.misc | 1 + synapse/handlers/message.py | 3 +++ 2 files changed, 4 insertions(+) create mode 100644 changelog.d/13134.misc diff --git a/changelog.d/13134.misc b/changelog.d/13134.misc new file mode 100644 index 0000000000..e3e16056d1 --- /dev/null +++ b/changelog.d/13134.misc @@ -0,0 +1 @@ +Apply ratelimiting earlier in processing of /send request. \ No newline at end of file diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index 189f52fe5a..c6b40a5b7a 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -903,6 +903,9 @@ class EventCreationHandler: await self.clock.sleep(random.randint(1, 10)) raise ShadowBanError() + if ratelimit: + await self.request_ratelimiter.ratelimit(requester, update=False) + # We limit the number of concurrent event sends in a room so that we # don't fork the DAG too much. If we don't limit then we can end up in # a situation where event persistence can't keep up, causing From 50f0e4028b334566a067b671d15246a9b05e8498 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jacek=20Ku=C5=9Bnierz?= Date: Thu, 30 Jun 2022 19:48:04 +0200 Subject: [PATCH 13/35] Allow dependency errors to pass through (#13113) Signed-off-by: Jacek Kusnierz Co-authored-by: Brendan Abolivier --- changelog.d/13113.misc | 1 + synapse/config/cache.py | 9 ++------- synapse/config/jwt.py | 17 +++-------------- synapse/config/metrics.py | 9 ++------- synapse/config/oidc.py | 10 ++-------- synapse/config/repository.py | 10 ++-------- synapse/config/saml2.py | 9 ++------- synapse/config/tracer.py | 9 ++------- 8 files changed, 16 insertions(+), 58 deletions(-) create mode 100644 changelog.d/13113.misc diff --git a/changelog.d/13113.misc b/changelog.d/13113.misc new file mode 100644 index 0000000000..7b1a50eec0 --- /dev/null +++ b/changelog.d/13113.misc @@ -0,0 +1 @@ +Raise a `DependencyError` on missing dependencies instead of a `ConfigError`. \ No newline at end of file diff --git a/synapse/config/cache.py b/synapse/config/cache.py index 63310c8d07..2db8cfb005 100644 --- a/synapse/config/cache.py +++ b/synapse/config/cache.py @@ -21,7 +21,7 @@ from typing import Any, Callable, Dict, Optional import attr from synapse.types import JsonDict -from synapse.util.check_dependencies import DependencyException, check_requirements +from synapse.util.check_dependencies import check_requirements from ._base import Config, ConfigError @@ -159,12 +159,7 @@ class CacheConfig(Config): self.track_memory_usage = cache_config.get("track_memory_usage", False) if self.track_memory_usage: - try: - check_requirements("cache_memory") - except DependencyException as e: - raise ConfigError( - e.message # noqa: B306, DependencyException.message is a property - ) + check_requirements("cache_memory") expire_caches = cache_config.get("expire_caches", True) cache_entry_ttl = cache_config.get("cache_entry_ttl", "30m") diff --git a/synapse/config/jwt.py b/synapse/config/jwt.py index 49aaca7cf6..a973bb5080 100644 --- a/synapse/config/jwt.py +++ b/synapse/config/jwt.py @@ -15,14 +15,9 @@ from typing import Any from synapse.types import JsonDict +from synapse.util.check_dependencies import check_requirements -from ._base import Config, ConfigError - -MISSING_AUTHLIB = """Missing authlib library. This is required for jwt login. - - Install by running: - pip install synapse[jwt] - """ +from ._base import Config class JWTConfig(Config): @@ -41,13 +36,7 @@ class JWTConfig(Config): # that the claims exist on the JWT. self.jwt_issuer = jwt_config.get("issuer") self.jwt_audiences = jwt_config.get("audiences") - - try: - from authlib.jose import JsonWebToken - - JsonWebToken # To stop unused lint. - except ImportError: - raise ConfigError(MISSING_AUTHLIB) + check_requirements("jwt") else: self.jwt_enabled = False self.jwt_secret = None diff --git a/synapse/config/metrics.py b/synapse/config/metrics.py index d636507886..3b42be5b5b 100644 --- a/synapse/config/metrics.py +++ b/synapse/config/metrics.py @@ -18,7 +18,7 @@ from typing import Any, Optional import attr from synapse.types import JsonDict -from synapse.util.check_dependencies import DependencyException, check_requirements +from synapse.util.check_dependencies import check_requirements from ._base import Config, ConfigError @@ -57,12 +57,7 @@ class MetricsConfig(Config): self.sentry_enabled = "sentry" in config if self.sentry_enabled: - try: - check_requirements("sentry") - except DependencyException as e: - raise ConfigError( - e.message # noqa: B306, DependencyException.message is a property - ) + check_requirements("sentry") self.sentry_dsn = config["sentry"].get("dsn") if not self.sentry_dsn: diff --git a/synapse/config/oidc.py b/synapse/config/oidc.py index 98e8cd8b5a..5418a332da 100644 --- a/synapse/config/oidc.py +++ b/synapse/config/oidc.py @@ -24,7 +24,7 @@ from synapse.types import JsonDict from synapse.util.module_loader import load_module from synapse.util.stringutils import parse_and_validate_mxc_uri -from ..util.check_dependencies import DependencyException, check_requirements +from ..util.check_dependencies import check_requirements from ._base import Config, ConfigError, read_file DEFAULT_USER_MAPPING_PROVIDER = "synapse.handlers.oidc.JinjaOidcMappingProvider" @@ -41,12 +41,7 @@ class OIDCConfig(Config): if not self.oidc_providers: return - try: - check_requirements("oidc") - except DependencyException as e: - raise ConfigError( - e.message # noqa: B306, DependencyException.message is a property - ) from e + check_requirements("oidc") # check we don't have any duplicate idp_ids now. (The SSO handler will also # check for duplicates when the REST listeners get registered, but that happens @@ -146,7 +141,6 @@ OIDC_PROVIDER_CONFIG_WITH_ID_SCHEMA = { "allOf": [OIDC_PROVIDER_CONFIG_SCHEMA, {"required": ["idp_id", "idp_name"]}] } - # the `oidc_providers` list can either be None (as it is in the default config), or # a list of provider configs, each of which requires an explicit ID and name. OIDC_PROVIDER_LIST_SCHEMA = { diff --git a/synapse/config/repository.py b/synapse/config/repository.py index aadec1e54e..3c69dd325f 100644 --- a/synapse/config/repository.py +++ b/synapse/config/repository.py @@ -21,7 +21,7 @@ import attr from synapse.config.server import generate_ip_set from synapse.types import JsonDict -from synapse.util.check_dependencies import DependencyException, check_requirements +from synapse.util.check_dependencies import check_requirements from synapse.util.module_loader import load_module from ._base import Config, ConfigError @@ -184,13 +184,7 @@ class ContentRepositoryConfig(Config): ) self.url_preview_enabled = config.get("url_preview_enabled", False) if self.url_preview_enabled: - try: - check_requirements("url_preview") - - except DependencyException as e: - raise ConfigError( - e.message # noqa: B306, DependencyException.message is a property - ) + check_requirements("url_preview") proxy_env = getproxies_environment() if "url_preview_ip_range_blacklist" not in config: diff --git a/synapse/config/saml2.py b/synapse/config/saml2.py index bd7c234d31..49ca663dde 100644 --- a/synapse/config/saml2.py +++ b/synapse/config/saml2.py @@ -18,7 +18,7 @@ from typing import Any, List, Set from synapse.config.sso import SsoAttributeRequirement from synapse.types import JsonDict -from synapse.util.check_dependencies import DependencyException, check_requirements +from synapse.util.check_dependencies import check_requirements from synapse.util.module_loader import load_module, load_python_module from ._base import Config, ConfigError @@ -76,12 +76,7 @@ class SAML2Config(Config): if not saml2_config.get("sp_config") and not saml2_config.get("config_path"): return - try: - check_requirements("saml2") - except DependencyException as e: - raise ConfigError( - e.message # noqa: B306, DependencyException.message is a property - ) + check_requirements("saml2") self.saml2_enabled = True diff --git a/synapse/config/tracer.py b/synapse/config/tracer.py index 6fbf927f11..c19270c6c5 100644 --- a/synapse/config/tracer.py +++ b/synapse/config/tracer.py @@ -15,7 +15,7 @@ from typing import Any, List, Set from synapse.types import JsonDict -from synapse.util.check_dependencies import DependencyException, check_requirements +from synapse.util.check_dependencies import check_requirements from ._base import Config, ConfigError @@ -40,12 +40,7 @@ class TracerConfig(Config): if not self.opentracer_enabled: return - try: - check_requirements("opentracing") - except DependencyException as e: - raise ConfigError( - e.message # noqa: B306, DependencyException.message is a property - ) + check_requirements("opentracing") # The tracer is enabled so sanitize the config From c0efc689cb925ff42e5617e7cddba11f18ab22de Mon Sep 17 00:00:00 2001 From: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com> Date: Thu, 30 Jun 2022 22:12:28 +0100 Subject: [PATCH 14/35] Add documentation for phone home stats (#13086) --- changelog.d/13086.doc | 1 + docs/SUMMARY.md | 1 + .../reporting_anonymised_statistics.md | 81 +++++++++++++++++++ 3 files changed, 83 insertions(+) create mode 100644 changelog.d/13086.doc create mode 100644 docs/usage/administration/monitoring/reporting_anonymised_statistics.md diff --git a/changelog.d/13086.doc b/changelog.d/13086.doc new file mode 100644 index 0000000000..a3960ca325 --- /dev/null +++ b/changelog.d/13086.doc @@ -0,0 +1 @@ +Add documentation for anonymised homeserver statistics collection. \ No newline at end of file diff --git a/docs/SUMMARY.md b/docs/SUMMARY.md index b51c7a3cb4..3978f96fc3 100644 --- a/docs/SUMMARY.md +++ b/docs/SUMMARY.md @@ -69,6 +69,7 @@ - [Federation](usage/administration/admin_api/federation.md) - [Manhole](manhole.md) - [Monitoring](metrics-howto.md) + - [Reporting Anonymised Statistics](usage/administration/monitoring/reporting_anonymised_statistics.md) - [Understanding Synapse Through Grafana Graphs](usage/administration/understanding_synapse_through_grafana_graphs.md) - [Useful SQL for Admins](usage/administration/useful_sql_for_admins.md) - [Database Maintenance Tools](usage/administration/database_maintenance_tools.md) diff --git a/docs/usage/administration/monitoring/reporting_anonymised_statistics.md b/docs/usage/administration/monitoring/reporting_anonymised_statistics.md new file mode 100644 index 0000000000..4f1e0fecf5 --- /dev/null +++ b/docs/usage/administration/monitoring/reporting_anonymised_statistics.md @@ -0,0 +1,81 @@ +# Reporting Anonymised Statistics + +When generating your Synapse configuration file, you are asked whether you +would like to report anonymised statistics to Matrix.org. These statistics +provide the foundation a glimpse into the number of Synapse homeservers +participating in the network, as well as statistics such as the number of +rooms being created and messages being sent. This feature is sometimes +affectionately called "phone-home" stats. Reporting +[is optional](../../configuration/config_documentation.md#report_stats) +and the reporting endpoint +[can be configured](../../configuration/config_documentation.md#report_stats_endpoint), +in case you would like to instead report statistics from a set of homeservers +to your own infrastructure. + +This documentation aims to define the statistics available and the +homeserver configuration options that exist to tweak it. + +## Available Statistics + +The following statistics are sent to the configured reporting endpoint: + +| Statistic Name | Type | Description | +|----------------------------|--------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| `memory_rss` | int | The memory usage of the process (in kilobytes on Unix-based systems, bytes on MacOS). | +| `cpu_average` | int | CPU time in % of a single core (not % of all cores). | +| `homeserver` | string | The homeserver's server name. | +| `server_context` | string | An arbitrary string used to group statistics from a set of homeservers. | +| `timestamp` | int | The current time, represented as the number of seconds since the epoch. | +| `uptime_seconds` | int | The number of seconds since the homeserver was last started. | +| `python_version` | string | The Python version number in use (e.g "3.7.1"). Taken from `sys.version_info`. | +| `total_users` | int | The number of registered users on the homeserver. | +| `total_nonbridged_users` | int | The number of users, excluding those created by an Application Service. | +| `daily_user_type_native` | int | The number of native users created in the last 24 hours. | +| `daily_user_type_guest` | int | The number of guest users created in the last 24 hours. | +| `daily_user_type_bridged` | int | The number of users created by Application Services in the last 24 hours. | +| `total_room_count` | int | The total number of rooms present on the homeserver. | +| `daily_active_users` | int | The number of unique users[^1] that have used the homeserver in the last 24 hours. | +| `monthly_active_users` | int | The number of unique users[^1] that have used the homeserver in the last 30 days. | +| `daily_active_rooms` | int | The number of rooms that have had a (state) event with the type `m.room.message` sent in them in the last 24 hours. | +| `daily_active_e2ee_rooms` | int | The number of rooms that have had a (state) event with the type `m.room.encrypted` sent in them in the last 24 hours. | +| `daily_messages` | int | The number of (state) events with the type `m.room.message` seen in the last 24 hours. | +| `daily_e2ee_messages` | int | The number of (state) events with the type `m.room.encrypted` seen in the last 24 hours. | +| `daily_sent_messages` | int | The number of (state) events sent by a local user with the type `m.room.message` seen in the last 24 hours. | +| `daily_sent_e2ee_messages` | int | The number of (state) events sent by a local user with the type `m.room.encrypted` seen in the last 24 hours. | +| `r30_users_all` | int | The number of 30 day retained users, defined as users who have created their accounts more than 30 days ago, where they were last seen at most 30 days ago and where those two timestamps are over 30 days apart. Includes clients that do not fit into the below r30 client types. | +| `r30_users_android` | int | The number of 30 day retained users, as defined above. Filtered only to clients with "Android" in the user agent string. | +| `r30_users_ios` | int | The number of 30 day retained users, as defined above. Filtered only to clients with "iOS" in the user agent string. | +| `r30_users_electron` | int | The number of 30 day retained users, as defined above. Filtered only to clients with "Electron" in the user agent string. | +| `r30_users_web` | int | The number of 30 day retained users, as defined above. Filtered only to clients with "Mozilla" or "Gecko" in the user agent string. | +| `r30v2_users_all` | int | The number of 30 day retained users, with a revised algorithm. Defined as users that appear more than once in the past 60 days, and have more than 30 days between the most and least recent appearances in the past 60 days. Includes clients that do not fit into the below r30 client types. | +| `r30v2_users_android` | int | The number of 30 day retained users, as defined above. Filtered only to clients with ("riot" or "element") and "android" (case-insensitive) in the user agent string. | +| `r30v2_users_ios` | int | The number of 30 day retained users, as defined above. Filtered only to clients with ("riot" or "element") and "ios" (case-insensitive) in the user agent string. | +| `r30v2_users_electron` | int | The number of 30 day retained users, as defined above. Filtered only to clients with ("riot" or "element") and "electron" (case-insensitive) in the user agent string. | +| `r30v2_users_web` | int | The number of 30 day retained users, as defined above. Filtered only to clients with "mozilla" or "gecko" (case-insensitive) in the user agent string. | +| `cache_factor` | int | The configured [`global factor`](../../configuration/config_documentation.md#caching) value for caching. | +| `event_cache_size` | int | The configured [`event_cache_size`](../../configuration/config_documentation.md#caching) value for caching. | +| `database_engine` | string | The database engine that is in use. Either "psycopg2" meaning PostgreSQL is in use, or "sqlite3" for SQLite3. | +| `database_server_version` | string | The version of the database server. Examples being "10.10" for PostgreSQL server version 10.0, and "3.38.5" for SQLite 3.38.5 installed on the system. | +| `log_level` | string | The log level in use. Examples are "INFO", "WARNING", "ERROR", "DEBUG", etc. | + + +[^1]: Native matrix users and guests are always counted. If the +[`track_puppeted_user_ips`](../../configuration/config_documentation.md#track_puppeted_user_ips) +option is set to `true`, "puppeted" users (users that an Application Service have performed +[an action on behalf of](https://spec.matrix.org/v1.3/application-service-api/#identity-assertion)) +will also be counted. Note that an Application Service can "puppet" any user in their +[user namespace](https://spec.matrix.org/v1.3/application-service-api/#registration), +not only users that the Application Service has created. If this happens, the Application Service +will additionally be counted as a user (irrespective of `track_puppeted_user_ips`). + +## Using a Custom Statistics Collection Server + +If statistics reporting is enabled, the endpoint that Synapse sends metrics to is configured by the +[`report_stats_endpoint`](../../configuration/config_documentation.md#report_stats_endpoint) config +option. By default, statistics are sent to Matrix.org. + +If you would like to set up your own statistics collection server and send metrics there, you may +consider using one of the following known implementations: + +* [Matrix.org's Panopticon](https://github.com/matrix-org/panopticon) +* [Famedly's Barad-dûr](https://gitlab.com/famedly/company/devops/services/barad-dur) From 8c2825276fec6e03434f1924482788ea3281a9fc Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Fri, 1 Jul 2022 10:19:27 +0100 Subject: [PATCH 15/35] Skip waiting for full state for incoming events (#13144) When we receive an event over federation during a faster join, there is no need to wait for full state, since we have a whole reconciliation process designed to take the partial state into account. --- changelog.d/13144.misc | 1 + synapse/state/__init__.py | 12 +++++++++--- tests/test_state.py | 4 +++- 3 files changed, 13 insertions(+), 4 deletions(-) create mode 100644 changelog.d/13144.misc diff --git a/changelog.d/13144.misc b/changelog.d/13144.misc new file mode 100644 index 0000000000..34762e2fcd --- /dev/null +++ b/changelog.d/13144.misc @@ -0,0 +1 @@ +Faster joins: skip waiting for full state when processing incoming events over federation. diff --git a/synapse/state/__init__.py b/synapse/state/__init__.py index 9d3fe66100..d5cbdb3eef 100644 --- a/synapse/state/__init__.py +++ b/synapse/state/__init__.py @@ -249,8 +249,12 @@ class StateHandler: partial_state = True logger.debug("calling resolve_state_groups from compute_event_context") + # we've already taken into account partial state, so no need to wait for + # complete state here. entry = await self.resolve_state_groups_for_events( - event.room_id, event.prev_event_ids() + event.room_id, + event.prev_event_ids(), + await_full_state=False, ) state_ids_before_event = entry.state @@ -335,7 +339,7 @@ class StateHandler: @measure_func() async def resolve_state_groups_for_events( - self, room_id: str, event_ids: Collection[str] + self, room_id: str, event_ids: Collection[str], await_full_state: bool = True ) -> _StateCacheEntry: """Given a list of event_ids this method fetches the state at each event, resolves conflicts between them and returns them. @@ -343,6 +347,8 @@ class StateHandler: Args: room_id event_ids + await_full_state: if true, will block if we do not yet have complete + state at these events. Returns: The resolved state @@ -350,7 +356,7 @@ class StateHandler: logger.debug("resolve_state_groups event_ids %s", event_ids) state_groups = await self._state_storage_controller.get_state_group_for_events( - event_ids + event_ids, await_full_state=await_full_state ) state_group_ids = state_groups.values() diff --git a/tests/test_state.py b/tests/test_state.py index b005dd8d0f..7b3f52f68e 100644 --- a/tests/test_state.py +++ b/tests/test_state.py @@ -131,7 +131,9 @@ class _DummyStore: async def get_room_version_id(self, room_id): return RoomVersions.V1.identifier - async def get_state_group_for_events(self, event_ids): + async def get_state_group_for_events( + self, event_ids, await_full_state: bool = True + ): res = {} for event in event_ids: res[event] = self._event_to_state_group[event] From 6da861ae6937e85689825c06c9198673f5209a2b Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Fri, 1 Jul 2022 10:52:10 +0100 Subject: [PATCH 16/35] `_process_received_pdu`: Improve exception handling (#13145) `_check_event_auth` is expected to raise `AuthError`s, so no need to log it again. --- changelog.d/13145.misc | 1 + synapse/handlers/federation_event.py | 13 ++++++------- 2 files changed, 7 insertions(+), 7 deletions(-) create mode 100644 changelog.d/13145.misc diff --git a/changelog.d/13145.misc b/changelog.d/13145.misc new file mode 100644 index 0000000000..d5e2dba866 --- /dev/null +++ b/changelog.d/13145.misc @@ -0,0 +1 @@ +Improve exception handling when processing events received over federation. diff --git a/synapse/handlers/federation_event.py b/synapse/handlers/federation_event.py index b7c54e642f..479d936dc0 100644 --- a/synapse/handlers/federation_event.py +++ b/synapse/handlers/federation_event.py @@ -1092,20 +1092,19 @@ class FederationEventHandler: logger.debug("Processing event: %s", event) assert not event.internal_metadata.outlier + context = await self._state_handler.compute_event_context( + event, + state_ids_before_event=state_ids, + ) try: - context = await self._state_handler.compute_event_context( - event, - state_ids_before_event=state_ids, - ) context = await self._check_event_auth( origin, event, context, ) except AuthError as e: - # FIXME richvdh 2021/10/07 I don't think this is reachable. Let's log it - # for now - logger.exception("Unexpected AuthError from _check_event_auth") + # This happens only if we couldn't find the auth events. We'll already have + # logged a warning, so now we just convert to a FederationError. raise FederationError("ERROR", e.code, e.msg, affected=event.event_id) if not backfilled and not context.rejected: From d70ff5cc3508f4010ca2d19b090f0338e99c1d28 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Fri, 1 Jul 2022 12:04:56 +0200 Subject: [PATCH 17/35] Extra validation for rest/client/account_data (#13148) * Extra validation for rest/client/account_data This is a fairly simple endpoint and we did pretty well here. * Changelog --- changelog.d/13148.feature | 1 + synapse/rest/client/account_data.py | 19 +++++++++++++++++-- 2 files changed, 18 insertions(+), 2 deletions(-) create mode 100644 changelog.d/13148.feature diff --git a/changelog.d/13148.feature b/changelog.d/13148.feature new file mode 100644 index 0000000000..d1104b04b0 --- /dev/null +++ b/changelog.d/13148.feature @@ -0,0 +1 @@ +Improve validation logic in Synapse's REST endpoints. diff --git a/synapse/rest/client/account_data.py b/synapse/rest/client/account_data.py index bfe985939b..f13970b898 100644 --- a/synapse/rest/client/account_data.py +++ b/synapse/rest/client/account_data.py @@ -15,11 +15,11 @@ import logging from typing import TYPE_CHECKING, Tuple -from synapse.api.errors import AuthError, NotFoundError, SynapseError +from synapse.api.errors import AuthError, Codes, NotFoundError, SynapseError from synapse.http.server import HttpServer from synapse.http.servlet import RestServlet, parse_json_object_from_request from synapse.http.site import SynapseRequest -from synapse.types import JsonDict +from synapse.types import JsonDict, RoomID from ._base import client_patterns @@ -104,6 +104,13 @@ class RoomAccountDataServlet(RestServlet): if user_id != requester.user.to_string(): raise AuthError(403, "Cannot add account data for other users.") + if not RoomID.is_valid(room_id): + raise SynapseError( + 400, + f"{room_id} is not a valid room ID", + Codes.INVALID_PARAM, + ) + body = parse_json_object_from_request(request) if account_data_type == "m.fully_read": @@ -111,6 +118,7 @@ class RoomAccountDataServlet(RestServlet): 405, "Cannot set m.fully_read through this API." " Use /rooms/!roomId:server.name/read_markers", + Codes.BAD_JSON, ) await self.handler.add_account_data_to_room( @@ -130,6 +138,13 @@ class RoomAccountDataServlet(RestServlet): if user_id != requester.user.to_string(): raise AuthError(403, "Cannot get account data for other users.") + if not RoomID.is_valid(room_id): + raise SynapseError( + 400, + f"{room_id} is not a valid room ID", + Codes.INVALID_PARAM, + ) + event = await self.store.get_account_data_for_room_and_type( user_id, room_id, account_data_type ) From fe910fb10ef854c8c884c6e9a8e7034da5124464 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Fri, 1 Jul 2022 13:33:59 +0100 Subject: [PATCH 18/35] complement.sh: Permit skipping docker build (#13143) Add a `-f` argument to `complement.sh` to skip the docker build --- changelog.d/13143.misc | 1 + scripts-dev/complement.sh | 68 ++++++++++++++++++++++++++++++--------- 2 files changed, 53 insertions(+), 16 deletions(-) create mode 100644 changelog.d/13143.misc diff --git a/changelog.d/13143.misc b/changelog.d/13143.misc new file mode 100644 index 0000000000..1cb77c02d7 --- /dev/null +++ b/changelog.d/13143.misc @@ -0,0 +1 @@ +Add support to `complement.sh` for skipping the docker build. diff --git a/scripts-dev/complement.sh b/scripts-dev/complement.sh index f1843717cb..20df5fbc24 100755 --- a/scripts-dev/complement.sh +++ b/scripts-dev/complement.sh @@ -14,9 +14,12 @@ # By default Synapse is run in monolith mode. This can be overridden by # setting the WORKERS environment variable. # -# A regular expression of test method names can be supplied as the first -# argument to the script. Complement will then only run those tests. If -# no regex is supplied, all tests are run. For example; +# You can optionally give a "-f" argument (for "fast") before any to skip +# rebuilding the docker images, if you just want to rerun the tests. +# +# Remaining commandline arguments are passed through to `go test`. For example, +# you can supply a regular expression of test method names via the "-run" +# argument: # # ./complement.sh -run "TestOutboundFederation(Profile|Send)" # @@ -32,6 +35,37 @@ echo_if_github() { fi } +# Helper to print out the usage instructions +usage() { + cat >&2 <... +Run the complement test suite on Synapse. + + -f Skip rebuilding the docker images, and just use the most recent + 'complement-synapse:latest' image + +For help on arguments to 'go test', run 'go help testflag'. +EOF +} + +# parse our arguments +skip_docker_build="" +while [ $# -ge 1 ]; do + arg=$1 + case "$arg" in + "-h") + usage + exit 1 + ;; + "-f") + skip_docker_build=1 + ;; + *) + # unknown arg: presumably an argument to gotest. break the loop. + break + esac + shift +done # enable buildkit for the docker builds export DOCKER_BUILDKIT=1 @@ -49,21 +83,23 @@ if [[ -z "$COMPLEMENT_DIR" ]]; then echo "Checkout available at 'complement-${COMPLEMENT_REF}'" fi -# Build the base Synapse image from the local checkout -echo_if_github "::group::Build Docker image: matrixdotorg/synapse" -docker build -t matrixdotorg/synapse -f "docker/Dockerfile" . -echo_if_github "::endgroup::" +if [ -z "$skip_docker_build" ]; then + # Build the base Synapse image from the local checkout + echo_if_github "::group::Build Docker image: matrixdotorg/synapse" + docker build -t matrixdotorg/synapse -f "docker/Dockerfile" . + echo_if_github "::endgroup::" -# Build the workers docker image (from the base Synapse image we just built). -echo_if_github "::group::Build Docker image: matrixdotorg/synapse-workers" -docker build -t matrixdotorg/synapse-workers -f "docker/Dockerfile-workers" . -echo_if_github "::endgroup::" + # Build the workers docker image (from the base Synapse image we just built). + echo_if_github "::group::Build Docker image: matrixdotorg/synapse-workers" + docker build -t matrixdotorg/synapse-workers -f "docker/Dockerfile-workers" . + echo_if_github "::endgroup::" -# Build the unified Complement image (from the worker Synapse image we just built). -echo_if_github "::group::Build Docker image: complement/Dockerfile" -docker build -t complement-synapse \ - -f "docker/complement/Dockerfile" "docker/complement" -echo_if_github "::endgroup::" + # Build the unified Complement image (from the worker Synapse image we just built). + echo_if_github "::group::Build Docker image: complement/Dockerfile" + docker build -t complement-synapse \ + -f "docker/complement/Dockerfile" "docker/complement" + echo_if_github "::endgroup::" +fi export COMPLEMENT_BASE_IMAGE=complement-synapse From c04e25789ee7fa5bd57864ad7687595f44996798 Mon Sep 17 00:00:00 2001 From: reivilibre Date: Fri, 1 Jul 2022 16:42:49 +0100 Subject: [PATCH 19/35] Enable Complement testing in the 'Twisted Trunk' CI runs. (#13079) --- .github/workflows/twisted_trunk.yml | 67 +++++++++++++++++++++++++++++ changelog.d/13079.misc | 1 + docker/Dockerfile | 9 +++- scripts-dev/complement.sh | 7 ++- 4 files changed, 82 insertions(+), 2 deletions(-) create mode 100644 changelog.d/13079.misc diff --git a/.github/workflows/twisted_trunk.yml b/.github/workflows/twisted_trunk.yml index 5f0671f350..12267405be 100644 --- a/.github/workflows/twisted_trunk.yml +++ b/.github/workflows/twisted_trunk.yml @@ -96,6 +96,72 @@ jobs: /logs/results.tap /logs/**/*.log* + complement: + if: "${{ !failure() && !cancelled() }}" + runs-on: ubuntu-latest + + strategy: + fail-fast: false + matrix: + include: + - arrangement: monolith + database: SQLite + + - arrangement: monolith + database: Postgres + + - arrangement: workers + database: Postgres + + steps: + # The path is set via a file given by $GITHUB_PATH. We need both Go 1.17 and GOPATH on the path to run Complement. + # See https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions#adding-a-system-path + - name: "Set Go Version" + run: | + # Add Go 1.17 to the PATH: see https://github.com/actions/virtual-environments/blob/main/images/linux/Ubuntu2004-Readme.md#environment-variables-2 + echo "$GOROOT_1_17_X64/bin" >> $GITHUB_PATH + # Add the Go path to the PATH: We need this so we can call gotestfmt + echo "~/go/bin" >> $GITHUB_PATH + + - name: "Install Complement Dependencies" + run: | + sudo apt-get update && sudo apt-get install -y libolm3 libolm-dev + go get -v github.com/haveyoudebuggedit/gotestfmt/v2/cmd/gotestfmt@latest + + - name: Run actions/checkout@v2 for synapse + uses: actions/checkout@v2 + with: + path: synapse + + # This step is specific to the 'Twisted trunk' test run: + - name: Patch dependencies + run: | + set -x + DEBIAN_FRONTEND=noninteractive sudo apt-get install -yqq python3 pipx + pipx install poetry==1.1.12 + + poetry remove -n twisted + poetry add -n --extras tls git+https://github.com/twisted/twisted.git#trunk + poetry lock --no-update + # NOT IN 1.1.12 poetry lock --check + working-directory: synapse + + - name: "Install custom gotestfmt template" + run: | + mkdir .gotestfmt/github -p + cp synapse/.ci/complement_package.gotpl .gotestfmt/github/package.gotpl + + # Attempt to check out the same branch of Complement as the PR. If it + # doesn't exist, fallback to HEAD. + - name: Checkout complement + run: synapse/.ci/scripts/checkout_complement.sh + + - run: | + set -o pipefail + TEST_ONLY_SKIP_DEP_HASH_VERIFICATION=1 POSTGRES=${{ (matrix.database == 'Postgres') && 1 || '' }} WORKERS=${{ (matrix.arrangement == 'workers') && 1 || '' }} COMPLEMENT_DIR=`pwd`/complement synapse/scripts-dev/complement.sh -json 2>&1 | gotestfmt + shell: bash + name: Run Complement Tests + # open an issue if the build fails, so we know about it. open-issue: if: failure() @@ -103,6 +169,7 @@ jobs: - mypy - trial - sytest + - complement runs-on: ubuntu-latest diff --git a/changelog.d/13079.misc b/changelog.d/13079.misc new file mode 100644 index 0000000000..0133097c83 --- /dev/null +++ b/changelog.d/13079.misc @@ -0,0 +1 @@ +Enable Complement testing in the 'Twisted Trunk' CI runs. \ No newline at end of file diff --git a/docker/Dockerfile b/docker/Dockerfile index c676f83775..22707ed142 100644 --- a/docker/Dockerfile +++ b/docker/Dockerfile @@ -62,7 +62,13 @@ WORKDIR /synapse # Copy just what we need to run `poetry export`... COPY pyproject.toml poetry.lock /synapse/ -RUN /root/.local/bin/poetry export --extras all -o /synapse/requirements.txt + +# If specified, we won't verify the hashes of dependencies. +# This is only needed if the hashes of dependencies cannot be checked for some +# reason, such as when a git repository is used directly as a dependency. +ARG TEST_ONLY_SKIP_DEP_HASH_VERIFICATION + +RUN /root/.local/bin/poetry export --extras all -o /synapse/requirements.txt ${TEST_ONLY_SKIP_DEP_HASH_VERIFICATION:+--without-hashes} ### ### Stage 1: builder @@ -85,6 +91,7 @@ RUN \ openssl \ rustc \ zlib1g-dev \ + git \ && rm -rf /var/lib/apt/lists/* # To speed up rebuilds, install all of the dependencies before we copy over diff --git a/scripts-dev/complement.sh b/scripts-dev/complement.sh index 20df5fbc24..8448d49e26 100755 --- a/scripts-dev/complement.sh +++ b/scripts-dev/complement.sh @@ -23,6 +23,9 @@ # # ./complement.sh -run "TestOutboundFederation(Profile|Send)" # +# Specifying TEST_ONLY_SKIP_DEP_HASH_VERIFICATION=1 will cause `poetry export` +# to not emit any hashes when building the Docker image. This then means that +# you can use 'unverifiable' sources such as git repositories as dependencies. # Exit if a line returns a non-zero exit code set -e @@ -86,7 +89,9 @@ fi if [ -z "$skip_docker_build" ]; then # Build the base Synapse image from the local checkout echo_if_github "::group::Build Docker image: matrixdotorg/synapse" - docker build -t matrixdotorg/synapse -f "docker/Dockerfile" . + docker build -t matrixdotorg/synapse \ + --build-arg TEST_ONLY_SKIP_DEP_HASH_VERIFICATION \ + -f "docker/Dockerfile" . echo_if_github "::endgroup::" # Build the workers docker image (from the base Synapse image we just built). From fa10468eb4eebb5e648aa2d4ca5c87c0cd1aed88 Mon Sep 17 00:00:00 2001 From: Till <2353100+S7evinK@users.noreply.github.com> Date: Mon, 4 Jul 2022 14:34:50 +0200 Subject: [PATCH 20/35] [Complement] Allow device_name lookup over federation (#13167) --- changelog.d/13167.misc | 1 + docker/complement/conf/workers-shared-extra.yaml.j2 | 2 ++ 2 files changed, 3 insertions(+) create mode 100644 changelog.d/13167.misc diff --git a/changelog.d/13167.misc b/changelog.d/13167.misc new file mode 100644 index 0000000000..a7c7a688de --- /dev/null +++ b/changelog.d/13167.misc @@ -0,0 +1 @@ +Update config used by Complement to allow device name lookup over federation. \ No newline at end of file diff --git a/docker/complement/conf/workers-shared-extra.yaml.j2 b/docker/complement/conf/workers-shared-extra.yaml.j2 index 7c6a0fd756..b5f675bc73 100644 --- a/docker/complement/conf/workers-shared-extra.yaml.j2 +++ b/docker/complement/conf/workers-shared-extra.yaml.j2 @@ -81,6 +81,8 @@ rc_invites: federation_rr_transactions_per_room_per_second: 9999 +allow_device_name_lookup_over_federation: true + ## Experimental Features ## experimental_features: From 9820665597ab6a3bbb1d23d0824752967b2170dd Mon Sep 17 00:00:00 2001 From: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com> Date: Mon, 4 Jul 2022 15:15:33 +0100 Subject: [PATCH 21/35] Remove tests/utils.py from mypy's exclude list (#13159) --- changelog.d/13159.misc | 1 + mypy.ini | 1 - tests/server.py | 1 - tests/utils.py | 4 ++-- 4 files changed, 3 insertions(+), 4 deletions(-) create mode 100644 changelog.d/13159.misc diff --git a/changelog.d/13159.misc b/changelog.d/13159.misc new file mode 100644 index 0000000000..bb5554ebe0 --- /dev/null +++ b/changelog.d/13159.misc @@ -0,0 +1 @@ +Improve and fix type hints. \ No newline at end of file diff --git a/mypy.ini b/mypy.ini index b9b16860db..d757a88fd1 100644 --- a/mypy.ini +++ b/mypy.ini @@ -73,7 +73,6 @@ exclude = (?x) |tests/util/test_lrucache.py |tests/util/test_rwlock.py |tests/util/test_wheel_timer.py - |tests/utils.py )$ [mypy-synapse.federation.transport.client] diff --git a/tests/server.py b/tests/server.py index b9f465971f..ce017ca0f6 100644 --- a/tests/server.py +++ b/tests/server.py @@ -830,7 +830,6 @@ def setup_test_homeserver( # Mock TLS hs.tls_server_context_factory = Mock() - hs.tls_client_options_factory = Mock() hs.setup() if homeserver_to_use == TestHomeServer: diff --git a/tests/utils.py b/tests/utils.py index cabb2c0dec..aca6a0083b 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -64,7 +64,7 @@ def setupdb(): password=POSTGRES_PASSWORD, dbname=POSTGRES_DBNAME_FOR_INITIAL_CREATE, ) - db_conn.autocommit = True + db_engine.attempt_to_set_autocommit(db_conn, autocommit=True) cur = db_conn.cursor() cur.execute("DROP DATABASE IF EXISTS %s;" % (POSTGRES_BASE_DB,)) cur.execute( @@ -94,7 +94,7 @@ def setupdb(): password=POSTGRES_PASSWORD, dbname=POSTGRES_DBNAME_FOR_INITIAL_CREATE, ) - db_conn.autocommit = True + db_engine.attempt_to_set_autocommit(db_conn, autocommit=True) cur = db_conn.cursor() cur.execute("DROP DATABASE IF EXISTS %s;" % (POSTGRES_BASE_DB,)) cur.close() From 95a260da7342ef654a6da4985e0539225969ddde Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Mon, 4 Jul 2022 16:29:04 +0100 Subject: [PATCH 22/35] Update changelog for v1.62.0rc2 --- CHANGES.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 1fb1ff9abc..babfe1628f 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -4,8 +4,8 @@ Synapse 1.62.0rc3 (2022-07-04) Bugfixes -------- -- Update the version of the [ldap3 plugin](https://github.com/matrix-org/matrix-synapse-ldap3/) includled in matrix.org docker images and debian packages to 0.2.1. This fixes [problems involving usernames that have uppercase characters](https://github.com/matrix-org/matrix-synapse-ldap3/pull/163). ([\#13156](https://github.com/matrix-org/synapse/issues/13156)) -- Fix unread counts for users on small servers. Introduced in v1.62.0rc1. ([\#13168](https://github.com/matrix-org/synapse/issues/13168)) +- Update the version of the [ldap3 plugin](https://github.com/matrix-org/matrix-synapse-ldap3/) included in the `matrixdotorg/synapse` DockerHub images and the Debian packages hosted on `packages.matrix.org` to 0.2.1. This fixes [a bug](https://github.com/matrix-org/matrix-synapse-ldap3/pull/163) with usernames containing uppercase characters. ([\#13156](https://github.com/matrix-org/synapse/issues/13156)) +- Fix a bug introduced in Synapse 1.62.0rc1 affecting unread counts for users on small servers. ([\#13168](https://github.com/matrix-org/synapse/issues/13168)) Synapse 1.62.0rc2 (2022-07-01) From dcc4e0621cc101271efc573600bd7591a12cea7c Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 4 Jul 2022 17:47:44 +0100 Subject: [PATCH 23/35] Up the dependency on canonicaljson to ^1.5.0 --- pyproject.toml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index b9f2ea432c..c098b8df03 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -110,7 +110,9 @@ jsonschema = ">=3.0.0" frozendict = ">=1,!=2.1.2" # We require 2.1.0 or higher for type hints. Previous guard was >= 1.1.0 unpaddedbase64 = ">=2.1.0" -canonicaljson = "^1.4.0" +# We require 1.5.0 to work around an issue when running against the C implementation of +# frozendict: https://github.com/matrix-org/python-canonicaljson/issues/36 +canonicaljson = "^1.5.0" # we use the type definitions added in signedjson 1.1. signedjson = "^1.1.0" # validating SSL certs for IP addresses requires service_identity 18.1. From 5b5c943e7d978475c30b52941b678eac36008dc9 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 4 Jul 2022 17:48:09 +0100 Subject: [PATCH 24/35] Revert "Up the dependency on canonicaljson to ^1.5.0" This reverts commit dcc4e0621cc101271efc573600bd7591a12cea7c. --- pyproject.toml | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index c098b8df03..b9f2ea432c 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -110,9 +110,7 @@ jsonschema = ">=3.0.0" frozendict = ">=1,!=2.1.2" # We require 2.1.0 or higher for type hints. Previous guard was >= 1.1.0 unpaddedbase64 = ">=2.1.0" -# We require 1.5.0 to work around an issue when running against the C implementation of -# frozendict: https://github.com/matrix-org/python-canonicaljson/issues/36 -canonicaljson = "^1.5.0" +canonicaljson = "^1.4.0" # we use the type definitions added in signedjson 1.1. signedjson = "^1.1.0" # validating SSL certs for IP addresses requires service_identity 18.1. From d102ad67fddc650c34baa89dc7b2926d46a9aeca Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 4 Jul 2022 18:08:56 +0100 Subject: [PATCH 25/35] annotate tests.server.FakeChannel (#13136) --- changelog.d/13136.misc | 1 + tests/rest/admin/test_room.py | 4 +-- tests/rest/admin/test_user.py | 2 +- tests/rest/client/test_account.py | 5 ++-- tests/rest/client/test_profile.py | 10 +++++--- tests/rest/client/test_relations.py | 2 +- tests/server.py | 38 ++++++++++++++++------------- 7 files changed, 36 insertions(+), 26 deletions(-) create mode 100644 changelog.d/13136.misc diff --git a/changelog.d/13136.misc b/changelog.d/13136.misc new file mode 100644 index 0000000000..6cf451d8cf --- /dev/null +++ b/changelog.d/13136.misc @@ -0,0 +1 @@ +Add type annotations to `tests.server`. diff --git a/tests/rest/admin/test_room.py b/tests/rest/admin/test_room.py index ca6af9417b..230dc76f72 100644 --- a/tests/rest/admin/test_room.py +++ b/tests/rest/admin/test_room.py @@ -1579,8 +1579,8 @@ class RoomTestCase(unittest.HomeserverTestCase): access_token=self.admin_user_tok, ) self.assertEqual(HTTPStatus.OK, channel.code, msg=channel.json_body) - self.assertEqual(room_id, channel.json_body.get("rooms")[0].get("room_id")) - self.assertEqual("ж", channel.json_body.get("rooms")[0].get("name")) + self.assertEqual(room_id, channel.json_body["rooms"][0].get("room_id")) + self.assertEqual("ж", channel.json_body["rooms"][0].get("name")) def test_single_room(self) -> None: """Test that a single room can be requested correctly""" diff --git a/tests/rest/admin/test_user.py b/tests/rest/admin/test_user.py index 0d44102237..e32aaadb98 100644 --- a/tests/rest/admin/test_user.py +++ b/tests/rest/admin/test_user.py @@ -1488,7 +1488,7 @@ class UserRestTestCase(unittest.HomeserverTestCase): if channel.code != HTTPStatus.OK: raise HttpResponseException( - channel.code, channel.result["reason"], channel.json_body + channel.code, channel.result["reason"], channel.result["body"] ) # Set monthly active users to the limit diff --git a/tests/rest/client/test_account.py b/tests/rest/client/test_account.py index a43a137273..1f9b65351e 100644 --- a/tests/rest/client/test_account.py +++ b/tests/rest/client/test_account.py @@ -949,7 +949,7 @@ class ThreepidEmailRestTestCase(unittest.HomeserverTestCase): client_secret: str, next_link: Optional[str] = None, expect_code: int = 200, - ) -> str: + ) -> Optional[str]: """Request a validation token to add an email address to a user's account Args: @@ -959,7 +959,8 @@ class ThreepidEmailRestTestCase(unittest.HomeserverTestCase): expect_code: Expected return code of the call Returns: - The ID of the new threepid validation session + The ID of the new threepid validation session, or None if the response + did not contain a session ID. """ body = {"client_secret": client_secret, "email": email, "send_attempt": 1} if next_link: diff --git a/tests/rest/client/test_profile.py b/tests/rest/client/test_profile.py index 29bed0e872..8de5a342ae 100644 --- a/tests/rest/client/test_profile.py +++ b/tests/rest/client/test_profile.py @@ -153,18 +153,22 @@ class ProfileTestCase(unittest.HomeserverTestCase): ) self.assertEqual(channel.code, 400, channel.result) - def _get_displayname(self, name: Optional[str] = None) -> str: + def _get_displayname(self, name: Optional[str] = None) -> Optional[str]: channel = self.make_request( "GET", "/profile/%s/displayname" % (name or self.owner,) ) self.assertEqual(channel.code, 200, channel.result) - return channel.json_body["displayname"] + # FIXME: If a user has no displayname set, Synapse returns 200 and omits a + # displayname from the response. This contradicts the spec, see #13137. + return channel.json_body.get("displayname") - def _get_avatar_url(self, name: Optional[str] = None) -> str: + def _get_avatar_url(self, name: Optional[str] = None) -> Optional[str]: channel = self.make_request( "GET", "/profile/%s/avatar_url" % (name or self.owner,) ) self.assertEqual(channel.code, 200, channel.result) + # FIXME: If a user has no avatar set, Synapse returns 200 and omits an + # avatar_url from the response. This contradicts the spec, see #13137. return channel.json_body.get("avatar_url") @unittest.override_config({"max_avatar_size": 50}) diff --git a/tests/rest/client/test_relations.py b/tests/rest/client/test_relations.py index aa84906548..ad03eee17b 100644 --- a/tests/rest/client/test_relations.py +++ b/tests/rest/client/test_relations.py @@ -800,7 +800,7 @@ class RelationPaginationTestCase(BaseRelationsTestCase): ) expected_event_ids.append(channel.json_body["event_id"]) - prev_token = "" + prev_token: Optional[str] = "" found_event_ids: List[str] = [] for _ in range(20): from_token = "" diff --git a/tests/server.py b/tests/server.py index ce017ca0f6..df3f1564c9 100644 --- a/tests/server.py +++ b/tests/server.py @@ -43,6 +43,7 @@ from twisted.internet.defer import Deferred, fail, maybeDeferred, succeed from twisted.internet.error import DNSLookupError from twisted.internet.interfaces import ( IAddress, + IConsumer, IHostnameResolver, IProtocol, IPullProducer, @@ -53,11 +54,7 @@ from twisted.internet.interfaces import ( ITransport, ) from twisted.python.failure import Failure -from twisted.test.proto_helpers import ( - AccumulatingProtocol, - MemoryReactor, - MemoryReactorClock, -) +from twisted.test.proto_helpers import AccumulatingProtocol, MemoryReactorClock from twisted.web.http_headers import Headers from twisted.web.resource import IResource from twisted.web.server import Request, Site @@ -96,6 +93,7 @@ class TimedOutException(Exception): """ +@implementer(IConsumer) @attr.s(auto_attribs=True) class FakeChannel: """ @@ -104,7 +102,7 @@ class FakeChannel: """ site: Union[Site, "FakeSite"] - _reactor: MemoryReactor + _reactor: MemoryReactorClock result: dict = attr.Factory(dict) _ip: str = "127.0.0.1" _producer: Optional[Union[IPullProducer, IPushProducer]] = None @@ -122,7 +120,7 @@ class FakeChannel: self._request = request @property - def json_body(self): + def json_body(self) -> JsonDict: return json.loads(self.text_body) @property @@ -140,7 +138,7 @@ class FakeChannel: return self.result.get("done", False) @property - def code(self): + def code(self) -> int: if not self.result: raise Exception("No result yet.") return int(self.result["code"]) @@ -160,7 +158,7 @@ class FakeChannel: self.result["reason"] = reason self.result["headers"] = headers - def write(self, content): + def write(self, content: bytes) -> None: assert isinstance(content, bytes), "Should be bytes! " + repr(content) if "body" not in self.result: @@ -168,11 +166,16 @@ class FakeChannel: self.result["body"] += content - def registerProducer(self, producer, streaming): + # Type ignore: mypy doesn't like the fact that producer isn't an IProducer. + def registerProducer( # type: ignore[override] + self, + producer: Union[IPullProducer, IPushProducer], + streaming: bool, + ) -> None: self._producer = producer self.producerStreaming = streaming - def _produce(): + def _produce() -> None: if self._producer: self._producer.resumeProducing() self._reactor.callLater(0.1, _produce) @@ -180,31 +183,32 @@ class FakeChannel: if not streaming: self._reactor.callLater(0.0, _produce) - def unregisterProducer(self): + def unregisterProducer(self) -> None: if self._producer is None: return self._producer = None - def requestDone(self, _self): + def requestDone(self, _self: Request) -> None: self.result["done"] = True if isinstance(_self, SynapseRequest): + assert _self.logcontext is not None self.resource_usage = _self.logcontext.get_resource_usage() - def getPeer(self): + def getPeer(self) -> IAddress: # We give an address so that getClientAddress/getClientIP returns a non null entry, # causing us to record the MAU return address.IPv4Address("TCP", self._ip, 3423) - def getHost(self): + def getHost(self) -> IAddress: # this is called by Request.__init__ to configure Request.host. return address.IPv4Address("TCP", "127.0.0.1", 8888) - def isSecure(self): + def isSecure(self) -> bool: return False @property - def transport(self): + def transport(self) -> "FakeChannel": return self def await_result(self, timeout_ms: int = 1000) -> None: From e514495465a52531da6c833e4c926f3d1625ae5e Mon Sep 17 00:00:00 2001 From: Dirk Klimpel <5740567+dklimpel@users.noreply.github.com> Date: Tue, 5 Jul 2022 11:10:26 +0200 Subject: [PATCH 26/35] Add missing links to config options (#13166) --- changelog.d/13166.doc | 1 + docs/usage/configuration/config_documentation.md | 6 +++--- 2 files changed, 4 insertions(+), 3 deletions(-) create mode 100644 changelog.d/13166.doc diff --git a/changelog.d/13166.doc b/changelog.d/13166.doc new file mode 100644 index 0000000000..2d92e341ed --- /dev/null +++ b/changelog.d/13166.doc @@ -0,0 +1 @@ +Add missing links to config options. diff --git a/docs/usage/configuration/config_documentation.md b/docs/usage/configuration/config_documentation.md index 82edd53e36..ef411c5356 100644 --- a/docs/usage/configuration/config_documentation.md +++ b/docs/usage/configuration/config_documentation.md @@ -591,7 +591,7 @@ Example configuration: dummy_events_threshold: 5 ``` --- -Config option `delete_stale_devices_after` +### `delete_stale_devices_after` An optional duration. If set, Synapse will run a daily background task to log out and delete any device that hasn't been accessed for more than the specified amount of time. @@ -1843,7 +1843,7 @@ Example configuration: turn_shared_secret: "YOUR_SHARED_SECRET" ``` ---- -Config options: `turn_username` and `turn_password` +### `turn_username` and `turn_password` The Username and password if the TURN server needs them and does not use a token. @@ -3373,7 +3373,7 @@ alias_creation_rules: action: deny ``` --- -Config options: `room_list_publication_rules` +### `room_list_publication_rules` The `room_list_publication_rules` option controls who can publish and which rooms can be published in the public room list. From 65e675504fe060e5e99e145be450fe4d492f404f Mon Sep 17 00:00:00 2001 From: reivilibre Date: Tue, 5 Jul 2022 10:46:20 +0100 Subject: [PATCH 27/35] Add the ability to set the log level using the `SYNAPSE_TEST_LOG_LEVEL` environment when using `complement.sh`. (#13152) --- changelog.d/13152.misc | 1 + docker/README.md | 7 +++++++ docker/conf/log.config | 6 ++++++ docker/configure_workers_and_start.py | 20 ++++++++++++++------ docs/development/contributing_guide.md | 4 ++++ scripts-dev/complement.sh | 12 ++++++++++++ 6 files changed, 44 insertions(+), 6 deletions(-) create mode 100644 changelog.d/13152.misc diff --git a/changelog.d/13152.misc b/changelog.d/13152.misc new file mode 100644 index 0000000000..0c919ab700 --- /dev/null +++ b/changelog.d/13152.misc @@ -0,0 +1 @@ +Add the ability to set the log level using the `SYNAPSE_TEST_LOG_LEVEL` environment when using `complement.sh`. \ No newline at end of file diff --git a/docker/README.md b/docker/README.md index 67c3bc65f0..5b7de2fe38 100644 --- a/docker/README.md +++ b/docker/README.md @@ -67,6 +67,13 @@ The following environment variables are supported in `generate` mode: * `UID`, `GID`: the user id and group id to use for creating the data directories. If unset, and no user is set via `docker run --user`, defaults to `991`, `991`. +* `SYNAPSE_LOG_LEVEL`: the log level to use (one of `DEBUG`, `INFO`, `WARNING` or `ERROR`). + Defaults to `INFO`. +* `SYNAPSE_LOG_SENSITIVE`: if set and the log level is set to `DEBUG`, Synapse + will log sensitive information such as access tokens. + This should not be needed unless you are a developer attempting to debug something + particularly tricky. + ## Postgres diff --git a/docker/conf/log.config b/docker/conf/log.config index d9e85aa533..90b5179838 100644 --- a/docker/conf/log.config +++ b/docker/conf/log.config @@ -49,11 +49,17 @@ handlers: class: logging.StreamHandler formatter: precise +{% if not SYNAPSE_LOG_SENSITIVE %} +{# + If SYNAPSE_LOG_SENSITIVE is unset, then override synapse.storage.SQL to INFO + so that DEBUG entries (containing sensitive information) are not emitted. +#} loggers: synapse.storage.SQL: # beware: increasing this to DEBUG will make synapse log sensitive # information such as access tokens. level: INFO +{% endif %} root: level: {{ SYNAPSE_LOG_LEVEL or "INFO" }} diff --git a/docker/configure_workers_and_start.py b/docker/configure_workers_and_start.py index 4521f99eb4..51583dc13d 100755 --- a/docker/configure_workers_and_start.py +++ b/docker/configure_workers_and_start.py @@ -29,6 +29,10 @@ # * SYNAPSE_USE_EXPERIMENTAL_FORKING_LAUNCHER: Whether to use the forking launcher, # only intended for usage in Complement at the moment. # No stability guarantees are provided. +# * SYNAPSE_LOG_LEVEL: Set this to DEBUG, INFO, WARNING or ERROR to change the +# log level. INFO is the default. +# * SYNAPSE_LOG_SENSITIVE: If unset, SQL and SQL values won't be logged, +# regardless of the SYNAPSE_LOG_LEVEL setting. # # NOTE: According to Complement's ENTRYPOINT expectations for a homeserver image (as defined # in the project's README), this script may be run multiple times, and functionality should @@ -38,7 +42,7 @@ import os import subprocess import sys from pathlib import Path -from typing import Any, Dict, List, Mapping, MutableMapping, NoReturn, Set +from typing import Any, Dict, List, Mapping, MutableMapping, NoReturn, Optional, Set import yaml from jinja2 import Environment, FileSystemLoader @@ -552,13 +556,17 @@ def generate_worker_log_config( Returns: the path to the generated file """ # Check whether we should write worker logs to disk, in addition to the console - extra_log_template_args = {} + extra_log_template_args: Dict[str, Optional[str]] = {} if environ.get("SYNAPSE_WORKERS_WRITE_LOGS_TO_DISK"): - extra_log_template_args["LOG_FILE_PATH"] = "{dir}/logs/{name}.log".format( - dir=data_dir, name=worker_name - ) + extra_log_template_args["LOG_FILE_PATH"] = f"{data_dir}/logs/{worker_name}.log" + + extra_log_template_args["SYNAPSE_LOG_LEVEL"] = environ.get("SYNAPSE_LOG_LEVEL") + extra_log_template_args["SYNAPSE_LOG_SENSITIVE"] = environ.get( + "SYNAPSE_LOG_SENSITIVE" + ) + # Render and write the file - log_config_filepath = "/conf/workers/{name}.log.config".format(name=worker_name) + log_config_filepath = f"/conf/workers/{worker_name}.log.config" convert( "/conf/log.config", log_config_filepath, diff --git a/docs/development/contributing_guide.md b/docs/development/contributing_guide.md index 4738f8a6b6..900369b80f 100644 --- a/docs/development/contributing_guide.md +++ b/docs/development/contributing_guide.md @@ -309,6 +309,10 @@ The above will run a monolithic (single-process) Synapse with SQLite as the data - Passing `POSTGRES=1` as an environment variable to use the Postgres database instead. - Passing `WORKERS=1` as an environment variable to use a workerised setup instead. This option implies the use of Postgres. +To increase the log level for the tests, set `SYNAPSE_TEST_LOG_LEVEL`, e.g: +```sh +SYNAPSE_TEST_LOG_LEVEL=DEBUG COMPLEMENT_DIR=../complement ./scripts-dev/complement.sh -run TestImportHistoricalMessages +``` ### Prettier formatting with `gotestfmt` diff --git a/scripts-dev/complement.sh b/scripts-dev/complement.sh index 8448d49e26..705243ca9b 100755 --- a/scripts-dev/complement.sh +++ b/scripts-dev/complement.sh @@ -145,6 +145,18 @@ else test_tags="$test_tags,faster_joins" fi + +if [[ -n "$SYNAPSE_TEST_LOG_LEVEL" ]]; then + # Set the log level to what is desired + export PASS_SYNAPSE_LOG_LEVEL="$SYNAPSE_TEST_LOG_LEVEL" + + # Allow logging sensitive things (currently SQL queries & parameters). + # (This won't have any effect if we're not logging at DEBUG level overall.) + # Since this is just a test suite, this is fine and won't reveal anyone's + # personal information + export PASS_SYNAPSE_LOG_SENSITIVE=1 +fi + # Run the tests! echo "Images built; running complement" cd "$COMPLEMENT_DIR" From cf63d57dcee264b43afb424a514da1dc7cf03a91 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 5 Jul 2022 11:14:27 +0100 Subject: [PATCH 28/35] 1.62.0 --- CHANGES.md | 6 ++++++ debian/changelog | 6 ++++++ pyproject.toml | 2 +- 3 files changed, 13 insertions(+), 1 deletion(-) diff --git a/CHANGES.md b/CHANGES.md index babfe1628f..2db96249e0 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,3 +1,9 @@ +Synapse 1.62.0 (2022-07-05) +=========================== + +No significant changes since 1.62.0rc3. + + Synapse 1.62.0rc3 (2022-07-04) ============================== diff --git a/debian/changelog b/debian/changelog index c3a727800c..520d8d20ae 100644 --- a/debian/changelog +++ b/debian/changelog @@ -1,3 +1,9 @@ +matrix-synapse-py3 (1.62.0) stable; urgency=medium + + * New Synapse release 1.62.0. + + -- Synapse Packaging team Tue, 05 Jul 2022 11:14:15 +0100 + matrix-synapse-py3 (1.62.0~rc3) stable; urgency=medium * New Synapse release 1.62.0rc3. diff --git a/pyproject.toml b/pyproject.toml index b9f2ea432c..4d1007fcb3 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -54,7 +54,7 @@ skip_gitignore = true [tool.poetry] name = "matrix-synapse" -version = "1.62.0rc3" +version = "1.62.0" description = "Homeserver for the Matrix decentralised comms protocol" authors = ["Matrix.org Team and Contributors "] license = "Apache-2.0" From b51a0f4be0287f88a747952fb3cc8132d29df4c8 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 5 Jul 2022 11:19:54 +0100 Subject: [PATCH 29/35] Mention the spamchecker plugins --- CHANGES.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGES.md b/CHANGES.md index 2db96249e0..ec27cda1b2 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -3,6 +3,8 @@ Synapse 1.62.0 (2022-07-05) No significant changes since 1.62.0rc3. +Authors of spam-checker plugins should consult the [upgrade notes](https://github.com/matrix-org/synapse/blob/release-v1.62/docs/upgrade.md#upgrading-to-v1620) to learn about the enriched signatures for spam checker callbacks, which are supported with this release of Synapse. + Synapse 1.62.0rc3 (2022-07-04) ============================== From 2c2a42cc107fb02bbf7c8d4e6141cbe601221629 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 5 Jul 2022 05:56:06 -0500 Subject: [PATCH 30/35] Fix application service not being able to join remote federated room without a profile set (#13131) Fix https://github.com/matrix-org/synapse/issues/4778 Complement tests: https://github.com/matrix-org/complement/pull/399 --- changelog.d/13131.bugfix | 1 + synapse/handlers/room_member.py | 32 +++++++++++++++++++++++--------- 2 files changed, 24 insertions(+), 9 deletions(-) create mode 100644 changelog.d/13131.bugfix diff --git a/changelog.d/13131.bugfix b/changelog.d/13131.bugfix new file mode 100644 index 0000000000..06602f03fe --- /dev/null +++ b/changelog.d/13131.bugfix @@ -0,0 +1 @@ +Fix application service not being able to join remote federated room without a profile set. diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index 5648ab4bf4..a1d8875dd8 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -846,10 +846,17 @@ class RoomMemberHandler(metaclass=abc.ABCMeta): content["membership"] = Membership.JOIN - profile = self.profile_handler - if not content_specified: - content["displayname"] = await profile.get_displayname(target) - content["avatar_url"] = await profile.get_avatar_url(target) + try: + profile = self.profile_handler + if not content_specified: + content["displayname"] = await profile.get_displayname(target) + content["avatar_url"] = await profile.get_avatar_url(target) + except Exception as e: + logger.info( + "Failed to get profile information while processing remote join for %r: %s", + target, + e, + ) if requester.is_guest: content["kind"] = "guest" @@ -926,11 +933,18 @@ class RoomMemberHandler(metaclass=abc.ABCMeta): content["membership"] = Membership.KNOCK - profile = self.profile_handler - if "displayname" not in content: - content["displayname"] = await profile.get_displayname(target) - if "avatar_url" not in content: - content["avatar_url"] = await profile.get_avatar_url(target) + try: + profile = self.profile_handler + if "displayname" not in content: + content["displayname"] = await profile.get_displayname(target) + if "avatar_url" not in content: + content["avatar_url"] = await profile.get_avatar_url(target) + except Exception as e: + logger.info( + "Failed to get profile information while processing remote knock for %r: %s", + target, + e, + ) return await self.remote_knock( remote_room_hosts, room_id, target, content From 578a5e24a905c5d90d5c609cb485a5ab7277f8a5 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 5 Jul 2022 13:51:04 +0100 Subject: [PATCH 31/35] Use upserts for updating `event_push_summary` (#13153) --- changelog.d/13153.misc | 1 + .../databases/main/event_push_actions.py | 47 +++---------------- 2 files changed, 8 insertions(+), 40 deletions(-) create mode 100644 changelog.d/13153.misc diff --git a/changelog.d/13153.misc b/changelog.d/13153.misc new file mode 100644 index 0000000000..3bb51962e7 --- /dev/null +++ b/changelog.d/13153.misc @@ -0,0 +1 @@ +Reduce DB usage of `/sync` when a large number of unread messages have recently been sent in a room. diff --git a/synapse/storage/databases/main/event_push_actions.py b/synapse/storage/databases/main/event_push_actions.py index bb6e104d71..32536430aa 100644 --- a/synapse/storage/databases/main/event_push_actions.py +++ b/synapse/storage/databases/main/event_push_actions.py @@ -1013,8 +1013,7 @@ class EventPushActionsWorkerStore(ReceiptsWorkerStore, StreamWorkerStore, SQLBas sql = """ SELECT user_id, room_id, coalesce(old.%s, 0) + upd.cnt, - upd.stream_ordering, - old.user_id + upd.stream_ordering FROM ( SELECT user_id, room_id, count(*) as cnt, max(stream_ordering) as stream_ordering @@ -1042,7 +1041,6 @@ class EventPushActionsWorkerStore(ReceiptsWorkerStore, StreamWorkerStore, SQLBas summaries[(row[0], row[1])] = _EventPushSummary( unread_count=row[2], stream_ordering=row[3], - old_user_id=row[4], notif_count=0, ) @@ -1063,57 +1061,27 @@ class EventPushActionsWorkerStore(ReceiptsWorkerStore, StreamWorkerStore, SQLBas summaries[(row[0], row[1])] = _EventPushSummary( unread_count=0, stream_ordering=row[3], - old_user_id=row[4], notif_count=row[2], ) logger.info("Rotating notifications, handling %d rows", len(summaries)) - # If the `old.user_id` above is NULL then we know there isn't already an - # entry in the table, so we simply insert it. Otherwise we update the - # existing table. - self.db_pool.simple_insert_many_txn( + self.db_pool.simple_upsert_many_txn( txn, table="event_push_summary", - keys=( - "user_id", - "room_id", - "notif_count", - "unread_count", - "stream_ordering", - ), - values=[ + key_names=("user_id", "room_id"), + key_values=[(user_id, room_id) for user_id, room_id in summaries], + value_names=("notif_count", "unread_count", "stream_ordering"), + value_values=[ ( - user_id, - room_id, summary.notif_count, summary.unread_count, summary.stream_ordering, ) - for ((user_id, room_id), summary) in summaries.items() - if summary.old_user_id is None + for summary in summaries.values() ], ) - txn.execute_batch( - """ - UPDATE event_push_summary - SET notif_count = ?, unread_count = ?, stream_ordering = ? - WHERE user_id = ? AND room_id = ? - """, - ( - ( - summary.notif_count, - summary.unread_count, - summary.stream_ordering, - user_id, - room_id, - ) - for ((user_id, room_id), summary) in summaries.items() - if summary.old_user_id is not None - ), - ) - txn.execute( "UPDATE event_push_summary_stream_ordering SET stream_ordering = ?", (rotate_to_stream_ordering,), @@ -1293,5 +1261,4 @@ class _EventPushSummary: unread_count: int stream_ordering: int - old_user_id: str notif_count: int From 68695d80074f4d3bdf07970d541c07b98adffc76 Mon Sep 17 00:00:00 2001 From: reivilibre Date: Tue, 5 Jul 2022 14:24:42 +0100 Subject: [PATCH 32/35] Factor out some common Complement CI setup commands to a script. (#13157) --- .ci/scripts/setup_complement_prerequisites.sh | 36 +++++++++++++++++++ .github/workflows/tests.yml | 25 ++----------- .github/workflows/twisted_trunk.yml | 27 ++------------ changelog.d/13157.misc | 1 + 4 files changed, 42 insertions(+), 47 deletions(-) create mode 100755 .ci/scripts/setup_complement_prerequisites.sh create mode 100644 changelog.d/13157.misc diff --git a/.ci/scripts/setup_complement_prerequisites.sh b/.ci/scripts/setup_complement_prerequisites.sh new file mode 100755 index 0000000000..4848901cbf --- /dev/null +++ b/.ci/scripts/setup_complement_prerequisites.sh @@ -0,0 +1,36 @@ +#!/bin/sh +# +# Common commands to set up Complement's prerequisites in a GitHub Actions CI run. +# +# Must be called after Synapse has been checked out to `synapse/`. +# +set -eu + +alias block='{ set +x; } 2>/dev/null; func() { echo "::group::$*"; set -x; }; func' +alias endblock='{ set +x; } 2>/dev/null; func() { echo "::endgroup::"; set -x; }; func' + +block Set Go Version + # The path is set via a file given by $GITHUB_PATH. We need both Go 1.17 and GOPATH on the path to run Complement. + # See https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions#adding-a-system-path + + # Add Go 1.17 to the PATH: see https://github.com/actions/virtual-environments/blob/main/images/linux/Ubuntu2004-Readme.md#environment-variables-2 + echo "$GOROOT_1_17_X64/bin" >> $GITHUB_PATH + # Add the Go path to the PATH: We need this so we can call gotestfmt + echo "~/go/bin" >> $GITHUB_PATH +endblock + +block Install Complement Dependencies + sudo apt-get -qq update && sudo apt-get install -qqy libolm3 libolm-dev + go get -v github.com/haveyoudebuggedit/gotestfmt/v2/cmd/gotestfmt@latest +endblock + +block Install custom gotestfmt template + mkdir .gotestfmt/github -p + cp synapse/.ci/complement_package.gotpl .gotestfmt/github/package.gotpl +endblock + +block Check out Complement + # Attempt to check out the same branch of Complement as the PR. If it + # doesn't exist, fallback to HEAD. + synapse/.ci/scripts/checkout_complement.sh +endblock diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index a775f70c4e..4bc29c8207 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -332,34 +332,13 @@ jobs: database: Postgres steps: - # The path is set via a file given by $GITHUB_PATH. We need both Go 1.17 and GOPATH on the path to run Complement. - # See https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions#adding-a-system-path - - name: "Set Go Version" - run: | - # Add Go 1.17 to the PATH: see https://github.com/actions/virtual-environments/blob/main/images/linux/Ubuntu2004-Readme.md#environment-variables-2 - echo "$GOROOT_1_17_X64/bin" >> $GITHUB_PATH - # Add the Go path to the PATH: We need this so we can call gotestfmt - echo "~/go/bin" >> $GITHUB_PATH - - - name: "Install Complement Dependencies" - run: | - sudo apt-get -qq update && sudo apt-get install -qqy libolm3 libolm-dev - go get -v github.com/haveyoudebuggedit/gotestfmt/v2/cmd/gotestfmt@latest - - name: Run actions/checkout@v2 for synapse uses: actions/checkout@v2 with: path: synapse - - name: "Install custom gotestfmt template" - run: | - mkdir .gotestfmt/github -p - cp synapse/.ci/complement_package.gotpl .gotestfmt/github/package.gotpl - - # Attempt to check out the same branch of Complement as the PR. If it - # doesn't exist, fallback to HEAD. - - name: Checkout complement - run: synapse/.ci/scripts/checkout_complement.sh + - name: Prepare Complement's Prerequisites + run: synapse/.ci/scripts/setup_complement_prerequisites.sh - run: | set -o pipefail diff --git a/.github/workflows/twisted_trunk.yml b/.github/workflows/twisted_trunk.yml index 12267405be..f35e82297f 100644 --- a/.github/workflows/twisted_trunk.yml +++ b/.github/workflows/twisted_trunk.yml @@ -114,25 +114,14 @@ jobs: database: Postgres steps: - # The path is set via a file given by $GITHUB_PATH. We need both Go 1.17 and GOPATH on the path to run Complement. - # See https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions#adding-a-system-path - - name: "Set Go Version" - run: | - # Add Go 1.17 to the PATH: see https://github.com/actions/virtual-environments/blob/main/images/linux/Ubuntu2004-Readme.md#environment-variables-2 - echo "$GOROOT_1_17_X64/bin" >> $GITHUB_PATH - # Add the Go path to the PATH: We need this so we can call gotestfmt - echo "~/go/bin" >> $GITHUB_PATH - - - name: "Install Complement Dependencies" - run: | - sudo apt-get update && sudo apt-get install -y libolm3 libolm-dev - go get -v github.com/haveyoudebuggedit/gotestfmt/v2/cmd/gotestfmt@latest - - name: Run actions/checkout@v2 for synapse uses: actions/checkout@v2 with: path: synapse + - name: Prepare Complement's Prerequisites + run: synapse/.ci/scripts/setup_complement_prerequisites.sh + # This step is specific to the 'Twisted trunk' test run: - name: Patch dependencies run: | @@ -146,16 +135,6 @@ jobs: # NOT IN 1.1.12 poetry lock --check working-directory: synapse - - name: "Install custom gotestfmt template" - run: | - mkdir .gotestfmt/github -p - cp synapse/.ci/complement_package.gotpl .gotestfmt/github/package.gotpl - - # Attempt to check out the same branch of Complement as the PR. If it - # doesn't exist, fallback to HEAD. - - name: Checkout complement - run: synapse/.ci/scripts/checkout_complement.sh - - run: | set -o pipefail TEST_ONLY_SKIP_DEP_HASH_VERIFICATION=1 POSTGRES=${{ (matrix.database == 'Postgres') && 1 || '' }} WORKERS=${{ (matrix.arrangement == 'workers') && 1 || '' }} COMPLEMENT_DIR=`pwd`/complement synapse/scripts-dev/complement.sh -json 2>&1 | gotestfmt diff --git a/changelog.d/13157.misc b/changelog.d/13157.misc new file mode 100644 index 0000000000..0133097c83 --- /dev/null +++ b/changelog.d/13157.misc @@ -0,0 +1 @@ +Enable Complement testing in the 'Twisted Trunk' CI runs. \ No newline at end of file From 6ba732fefe732f92b0266b17cb6e45388bbe002a Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 5 Jul 2022 15:13:47 +0100 Subject: [PATCH 33/35] Type `tests.utils` (#13028) * Cast to postgres types when handling postgres db * Remove unused method * Easy annotations * Annotate create_room * Use `ParamSpec` to annotate looping_call * Annotate `default_config` * Track `now` as a float `time_ms` returns an int like the proper Synapse `Clock` * Introduce a `Timer` dataclass * Introduce a Looper type * Suppress checking of a mock * tests.utils is typed * Changelog * Whoops, import ParamSpec from typing_extensions * ditch the psycopg2 casts --- changelog.d/13028.misc | 1 + mypy.ini | 3 + synapse/util/__init__.py | 6 +- synapse/util/caches/lrucache.py | 2 +- tests/utils.py | 134 ++++++++++++++++++++++---------- 5 files changed, 101 insertions(+), 45 deletions(-) create mode 100644 changelog.d/13028.misc diff --git a/changelog.d/13028.misc b/changelog.d/13028.misc new file mode 100644 index 0000000000..4e5f3d8f91 --- /dev/null +++ b/changelog.d/13028.misc @@ -0,0 +1 @@ +Add type annotations to `tests.utils`. diff --git a/mypy.ini b/mypy.ini index d757a88fd1..ea0ab003a8 100644 --- a/mypy.ini +++ b/mypy.ini @@ -126,6 +126,9 @@ disallow_untyped_defs = True [mypy-tests.federation.transport.test_client] disallow_untyped_defs = True +[mypy-tests.utils] +disallow_untyped_defs = True + ;; Dependencies without annotations ;; Before ignoring a module, check to see if type stubs are available. diff --git a/synapse/util/__init__.py b/synapse/util/__init__.py index 6323d452e7..a90f08dd4c 100644 --- a/synapse/util/__init__.py +++ b/synapse/util/__init__.py @@ -20,6 +20,7 @@ from typing import Any, Callable, Dict, Generator, Optional import attr from frozendict import frozendict from matrix_common.versionstring import get_distribution_version_string +from typing_extensions import ParamSpec from twisted.internet import defer, task from twisted.internet.defer import Deferred @@ -82,6 +83,9 @@ def unwrapFirstError(failure: Failure) -> Failure: return failure.value.subFailure # type: ignore[union-attr] # Issue in Twisted's annotations +P = ParamSpec("P") + + @attr.s(slots=True) class Clock: """ @@ -110,7 +114,7 @@ class Clock: return int(self.time() * 1000) def looping_call( - self, f: Callable, msec: float, *args: Any, **kwargs: Any + self, f: Callable[P, object], msec: float, *args: P.args, **kwargs: P.kwargs ) -> LoopingCall: """Call a function repeatedly. diff --git a/synapse/util/caches/lrucache.py b/synapse/util/caches/lrucache.py index a3b60578e3..8ed5325c5d 100644 --- a/synapse/util/caches/lrucache.py +++ b/synapse/util/caches/lrucache.py @@ -109,7 +109,7 @@ GLOBAL_ROOT = ListNode["_Node"].create_root_node() @wrap_as_background_process("LruCache._expire_old_entries") async def _expire_old_entries( - clock: Clock, expiry_seconds: int, autotune_config: Optional[dict] + clock: Clock, expiry_seconds: float, autotune_config: Optional[dict] ) -> None: """Walks the global cache list to find cache entries that haven't been accessed in the given number of seconds, or if a given memory threshold has been breached. diff --git a/tests/utils.py b/tests/utils.py index aca6a0083b..424cc4c2a0 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -15,12 +15,17 @@ import atexit import os +from typing import Any, Callable, Dict, List, Tuple, Union, overload + +import attr +from typing_extensions import Literal, ParamSpec from synapse.api.constants import EventTypes from synapse.api.room_versions import RoomVersions from synapse.config.homeserver import HomeServerConfig from synapse.config.server import DEFAULT_ROOM_VERSION from synapse.logging.context import current_context, set_current_context +from synapse.server import HomeServer from synapse.storage.database import LoggingDatabaseConnection from synapse.storage.engines import create_engine from synapse.storage.prepare_database import prepare_database @@ -50,12 +55,11 @@ SQLITE_PERSIST_DB = os.environ.get("SYNAPSE_TEST_PERSIST_SQLITE_DB") is not None POSTGRES_DBNAME_FOR_INITIAL_CREATE = "postgres" -def setupdb(): +def setupdb() -> None: # If we're using PostgreSQL, set up the db once if USE_POSTGRES_FOR_TESTS: # create a PostgresEngine db_engine = create_engine({"name": "psycopg2", "args": {}}) - # connect to postgres to create the base database. db_conn = db_engine.module.connect( user=POSTGRES_USER, @@ -82,11 +86,11 @@ def setupdb(): port=POSTGRES_PORT, password=POSTGRES_PASSWORD, ) - db_conn = LoggingDatabaseConnection(db_conn, db_engine, "tests") - prepare_database(db_conn, db_engine, None) - db_conn.close() + logging_conn = LoggingDatabaseConnection(db_conn, db_engine, "tests") + prepare_database(logging_conn, db_engine, None) + logging_conn.close() - def _cleanup(): + def _cleanup() -> None: db_conn = db_engine.module.connect( user=POSTGRES_USER, host=POSTGRES_HOST, @@ -103,7 +107,19 @@ def setupdb(): atexit.register(_cleanup) -def default_config(name, parse=False): +@overload +def default_config(name: str, parse: Literal[False] = ...) -> Dict[str, object]: + ... + + +@overload +def default_config(name: str, parse: Literal[True]) -> HomeServerConfig: + ... + + +def default_config( + name: str, parse: bool = False +) -> Union[Dict[str, object], HomeServerConfig]: """ Create a reasonable test config. """ @@ -181,90 +197,122 @@ def default_config(name, parse=False): return config_dict -def mock_getRawHeaders(headers=None): +def mock_getRawHeaders(headers=None): # type: ignore[no-untyped-def] headers = headers if headers is not None else {} - def getRawHeaders(name, default=None): + def getRawHeaders(name, default=None): # type: ignore[no-untyped-def] + # If the requested header is present, the real twisted function returns + # List[str] if name is a str and List[bytes] if name is a bytes. + # This mock doesn't support that behaviour. + # Fortunately, none of the current callers of mock_getRawHeaders() provide a + # headers dict, so we don't encounter this discrepancy in practice. return headers.get(name, default) return getRawHeaders +P = ParamSpec("P") + + +@attr.s(slots=True, auto_attribs=True) +class Timer: + absolute_time: float + callback: Callable[[], None] + expired: bool + + +# TODO: Make this generic over a ParamSpec? +@attr.s(slots=True, auto_attribs=True) +class Looper: + func: Callable[..., Any] + interval: float # seconds + last: float + args: Tuple[object, ...] + kwargs: Dict[str, object] + + class MockClock: - now = 1000 + now = 1000.0 - def __init__(self): - # list of lists of [absolute_time, callback, expired] in no particular - # order - self.timers = [] - self.loopers = [] + def __init__(self) -> None: + # Timers in no particular order + self.timers: List[Timer] = [] + self.loopers: List[Looper] = [] - def time(self): + def time(self) -> float: return self.now - def time_msec(self): - return self.time() * 1000 + def time_msec(self) -> int: + return int(self.time() * 1000) - def call_later(self, delay, callback, *args, **kwargs): + def call_later( + self, + delay: float, + callback: Callable[P, object], + *args: P.args, + **kwargs: P.kwargs, + ) -> Timer: ctx = current_context() - def wrapped_callback(): + def wrapped_callback() -> None: set_current_context(ctx) callback(*args, **kwargs) - t = [self.now + delay, wrapped_callback, False] + t = Timer(self.now + delay, wrapped_callback, False) self.timers.append(t) return t - def looping_call(self, function, interval, *args, **kwargs): - self.loopers.append([function, interval / 1000.0, self.now, args, kwargs]) + def looping_call( + self, + function: Callable[P, object], + interval: float, + *args: P.args, + **kwargs: P.kwargs, + ) -> None: + # This type-ignore should be redundant once we use a mypy release with + # https://github.com/python/mypy/pull/12668. + self.loopers.append(Looper(function, interval / 1000.0, self.now, args, kwargs)) # type: ignore[arg-type] - def cancel_call_later(self, timer, ignore_errs=False): - if timer[2]: + def cancel_call_later(self, timer: Timer, ignore_errs: bool = False) -> None: + if timer.expired: if not ignore_errs: raise Exception("Cannot cancel an expired timer") - timer[2] = True + timer.expired = True self.timers = [t for t in self.timers if t != timer] # For unit testing - def advance_time(self, secs): + def advance_time(self, secs: float) -> None: self.now += secs timers = self.timers self.timers = [] for t in timers: - time, callback, expired = t - - if expired: + if t.expired: raise Exception("Timer already expired") - if self.now >= time: - t[2] = True - callback() + if self.now >= t.absolute_time: + t.expired = True + t.callback() else: self.timers.append(t) for looped in self.loopers: - func, interval, last, args, kwargs = looped - if last + interval < self.now: - func(*args, **kwargs) - looped[2] = self.now + if looped.last + looped.interval < self.now: + looped.func(*looped.args, **looped.kwargs) + looped.last = self.now - def advance_time_msec(self, ms): + def advance_time_msec(self, ms: float) -> None: self.advance_time(ms / 1000.0) - def time_bound_deferred(self, d, *args, **kwargs): - # We don't bother timing things out for now. - return d - -async def create_room(hs, room_id: str, creator_id: str): +async def create_room(hs: HomeServer, room_id: str, creator_id: str) -> None: """Creates and persist a creation event for the given room""" persistence_store = hs.get_storage_controllers().persistence + assert persistence_store is not None store = hs.get_datastores().main event_builder_factory = hs.get_event_builder_factory() event_creation_handler = hs.get_event_creation_handler() From 68db233f0cf16a20f21fd927374121966976d9c7 Mon Sep 17 00:00:00 2001 From: Sean Quah <8349537+squahtx@users.noreply.github.com> Date: Tue, 5 Jul 2022 16:12:52 +0100 Subject: [PATCH 34/35] Handle race between persisting an event and un-partial stating a room (#13100) Whenever we want to persist an event, we first compute an event context, which includes the state at the event and a flag indicating whether the state is partial. After a lot of processing, we finally try to store the event in the database, which can fail for partial state events when the containing room has been un-partial stated in the meantime. We detect the race as a foreign key constraint failure in the data store layer and turn it into a special `PartialStateConflictError` exception, which makes its way up to the method in which we computed the event context. To make things difficult, the exception needs to cross a replication request: `/fed_send_events` for events coming over federation and `/send_event` for events from clients. We transport the `PartialStateConflictError` as a `409 Conflict` over replication and turn `409`s back into `PartialStateConflictError`s on the worker making the request. All client events go through `EventCreationHandler.handle_new_client_event`, which is called in *a lot* of places. Instead of trying to update all the code which creates client events, we turn the `PartialStateConflictError` into a `429 Too Many Requests` in `EventCreationHandler.handle_new_client_event` and hope that clients take it as a hint to retry their request. On the federation event side, there are 7 places which compute event contexts. 4 of them use outlier event contexts: `FederationEventHandler._auth_and_persist_outliers_inner`, `FederationHandler.do_knock`, `FederationHandler.on_invite_request` and `FederationHandler.do_remotely_reject_invite`. These events won't have the partial state flag, so we do not need to do anything for then. The remaining 3 paths which create events are `FederationEventHandler.process_remote_join`, `FederationEventHandler.on_send_membership_event` and `FederationEventHandler._process_received_pdu`. We can't experience the race in `process_remote_join`, unless we're handling an additional join into a partial state room, which currently blocks, so we make no attempt to handle it correctly. `on_send_membership_event` is only called by `FederationServer._on_send_membership_event`, so we catch the `PartialStateConflictError` there and retry just once. `_process_received_pdu` is called by `on_receive_pdu` for incoming events and `_process_pulled_event` for backfill. The latter should never try to persist partial state events, so we ignore it. We catch the `PartialStateConflictError` in `on_receive_pdu` and retry just once. Refering to the graph of code paths in https://github.com/matrix-org/synapse/issues/12988#issuecomment-1156857648 may make the above make more sense. Signed-off-by: Sean Quah --- changelog.d/13100.misc | 1 + synapse/federation/federation_server.py | 18 ++++- synapse/handlers/federation.py | 39 +++++---- synapse/handlers/federation_event.py | 51 +++++++++--- synapse/handlers/message.py | 79 ++++++++++++------ synapse/replication/http/federation.py | 3 + synapse/replication/http/send_event.py | 3 + synapse/storage/controllers/persist_events.py | 12 +++ synapse/storage/databases/main/events.py | 80 ++++++++++++++++--- synapse/storage/databases/main/room.py | 22 +++-- 10 files changed, 234 insertions(+), 74 deletions(-) create mode 100644 changelog.d/13100.misc diff --git a/changelog.d/13100.misc b/changelog.d/13100.misc new file mode 100644 index 0000000000..28f2fe0349 --- /dev/null +++ b/changelog.d/13100.misc @@ -0,0 +1 @@ +Faster room joins: Handle race between persisting an event and un-partial stating a room. diff --git a/synapse/federation/federation_server.py b/synapse/federation/federation_server.py index 3e1518f1f6..5dfdc86740 100644 --- a/synapse/federation/federation_server.py +++ b/synapse/federation/federation_server.py @@ -67,6 +67,7 @@ from synapse.replication.http.federation import ( ReplicationFederationSendEduRestServlet, ReplicationGetQueryRestServlet, ) +from synapse.storage.databases.main.events import PartialStateConflictError from synapse.storage.databases.main.lock import Lock from synapse.types import JsonDict, StateMap, get_domain_from_id from synapse.util import json_decoder, unwrapFirstError @@ -882,9 +883,20 @@ class FederationServer(FederationBase): logger.warning("%s", errmsg) raise SynapseError(403, errmsg, Codes.FORBIDDEN) - return await self._federation_event_handler.on_send_membership_event( - origin, event - ) + try: + return await self._federation_event_handler.on_send_membership_event( + origin, event + ) + except PartialStateConflictError: + # The room was un-partial stated while we were persisting the event. + # Try once more, with full state this time. + logger.info( + "Room %s was un-partial stated during `on_send_membership_event`, trying again.", + room_id, + ) + return await self._federation_event_handler.on_send_membership_event( + origin, event + ) async def on_event_auth( self, origin: str, room_id: str, event_id: str diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 34cc5ecd11..3c44b4bf86 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -45,6 +45,7 @@ from synapse.api.errors import ( FederationDeniedError, FederationError, HttpResponseException, + LimitExceededError, NotFoundError, RequestSendFailed, SynapseError, @@ -64,6 +65,7 @@ from synapse.replication.http.federation import ( ReplicationCleanRoomRestServlet, ReplicationStoreRoomOnOutlierMembershipRestServlet, ) +from synapse.storage.databases.main.events import PartialStateConflictError from synapse.storage.databases.main.events_worker import EventRedactBehaviour from synapse.storage.state import StateFilter from synapse.types import JsonDict, StateMap, get_domain_from_id @@ -549,15 +551,29 @@ class FederationHandler: # https://github.com/matrix-org/synapse/issues/12998 await self.store.store_partial_state_room(room_id, ret.servers_in_room) - max_stream_id = await self._federation_event_handler.process_remote_join( - origin, - room_id, - auth_chain, - state, - event, - room_version_obj, - partial_state=ret.partial_state, - ) + try: + max_stream_id = ( + await self._federation_event_handler.process_remote_join( + origin, + room_id, + auth_chain, + state, + event, + room_version_obj, + partial_state=ret.partial_state, + ) + ) + except PartialStateConflictError as e: + # The homeserver was already in the room and it is no longer partial + # stated. We ought to be doing a local join instead. Turn the error into + # a 429, as a hint to the client to try again. + # TODO(faster_joins): `_should_perform_remote_join` suggests that we may + # do a remote join for restricted rooms even if we have full state. + logger.error( + "Room %s was un-partial stated while processing remote join.", + room_id, + ) + raise LimitExceededError(msg=e.msg, errcode=e.errcode, retry_after_ms=0) if ret.partial_state: # Kick off the process of asynchronously fetching the state for this @@ -1567,11 +1583,6 @@ class FederationHandler: # we raced against more events arriving with partial state. Go round # the loop again. We've already logged a warning, so no need for more. - # TODO(faster_joins): there is still a race here, whereby incoming events which raced - # with us will fail to be persisted after the call to `clear_partial_state_room` due to - # having partial state. - # https://github.com/matrix-org/synapse/issues/12988 - # continue events = await self.store.get_events_as_list( diff --git a/synapse/handlers/federation_event.py b/synapse/handlers/federation_event.py index 479d936dc0..c74117c19a 100644 --- a/synapse/handlers/federation_event.py +++ b/synapse/handlers/federation_event.py @@ -64,6 +64,7 @@ from synapse.replication.http.federation import ( ReplicationFederationSendEventsRestServlet, ) from synapse.state import StateResolutionStore +from synapse.storage.databases.main.events import PartialStateConflictError from synapse.storage.databases.main.events_worker import EventRedactBehaviour from synapse.storage.state import StateFilter from synapse.types import ( @@ -275,7 +276,16 @@ class FederationEventHandler: affected=pdu.event_id, ) - await self._process_received_pdu(origin, pdu, state_ids=None) + try: + await self._process_received_pdu(origin, pdu, state_ids=None) + except PartialStateConflictError: + # The room was un-partial stated while we were processing the PDU. + # Try once more, with full state this time. + logger.info( + "Room %s was un-partial stated while processing the PDU, trying again.", + room_id, + ) + await self._process_received_pdu(origin, pdu, state_ids=None) async def on_send_membership_event( self, origin: str, event: EventBase @@ -306,6 +316,9 @@ class FederationEventHandler: Raises: SynapseError if the event is not accepted into the room + PartialStateConflictError if the room was un-partial stated in between + computing the state at the event and persisting it. The caller should + retry exactly once in this case. """ logger.debug( "on_send_membership_event: Got event: %s, signatures: %s", @@ -423,6 +436,8 @@ class FederationEventHandler: Raises: SynapseError if the response is in some way invalid. + PartialStateConflictError if the homeserver is already in the room and it + has been un-partial stated. """ create_event = None for e in state: @@ -1084,10 +1099,14 @@ class FederationEventHandler: state_ids: Normally None, but if we are handling a gap in the graph (ie, we are missing one or more prev_events), the resolved state at the - event + event. Must not be partial state. backfilled: True if this is part of a historical batch of events (inhibits notification to clients, and validation of device keys.) + + PartialStateConflictError: if the room was un-partial stated in between + computing the state at the event and persisting it. The caller should retry + exactly once in this case. Will never be raised if `state_ids` is provided. """ logger.debug("Processing event: %s", event) assert not event.internal_metadata.outlier @@ -1933,6 +1952,9 @@ class FederationEventHandler: event: The event itself. context: The event context. backfilled: True if the event was backfilled. + + PartialStateConflictError: if attempting to persist a partial state event in + a room that has been un-partial stated. """ # this method should not be called on outliers (those code paths call # persist_events_and_notify directly.) @@ -1985,6 +2007,10 @@ class FederationEventHandler: Returns: The stream ID after which all events have been persisted. + + Raises: + PartialStateConflictError: if attempting to persist a partial state event in + a room that has been un-partial stated. """ if not event_and_contexts: return self._store.get_room_max_stream_ordering() @@ -1993,14 +2019,19 @@ class FederationEventHandler: if instance != self._instance_name: # Limit the number of events sent over replication. We choose 200 # here as that is what we default to in `max_request_body_size(..)` - for batch in batch_iter(event_and_contexts, 200): - result = await self._send_events( - instance_name=instance, - store=self._store, - room_id=room_id, - event_and_contexts=batch, - backfilled=backfilled, - ) + try: + for batch in batch_iter(event_and_contexts, 200): + result = await self._send_events( + instance_name=instance, + store=self._store, + room_id=room_id, + event_and_contexts=batch, + backfilled=backfilled, + ) + except SynapseError as e: + if e.code == HTTPStatus.CONFLICT: + raise PartialStateConflictError() + raise return result["max_stream_id"] else: assert self._storage_controllers.persistence diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index c6b40a5b7a..1980e37dae 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -37,6 +37,7 @@ from synapse.api.errors import ( AuthError, Codes, ConsentNotGivenError, + LimitExceededError, NotFoundError, ShadowBanError, SynapseError, @@ -53,6 +54,7 @@ from synapse.handlers.directory import DirectoryHandler from synapse.logging.context import make_deferred_yieldable, run_in_background from synapse.metrics.background_process_metrics import run_as_background_process from synapse.replication.http.send_event import ReplicationSendEventRestServlet +from synapse.storage.databases.main.events import PartialStateConflictError from synapse.storage.databases.main.events_worker import EventRedactBehaviour from synapse.storage.state import StateFilter from synapse.types import ( @@ -1250,6 +1252,8 @@ class EventCreationHandler: Raises: ShadowBanError if the requester has been shadow-banned. + SynapseError(503) if attempting to persist a partial state event in + a room that has been un-partial stated. """ extra_users = extra_users or [] @@ -1300,24 +1304,35 @@ class EventCreationHandler: # We now persist the event (and update the cache in parallel, since we # don't want to block on it). - result, _ = await make_deferred_yieldable( - gather_results( - ( - run_in_background( - self._persist_event, - requester=requester, - event=event, - context=context, - ratelimit=ratelimit, - extra_users=extra_users, + try: + result, _ = await make_deferred_yieldable( + gather_results( + ( + run_in_background( + self._persist_event, + requester=requester, + event=event, + context=context, + ratelimit=ratelimit, + extra_users=extra_users, + ), + run_in_background( + self.cache_joined_hosts_for_event, event, context + ).addErrback( + log_failure, "cache_joined_hosts_for_event failed" + ), ), - run_in_background( - self.cache_joined_hosts_for_event, event, context - ).addErrback(log_failure, "cache_joined_hosts_for_event failed"), - ), - consumeErrors=True, + consumeErrors=True, + ) + ).addErrback(unwrapFirstError) + except PartialStateConflictError as e: + # The event context needs to be recomputed. + # Turn the error into a 429, as a hint to the client to try again. + logger.info( + "Room %s was un-partial stated while persisting client event.", + event.room_id, ) - ).addErrback(unwrapFirstError) + raise LimitExceededError(msg=e.msg, errcode=e.errcode, retry_after_ms=0) return result @@ -1332,6 +1347,9 @@ class EventCreationHandler: """Actually persists the event. Should only be called by `handle_new_client_event`, and see its docstring for documentation of the arguments. + + PartialStateConflictError: if attempting to persist a partial state event in + a room that has been un-partial stated. """ # Skip push notification actions for historical messages @@ -1348,16 +1366,21 @@ class EventCreationHandler: # If we're a worker we need to hit out to the master. writer_instance = self._events_shard_config.get_instance(event.room_id) if writer_instance != self._instance_name: - result = await self.send_event( - instance_name=writer_instance, - event_id=event.event_id, - store=self.store, - requester=requester, - event=event, - context=context, - ratelimit=ratelimit, - extra_users=extra_users, - ) + try: + result = await self.send_event( + instance_name=writer_instance, + event_id=event.event_id, + store=self.store, + requester=requester, + event=event, + context=context, + ratelimit=ratelimit, + extra_users=extra_users, + ) + except SynapseError as e: + if e.code == HTTPStatus.CONFLICT: + raise PartialStateConflictError() + raise stream_id = result["stream_id"] event_id = result["event_id"] if event_id != event.event_id: @@ -1485,6 +1508,10 @@ class EventCreationHandler: The persisted event. This may be different than the given event if it was de-duplicated (e.g. because we had already persisted an event with the same transaction ID.) + + Raises: + PartialStateConflictError: if attempting to persist a partial state event in + a room that has been un-partial stated. """ extra_users = extra_users or [] diff --git a/synapse/replication/http/federation.py b/synapse/replication/http/federation.py index eed29cd597..d3abafed28 100644 --- a/synapse/replication/http/federation.py +++ b/synapse/replication/http/federation.py @@ -60,6 +60,9 @@ class ReplicationFederationSendEventsRestServlet(ReplicationEndpoint): { "max_stream_id": 32443, } + + Responds with a 409 when a `PartialStateConflictError` is raised due to an event + context that needs to be recomputed due to the un-partial stating of a room. """ NAME = "fed_send_events" diff --git a/synapse/replication/http/send_event.py b/synapse/replication/http/send_event.py index c2b2588ea5..486f04723c 100644 --- a/synapse/replication/http/send_event.py +++ b/synapse/replication/http/send_event.py @@ -59,6 +59,9 @@ class ReplicationSendEventRestServlet(ReplicationEndpoint): { "stream_id": 12345, "event_id": "$abcdef..." } + Responds with a 409 when a `PartialStateConflictError` is raised due to an event + context that needs to be recomputed due to the un-partial stating of a room. + The returned event ID may not match the sent event if it was deduplicated. """ diff --git a/synapse/storage/controllers/persist_events.py b/synapse/storage/controllers/persist_events.py index 4bcb99d06e..c248fccc81 100644 --- a/synapse/storage/controllers/persist_events.py +++ b/synapse/storage/controllers/persist_events.py @@ -315,6 +315,10 @@ class EventsPersistenceStorageController: if they were deduplicated due to an event already existing that matched the transaction ID; the existing event is returned in such a case. + + Raises: + PartialStateConflictError: if attempting to persist a partial state event in + a room that has been un-partial stated. """ partitioned: Dict[str, List[Tuple[EventBase, EventContext]]] = {} for event, ctx in events_and_contexts: @@ -363,6 +367,10 @@ class EventsPersistenceStorageController: latest persisted event. The returned event may not match the given event if it was deduplicated due to an existing event matching the transaction ID. + + Raises: + PartialStateConflictError: if attempting to persist a partial state event in + a room that has been un-partial stated. """ # add_to_queue returns a map from event ID to existing event ID if the # event was deduplicated. (The dict may also include other entries if @@ -453,6 +461,10 @@ class EventsPersistenceStorageController: Returns: A dictionary of event ID to event ID we didn't persist as we already had another event persisted with the same TXN ID. + + Raises: + PartialStateConflictError: if attempting to persist a partial state event in + a room that has been un-partial stated. """ replaced_events: Dict[str, str] = {} if not events_and_contexts: diff --git a/synapse/storage/databases/main/events.py b/synapse/storage/databases/main/events.py index a3e12f1e9b..8a0e4e9589 100644 --- a/synapse/storage/databases/main/events.py +++ b/synapse/storage/databases/main/events.py @@ -16,6 +16,7 @@ import itertools import logging from collections import OrderedDict +from http import HTTPStatus from typing import ( TYPE_CHECKING, Any, @@ -35,6 +36,7 @@ from prometheus_client import Counter import synapse.metrics from synapse.api.constants import EventContentFields, EventTypes, RelationTypes +from synapse.api.errors import Codes, SynapseError from synapse.api.room_versions import RoomVersions from synapse.events import EventBase, relation_from_event from synapse.events.snapshot import EventContext @@ -69,6 +71,24 @@ event_counter = Counter( ) +class PartialStateConflictError(SynapseError): + """An internal error raised when attempting to persist an event with partial state + after the room containing the event has been un-partial stated. + + This error should be handled by recomputing the event context and trying again. + + This error has an HTTP status code so that it can be transported over replication. + It should not be exposed to clients. + """ + + def __init__(self) -> None: + super().__init__( + HTTPStatus.CONFLICT, + msg="Cannot persist partial state event in un-partial stated room", + errcode=Codes.UNKNOWN, + ) + + @attr.s(slots=True, auto_attribs=True) class DeltaState: """Deltas to use to update the `current_state_events` table. @@ -154,6 +174,10 @@ class PersistEventsStore: Returns: Resolves when the events have been persisted + + Raises: + PartialStateConflictError: if attempting to persist a partial state event in + a room that has been un-partial stated. """ # We want to calculate the stream orderings as late as possible, as @@ -354,6 +378,9 @@ class PersistEventsStore: For each room, a list of the event ids which are the forward extremities. + Raises: + PartialStateConflictError: if attempting to persist a partial state event in + a room that has been un-partial stated. """ state_delta_for_room = state_delta_for_room or {} new_forward_extremities = new_forward_extremities or {} @@ -1304,6 +1331,10 @@ class PersistEventsStore: Returns: new list, without events which are already in the events table. + + Raises: + PartialStateConflictError: if attempting to persist a partial state event in + a room that has been un-partial stated. """ txn.execute( "SELECT event_id, outlier FROM events WHERE event_id in (%s)" @@ -2215,6 +2246,11 @@ class PersistEventsStore: txn: LoggingTransaction, events_and_contexts: Collection[Tuple[EventBase, EventContext]], ) -> None: + """ + Raises: + PartialStateConflictError: if attempting to persist a partial state event in + a room that has been un-partial stated. + """ state_groups = {} for event, context in events_and_contexts: if event.internal_metadata.is_outlier(): @@ -2239,19 +2275,37 @@ class PersistEventsStore: # if we have partial state for these events, record the fact. (This happens # here rather than in _store_event_txn because it also needs to happen when # we de-outlier an event.) - self.db_pool.simple_insert_many_txn( - txn, - table="partial_state_events", - keys=("room_id", "event_id"), - values=[ - ( - event.room_id, - event.event_id, - ) - for event, ctx in events_and_contexts - if ctx.partial_state - ], - ) + try: + self.db_pool.simple_insert_many_txn( + txn, + table="partial_state_events", + keys=("room_id", "event_id"), + values=[ + ( + event.room_id, + event.event_id, + ) + for event, ctx in events_and_contexts + if ctx.partial_state + ], + ) + except self.db_pool.engine.module.IntegrityError: + logger.info( + "Cannot persist events %s in rooms %s: room has been un-partial stated", + [ + event.event_id + for event, ctx in events_and_contexts + if ctx.partial_state + ], + list( + { + event.room_id + for event, ctx in events_and_contexts + if ctx.partial_state + } + ), + ) + raise PartialStateConflictError() self.db_pool.simple_upsert_many_txn( txn, diff --git a/synapse/storage/databases/main/room.py b/synapse/storage/databases/main/room.py index d8026e3fac..13d6a1d5c0 100644 --- a/synapse/storage/databases/main/room.py +++ b/synapse/storage/databases/main/room.py @@ -1156,19 +1156,25 @@ class RoomWorkerStore(CacheInvalidationWorkerStore): return room_servers async def clear_partial_state_room(self, room_id: str) -> bool: - # this can race with incoming events, so we watch out for FK errors. - # TODO(faster_joins): this still doesn't completely fix the race, since the persist process - # is not atomic. I fear we need an application-level lock. - # https://github.com/matrix-org/synapse/issues/12988 + """Clears the partial state flag for a room. + + Args: + room_id: The room whose partial state flag is to be cleared. + + Returns: + `True` if the partial state flag has been cleared successfully. + + `False` if the partial state flag could not be cleared because the room + still contains events with partial state. + """ try: await self.db_pool.runInteraction( "clear_partial_state_room", self._clear_partial_state_room_txn, room_id ) return True - except self.db_pool.engine.module.DatabaseError as e: - # TODO(faster_joins): how do we distinguish between FK errors and other errors? - # https://github.com/matrix-org/synapse/issues/12988 - logger.warning( + except self.db_pool.engine.module.IntegrityError as e: + # Assume that any `IntegrityError`s are due to partial state events. + logger.info( "Exception while clearing lazy partial-state-room %s, retrying: %s", room_id, e, From a0f51b059c2aa1bbe0a2d6991c369cba5cf43c0a Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 6 Jul 2022 12:09:19 +0100 Subject: [PATCH 35/35] Fix bug where we failed to delete old push actions (#13194) This happened if we encountered a stream ordering in `event_push_actions` that had more rows than the batch size of the delete, as If we don't delete any rows in an iteration then the next time round we get the exact same stream ordering and get stuck. --- changelog.d/13194.bugfix | 1 + synapse/storage/databases/main/event_push_actions.py | 6 ++++-- 2 files changed, 5 insertions(+), 2 deletions(-) create mode 100644 changelog.d/13194.bugfix diff --git a/changelog.d/13194.bugfix b/changelog.d/13194.bugfix new file mode 100644 index 0000000000..2c2e8bb21b --- /dev/null +++ b/changelog.d/13194.bugfix @@ -0,0 +1 @@ +Fix bug where rows were not deleted from `event_push_actions` table on large servers. Introduced in v1.62.0. diff --git a/synapse/storage/databases/main/event_push_actions.py b/synapse/storage/databases/main/event_push_actions.py index 32536430aa..a3edcbb398 100644 --- a/synapse/storage/databases/main/event_push_actions.py +++ b/synapse/storage/databases/main/event_push_actions.py @@ -1114,7 +1114,7 @@ class EventPushActionsWorkerStore(ReceiptsWorkerStore, StreamWorkerStore, SQLBas txn.execute( """ SELECT stream_ordering FROM event_push_actions - WHERE stream_ordering < ? AND highlight = 0 + WHERE stream_ordering <= ? AND highlight = 0 ORDER BY stream_ordering ASC LIMIT 1 OFFSET ? """, ( @@ -1129,10 +1129,12 @@ class EventPushActionsWorkerStore(ReceiptsWorkerStore, StreamWorkerStore, SQLBas else: stream_ordering = max_stream_ordering_to_delete + # We need to use a inclusive bound here to handle the case where a + # single stream ordering has more than `batch_size` rows. txn.execute( """ DELETE FROM event_push_actions - WHERE stream_ordering < ? AND highlight = 0 + WHERE stream_ordering <= ? AND highlight = 0 """, (stream_ordering,), )