From d936371b698ea3085472ee83ae9a88ea7832280e Mon Sep 17 00:00:00 2001 From: Sorunome Date: Wed, 9 Jun 2021 20:39:51 +0200 Subject: Implement knock feature (#6739) This PR aims to implement the knock feature as proposed in https://github.com/matrix-org/matrix-doc/pull/2403 Signed-off-by: Sorunome mail@sorunome.de Signed-off-by: Andrew Morgan andrewm@element.io --- tests/rest/client/v2_alpha/test_sync.py | 95 ++++++++++++++++++++++++++++++++- 1 file changed, 93 insertions(+), 2 deletions(-) (limited to 'tests/rest/client/v2_alpha/test_sync.py') diff --git a/tests/rest/client/v2_alpha/test_sync.py b/tests/rest/client/v2_alpha/test_sync.py index dbcbdf159a..be5737e420 100644 --- a/tests/rest/client/v2_alpha/test_sync.py +++ b/tests/rest/client/v2_alpha/test_sync.py @@ -17,10 +17,14 @@ import json import synapse.rest.admin from synapse.api.constants import EventContentFields, EventTypes, RelationTypes from synapse.rest.client.v1 import login, room -from synapse.rest.client.v2_alpha import read_marker, sync +from synapse.rest.client.v2_alpha import knock, read_marker, sync from tests import unittest +from tests.federation.transport.test_knocking import ( + KnockingStrippedStateEventHelperMixin, +) from tests.server import TimedOutException +from tests.unittest import override_config class FilterTestCase(unittest.HomeserverTestCase): @@ -305,6 +309,93 @@ class SyncTypingTests(unittest.HomeserverTestCase): self.make_request("GET", sync_url % (access_token, next_batch)) +class SyncKnockTestCase( + unittest.HomeserverTestCase, KnockingStrippedStateEventHelperMixin +): + servlets = [ + synapse.rest.admin.register_servlets, + login.register_servlets, + room.register_servlets, + sync.register_servlets, + knock.register_servlets, + ] + + def prepare(self, reactor, clock, hs): + self.store = hs.get_datastore() + self.url = "/sync?since=%s" + self.next_batch = "s0" + + # Register the first user (used to create the room to knock on). + self.user_id = self.register_user("kermit", "monkey") + self.tok = self.login("kermit", "monkey") + + # Create the room we'll knock on. + self.room_id = self.helper.create_room_as( + self.user_id, + is_public=False, + room_version="xyz.amorgan.knock", + tok=self.tok, + ) + + # Register the second user (used to knock on the room). + self.knocker = self.register_user("knocker", "monkey") + self.knocker_tok = self.login("knocker", "monkey") + + # Perform an initial sync for the knocking user. + channel = self.make_request( + "GET", + self.url % self.next_batch, + access_token=self.tok, + ) + self.assertEqual(channel.code, 200, channel.json_body) + + # Store the next batch for the next request. + self.next_batch = channel.json_body["next_batch"] + + # Set up some room state to test with. + self.expected_room_state = self.send_example_state_events_to_room( + hs, self.room_id, self.user_id + ) + + @override_config({"experimental_features": {"msc2403_enabled": True}}) + def test_knock_room_state(self): + """Tests that /sync returns state from a room after knocking on it.""" + # Knock on a room + channel = self.make_request( + "POST", + "/_matrix/client/unstable/xyz.amorgan.knock/%s" % (self.room_id,), + b"{}", + self.knocker_tok, + ) + self.assertEquals(200, channel.code, channel.result) + + # We expect to see the knock event in the stripped room state later + self.expected_room_state[EventTypes.Member] = { + "content": {"membership": "xyz.amorgan.knock", "displayname": "knocker"}, + "state_key": "@knocker:test", + } + + # Check that /sync includes stripped state from the room + channel = self.make_request( + "GET", + self.url % self.next_batch, + access_token=self.knocker_tok, + ) + self.assertEqual(channel.code, 200, channel.json_body) + + # Extract the stripped room state events from /sync + knock_entry = channel.json_body["rooms"]["xyz.amorgan.knock"] + room_state_events = knock_entry[self.room_id]["knock_state"]["events"] + + # Validate that the knock membership event came last + self.assertEqual(room_state_events[-1]["type"], EventTypes.Member) + + # Validate the stripped room state events + self.check_knock_room_state_against_room_state( + room_state_events, self.expected_room_state + ) + + class UnreadMessagesTestCase(unittest.HomeserverTestCase): servlets = [ synapse.rest.admin.register_servlets, @@ -447,7 +538,7 @@ class UnreadMessagesTestCase(unittest.HomeserverTestCase): ) self._check_unread_count(5) - def _check_unread_count(self, expected_count: True): + def _check_unread_count(self, expected_count: int): """Syncs and compares the unread count with the expected value.""" channel = self.make_request( -- cgit 1.5.1 From 9e5ab6dd581389271b817d256e2fca113614a080 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 15 Jun 2021 07:45:14 -0400 Subject: Remove the experimental flag for knocking and use stable prefixes / endpoints. (#10167) * Room version 7 for knocking. * Stable prefixes and endpoints (both client and federation) for knocking. * Removes the experimental configuration flag. --- changelog.d/10167.feature | 1 + synapse/api/constants.py | 4 ++-- synapse/api/room_versions.py | 7 ++++--- synapse/config/experimental.py | 7 ------- synapse/federation/federation_client.py | 9 ++------- synapse/federation/transport/client.py | 27 +++------------------------ synapse/federation/transport/server.py | 22 ++-------------------- synapse/handlers/federation.py | 6 ++---- synapse/handlers/room_member.py | 5 +---- synapse/rest/__init__.py | 5 +---- synapse/rest/client/v2_alpha/knock.py | 6 ++---- tests/federation/transport/test_knocking.py | 22 +++++++++------------- tests/rest/client/v2_alpha/test_sync.py | 8 ++++---- 13 files changed, 33 insertions(+), 96 deletions(-) create mode 100644 changelog.d/10167.feature (limited to 'tests/rest/client/v2_alpha/test_sync.py') diff --git a/changelog.d/10167.feature b/changelog.d/10167.feature new file mode 100644 index 0000000000..9c41140194 --- /dev/null +++ b/changelog.d/10167.feature @@ -0,0 +1 @@ +Implement "room knocking" as per [MSC2403](https://github.com/matrix-org/matrix-doc/pull/2403). Contributed by Sorunome and anoa. \ No newline at end of file diff --git a/synapse/api/constants.py b/synapse/api/constants.py index 8d5b2177d2..3940da5c88 100644 --- a/synapse/api/constants.py +++ b/synapse/api/constants.py @@ -41,7 +41,7 @@ class Membership: INVITE = "invite" JOIN = "join" - KNOCK = "xyz.amorgan.knock" + KNOCK = "knock" LEAVE = "leave" BAN = "ban" LIST = (INVITE, JOIN, KNOCK, LEAVE, BAN) @@ -58,7 +58,7 @@ class PresenceState: class JoinRules: PUBLIC = "public" - KNOCK = "xyz.amorgan.knock" + KNOCK = "knock" INVITE = "invite" PRIVATE = "private" # As defined for MSC3083. diff --git a/synapse/api/room_versions.py b/synapse/api/room_versions.py index 3349f399ba..f6c1c97b40 100644 --- a/synapse/api/room_versions.py +++ b/synapse/api/room_versions.py @@ -180,9 +180,9 @@ class RoomVersions: msc3083_join_rules=True, msc2403_knocking=False, ) - MSC2403 = RoomVersion( - "xyz.amorgan.knock", - RoomDisposition.UNSTABLE, + V7 = RoomVersion( + "7", + RoomDisposition.STABLE, EventFormatVersions.V3, StateResolutionVersions.V2, enforce_key_validity=True, @@ -206,6 +206,7 @@ KNOWN_ROOM_VERSIONS = { RoomVersions.V6, RoomVersions.MSC2176, RoomVersions.MSC3083, + RoomVersions.V7, ) # Note that we do not include MSC2043 here unless it is enabled in the config. } # type: Dict[str, RoomVersion] diff --git a/synapse/config/experimental.py b/synapse/config/experimental.py index 37668079e7..6ebce4b2f7 100644 --- a/synapse/config/experimental.py +++ b/synapse/config/experimental.py @@ -12,7 +12,6 @@ # See the License for the specific language governing permissions and # limitations under the License. -from synapse.api.room_versions import KNOWN_ROOM_VERSIONS, RoomVersions from synapse.config._base import Config from synapse.types import JsonDict @@ -30,9 +29,3 @@ class ExperimentalConfig(Config): # MSC3026 (busy presence state) self.msc3026_enabled = experimental.get("msc3026_enabled", False) # type: bool - - # MSC2403 (room knocking) - self.msc2403_enabled = experimental.get("msc2403_enabled", False) # type: bool - if self.msc2403_enabled: - # Enable the MSC2403 unstable room version - KNOWN_ROOM_VERSIONS[RoomVersions.MSC2403.identifier] = RoomVersions.MSC2403 diff --git a/synapse/federation/federation_client.py b/synapse/federation/federation_client.py index 03ec14ce87..ed09c6af1f 100644 --- a/synapse/federation/federation_client.py +++ b/synapse/federation/federation_client.py @@ -90,7 +90,6 @@ class FederationClient(FederationBase): self._clock.looping_call(self._clear_tried_cache, 60 * 1000) self.state = hs.get_state_handler() self.transport_layer = hs.get_federation_transport_client() - self._msc2403_enabled = hs.config.experimental.msc2403_enabled self.hostname = hs.hostname self.signing_key = hs.signing_key @@ -621,11 +620,7 @@ class FederationClient(FederationBase): SynapseError: if the chosen remote server returns a 300/400 code, or no servers successfully handle the request. """ - valid_memberships = {Membership.JOIN, Membership.LEAVE} - - # Allow knocking if the feature is enabled - if self._msc2403_enabled: - valid_memberships.add(Membership.KNOCK) + valid_memberships = {Membership.JOIN, Membership.LEAVE, Membership.KNOCK} if membership not in valid_memberships: raise RuntimeError( @@ -989,7 +984,7 @@ class FederationClient(FederationBase): return await self._do_send_knock(destination, pdu) return await self._try_destination_list( - "xyz.amorgan.knock/send_knock", destinations, send_request + "send_knock", destinations, send_request ) async def _do_send_knock(self, destination: str, pdu: EventBase) -> JsonDict: diff --git a/synapse/federation/transport/client.py b/synapse/federation/transport/client.py index af0c679ed9..c9e7c57461 100644 --- a/synapse/federation/transport/client.py +++ b/synapse/federation/transport/client.py @@ -47,7 +47,6 @@ class TransportLayerClient: def __init__(self, hs): self.server_name = hs.hostname self.client = hs.get_federation_http_client() - self._msc2403_enabled = hs.config.experimental.msc2403_enabled @log_function def get_room_state_ids(self, destination, room_id, event_id): @@ -221,29 +220,14 @@ class TransportLayerClient: Fails with ``FederationDeniedError`` if the remote destination is not in our federation whitelist """ - valid_memberships = {Membership.JOIN, Membership.LEAVE} - - # Allow knocking if the feature is enabled - if self._msc2403_enabled: - valid_memberships.add(Membership.KNOCK) + valid_memberships = {Membership.JOIN, Membership.LEAVE, Membership.KNOCK} if membership not in valid_memberships: raise RuntimeError( "make_membership_event called with membership='%s', must be one of %s" % (membership, ",".join(valid_memberships)) ) - - # Knock currently uses an unstable prefix - if membership == Membership.KNOCK: - # Create a path in the form of /unstable/xyz.amorgan.knock/make_knock/... - path = _create_path( - FEDERATION_UNSTABLE_PREFIX + "/xyz.amorgan.knock", - "/make_knock/%s/%s", - room_id, - user_id, - ) - else: - path = _create_v1_path("/make_%s/%s/%s", membership, room_id, user_id) + path = _create_v1_path("/make_%s/%s/%s", membership, room_id, user_id) ignore_backoff = False retry_on_dns_fail = False @@ -366,12 +350,7 @@ class TransportLayerClient: The list of state events may be empty. """ - path = _create_path( - FEDERATION_UNSTABLE_PREFIX + "/xyz.amorgan.knock", - "/send_knock/%s/%s", - room_id, - event_id, - ) + path = _create_v1_path("/send_knock/%s/%s", room_id, event_id) return await self.client.put_json( destination=destination, path=path, data=content diff --git a/synapse/federation/transport/server.py b/synapse/federation/transport/server.py index fe5fb6bee7..16d740cf58 100644 --- a/synapse/federation/transport/server.py +++ b/synapse/federation/transport/server.py @@ -567,8 +567,6 @@ class FederationV2SendLeaveServlet(BaseFederationServerServlet): class FederationMakeKnockServlet(BaseFederationServerServlet): PATH = "/make_knock/(?P[^/]*)/(?P[^/]*)" - PREFIX = FEDERATION_UNSTABLE_PREFIX + "/xyz.amorgan.knock" - async def on_GET(self, origin, content, query, room_id, user_id): try: # Retrieve the room versions the remote homeserver claims to support @@ -585,8 +583,6 @@ class FederationMakeKnockServlet(BaseFederationServerServlet): class FederationV1SendKnockServlet(BaseFederationServerServlet): PATH = "/send_knock/(?P[^/]*)/(?P[^/]*)" - PREFIX = FEDERATION_UNSTABLE_PREFIX + "/xyz.amorgan.knock" - async def on_PUT(self, origin, content, query, room_id, event_id): content = await self.handler.on_send_knock_request(origin, content, room_id) return 200, content @@ -1610,6 +1606,8 @@ FEDERATION_SERVLET_CLASSES = ( FederationVersionServlet, RoomComplexityServlet, FederationSpaceSummaryServlet, + FederationV1SendKnockServlet, + FederationMakeKnockServlet, ) # type: Tuple[Type[BaseFederationServlet], ...] OPENID_SERVLET_CLASSES = ( @@ -1652,12 +1650,6 @@ GROUP_ATTESTATION_SERVLET_CLASSES = ( ) # type: Tuple[Type[BaseFederationServlet], ...] -MSC2403_SERVLET_CLASSES = ( - FederationV1SendKnockServlet, - FederationMakeKnockServlet, -) - - DEFAULT_SERVLET_GROUPS = ( "federation", "room_list", @@ -1700,16 +1692,6 @@ def register_servlets( server_name=hs.hostname, ).register(resource) - # Register msc2403 (knocking) servlets if the feature is enabled - if hs.config.experimental.msc2403_enabled: - for servletclass in MSC2403_SERVLET_CLASSES: - servletclass( - hs=hs, - authenticator=authenticator, - ratelimiter=ratelimiter, - server_name=hs.hostname, - ).register(resource) - if "openid" in servlet_groups: for servletclass in OPENID_SERVLET_CLASSES: servletclass( diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 6647063485..b3a93212f1 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -2009,8 +2009,7 @@ class FederationHandler(BaseHandler): """ if get_domain_from_id(user_id) != origin: logger.info( - "Get /xyz.amorgan.knock/make_knock request for user %r" - "from different origin %s, ignoring", + "Get /make_knock request for user %r from different origin %s, ignoring", user_id, origin, ) @@ -2077,8 +2076,7 @@ class FederationHandler(BaseHandler): if get_domain_from_id(event.sender) != origin: logger.info( - "Got /xyz.amorgan.knock/send_knock request for user %r " - "from different origin %s", + "Got /send_knock request for user %r from different origin %s", event.sender, origin, ) diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index c26963b1e1..a49a61a34c 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -707,10 +707,7 @@ class RoomMemberHandler(metaclass=abc.ABCMeta): knock.event_id, txn_id, requester, content ) - elif ( - self.config.experimental.msc2403_enabled - and effective_membership_state == Membership.KNOCK - ): + elif effective_membership_state == Membership.KNOCK: if not is_host_in_room: # The knock needs to be sent over federation instead remote_room_hosts.append(get_domain_from_id(room_id)) diff --git a/synapse/rest/__init__.py b/synapse/rest/__init__.py index 138411ad19..d29f2fea5e 100644 --- a/synapse/rest/__init__.py +++ b/synapse/rest/__init__.py @@ -121,10 +121,7 @@ class ClientRestResource(JsonResource): account_validity.register_servlets(hs, client_resource) relations.register_servlets(hs, client_resource) password_policy.register_servlets(hs, client_resource) - - # Register msc2403 (knocking) servlets if the feature is enabled - if hs.config.experimental.msc2403_enabled: - knock.register_servlets(hs, client_resource) + knock.register_servlets(hs, client_resource) # moving to /_synapse/admin admin.register_servlets_for_client_rest_resource(hs, client_resource) diff --git a/synapse/rest/client/v2_alpha/knock.py b/synapse/rest/client/v2_alpha/knock.py index f046bf9cb3..7d1bc40658 100644 --- a/synapse/rest/client/v2_alpha/knock.py +++ b/synapse/rest/client/v2_alpha/knock.py @@ -39,12 +39,10 @@ logger = logging.getLogger(__name__) class KnockRoomAliasServlet(RestServlet): """ - POST /xyz.amorgan.knock/{roomIdOrAlias} + POST /knock/{roomIdOrAlias} """ - PATTERNS = client_patterns( - "/xyz.amorgan.knock/(?P[^/]*)", releases=() - ) + PATTERNS = client_patterns("/knock/(?P[^/]*)") def __init__(self, hs: "HomeServer"): super().__init__() diff --git a/tests/federation/transport/test_knocking.py b/tests/federation/transport/test_knocking.py index 121aa88cfa..8c215d50f2 100644 --- a/tests/federation/transport/test_knocking.py +++ b/tests/federation/transport/test_knocking.py @@ -25,9 +25,6 @@ from synapse.types import RoomAlias from tests.test_utils import event_injection from tests.unittest import FederatingHomeserverTestCase, TestCase, override_config -# An identifier to use while MSC2304 is not in a stable release of the spec -KNOCK_UNSTABLE_IDENTIFIER = "xyz.amorgan.knock" - class KnockingStrippedStateEventHelperMixin(TestCase): def send_example_state_events_to_room( @@ -61,7 +58,7 @@ class KnockingStrippedStateEventHelperMixin(TestCase): self.get_success( event_injection.inject_event( hs, - room_version=RoomVersions.MSC2403.identifier, + room_version=RoomVersions.V7.identifier, room_id=room_id, sender=sender, type="com.example.secret", @@ -121,7 +118,7 @@ class KnockingStrippedStateEventHelperMixin(TestCase): self.get_success( event_injection.inject_event( hs, - room_version=RoomVersions.MSC2403.identifier, + room_version=RoomVersions.V7.identifier, room_id=room_id, sender=sender, type=event_type, @@ -135,7 +132,7 @@ class KnockingStrippedStateEventHelperMixin(TestCase): room_state[EventTypes.Create] = { "content": { "creator": sender, - "room_version": RoomVersions.MSC2403.identifier, + "room_version": RoomVersions.V7.identifier, }, "state_key": "", } @@ -232,7 +229,7 @@ class FederationKnockingTestCase( room_id = self.helper.create_room_as( "u1", is_public=False, - room_version=RoomVersions.MSC2403.identifier, + room_version=RoomVersions.V7.identifier, tok=user_token, ) @@ -243,14 +240,13 @@ class FederationKnockingTestCase( channel = self.make_request( "GET", - "/_matrix/federation/unstable/%s/make_knock/%s/%s?ver=%s" + "/_matrix/federation/v1/make_knock/%s/%s?ver=%s" % ( - KNOCK_UNSTABLE_IDENTIFIER, room_id, fake_knocking_user_id, # Inform the remote that we support the room version of the room we're # knocking on - RoomVersions.MSC2403.identifier, + RoomVersions.V7.identifier, ), ) self.assertEquals(200, channel.code, channel.result) @@ -275,7 +271,7 @@ class FederationKnockingTestCase( self.clock, self.hs.hostname, self.hs.signing_key, - room_version=RoomVersions.MSC2403, + room_version=RoomVersions.V7, event_dict=knock_event, ) @@ -287,8 +283,8 @@ class FederationKnockingTestCase( # Send the signed knock event into the room channel = self.make_request( "PUT", - "/_matrix/federation/unstable/%s/send_knock/%s/%s" - % (KNOCK_UNSTABLE_IDENTIFIER, room_id, signed_knock_event.event_id), + "/_matrix/federation/v1/send_knock/%s/%s" + % (room_id, signed_knock_event.event_id), signed_knock_event_json, ) self.assertEquals(200, channel.code, channel.result) diff --git a/tests/rest/client/v2_alpha/test_sync.py b/tests/rest/client/v2_alpha/test_sync.py index be5737e420..b52f78ba69 100644 --- a/tests/rest/client/v2_alpha/test_sync.py +++ b/tests/rest/client/v2_alpha/test_sync.py @@ -333,7 +333,7 @@ class SyncKnockTestCase( self.room_id = self.helper.create_room_as( self.user_id, is_public=False, - room_version="xyz.amorgan.knock", + room_version="7", tok=self.tok, ) @@ -363,7 +363,7 @@ class SyncKnockTestCase( # Knock on a room channel = self.make_request( "POST", - "/_matrix/client/unstable/xyz.amorgan.knock/%s" % (self.room_id,), + "/_matrix/client/r0/knock/%s" % (self.room_id,), b"{}", self.knocker_tok, ) @@ -371,7 +371,7 @@ class SyncKnockTestCase( # We expect to see the knock event in the stripped room state later self.expected_room_state[EventTypes.Member] = { - "content": {"membership": "xyz.amorgan.knock", "displayname": "knocker"}, + "content": {"membership": "knock", "displayname": "knocker"}, "state_key": "@knocker:test", } @@ -384,7 +384,7 @@ class SyncKnockTestCase( self.assertEqual(channel.code, 200, channel.json_body) # Extract the stripped room state events from /sync - knock_entry = channel.json_body["rooms"]["xyz.amorgan.knock"] + knock_entry = channel.json_body["rooms"]["knock"] room_state_events = knock_entry[self.room_id]["knock_state"]["events"] # Validate that the knock membership event came last -- cgit 1.5.1 From fcf3c7032b96ab454120f86f3f070160c409d599 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Thu, 17 Jun 2021 16:23:11 +0100 Subject: Ensure that we do not cache empty sync responses after a timeout (#10158) Fixes #8518 by telling the ResponseCache not to cache the /sync response if the next_batch param is the same as the since token. --- changelog.d/10157.bugfix | 1 + changelog.d/10157.misc | 1 - changelog.d/10158.bugfix | 1 + synapse/handlers/sync.py | 36 +++++++++++++++++------- synapse/python_dependencies.py | 6 ++-- synapse/types.py | 2 +- tests/rest/client/v2_alpha/test_sync.py | 50 +++++++++++++++++++++++++++++++++ tests/server.py | 8 ++---- 8 files changed, 84 insertions(+), 21 deletions(-) create mode 100644 changelog.d/10157.bugfix delete mode 100644 changelog.d/10157.misc create mode 100644 changelog.d/10158.bugfix (limited to 'tests/rest/client/v2_alpha/test_sync.py') diff --git a/changelog.d/10157.bugfix b/changelog.d/10157.bugfix new file mode 100644 index 0000000000..6eaaa05b80 --- /dev/null +++ b/changelog.d/10157.bugfix @@ -0,0 +1 @@ +Fix a bug introduced in v1.21.0 which could cause `/sync` to return immediately with an empty response. diff --git a/changelog.d/10157.misc b/changelog.d/10157.misc deleted file mode 100644 index 6c1d0e6e59..0000000000 --- a/changelog.d/10157.misc +++ /dev/null @@ -1 +0,0 @@ -Extend `ResponseCache` to pass a context object into the callback. diff --git a/changelog.d/10158.bugfix b/changelog.d/10158.bugfix new file mode 100644 index 0000000000..6eaaa05b80 --- /dev/null +++ b/changelog.d/10158.bugfix @@ -0,0 +1 @@ +Fix a bug introduced in v1.21.0 which could cause `/sync` to return immediately with an empty response. diff --git a/synapse/handlers/sync.py b/synapse/handlers/sync.py index 7f2138d804..b9a0361059 100644 --- a/synapse/handlers/sync.py +++ b/synapse/handlers/sync.py @@ -49,7 +49,7 @@ from synapse.types import ( from synapse.util.async_helpers import concurrently_execute from synapse.util.caches.expiringcache import ExpiringCache from synapse.util.caches.lrucache import LruCache -from synapse.util.caches.response_cache import ResponseCache +from synapse.util.caches.response_cache import ResponseCache, ResponseCacheContext from synapse.util.metrics import Measure, measure_func from synapse.visibility import filter_events_for_client @@ -83,12 +83,15 @@ LAZY_LOADED_MEMBERS_CACHE_MAX_AGE = 30 * 60 * 1000 LAZY_LOADED_MEMBERS_CACHE_MAX_SIZE = 100 +SyncRequestKey = Tuple[Any, ...] + + @attr.s(slots=True, frozen=True) class SyncConfig: user = attr.ib(type=UserID) filter_collection = attr.ib(type=FilterCollection) is_guest = attr.ib(type=bool) - request_key = attr.ib(type=Tuple[Any, ...]) + request_key = attr.ib(type=SyncRequestKey) device_id = attr.ib(type=Optional[str]) @@ -266,9 +269,9 @@ class SyncHandler: self.presence_handler = hs.get_presence_handler() self.event_sources = hs.get_event_sources() self.clock = hs.get_clock() - self.response_cache = ResponseCache( + self.response_cache: ResponseCache[SyncRequestKey] = ResponseCache( hs.get_clock(), "sync" - ) # type: ResponseCache[Tuple[Any, ...]] + ) self.state = hs.get_state_handler() self.auth = hs.get_auth() self.storage = hs.get_storage() @@ -307,6 +310,7 @@ class SyncHandler: since_token, timeout, full_state, + cache_context=True, ) logger.debug("Returning sync response for %s", user_id) return res @@ -314,9 +318,10 @@ class SyncHandler: async def _wait_for_sync_for_user( self, sync_config: SyncConfig, - since_token: Optional[StreamToken] = None, - timeout: int = 0, - full_state: bool = False, + since_token: Optional[StreamToken], + timeout: int, + full_state: bool, + cache_context: ResponseCacheContext[SyncRequestKey], ) -> SyncResult: if since_token is None: sync_type = "initial_sync" @@ -343,13 +348,13 @@ class SyncHandler: if timeout == 0 or since_token is None or full_state: # we are going to return immediately, so don't bother calling # notifier.wait_for_events. - result = await self.current_sync_for_user( + result: SyncResult = await self.current_sync_for_user( sync_config, since_token, full_state=full_state ) else: - def current_sync_callback(before_token, after_token): - return self.current_sync_for_user(sync_config, since_token) + async def current_sync_callback(before_token, after_token) -> SyncResult: + return await self.current_sync_for_user(sync_config, since_token) result = await self.notifier.wait_for_events( sync_config.user.to_string(), @@ -358,6 +363,17 @@ class SyncHandler: from_token=since_token, ) + # if nothing has happened in any of the users' rooms since /sync was called, + # the resultant next_batch will be the same as since_token (since the result + # is generated when wait_for_events is first called, and not regenerated + # when wait_for_events times out). + # + # If that happens, we mustn't cache it, so that when the client comes back + # with the same cache token, we don't immediately return the same empty + # result, causing a tightloop. (#8518) + if result.next_batch == since_token: + cache_context.should_cache = False + if result: if sync_config.filter_collection.lazy_load_members(): lazy_loaded = "true" diff --git a/synapse/python_dependencies.py b/synapse/python_dependencies.py index 546231bec0..bf361c42d6 100644 --- a/synapse/python_dependencies.py +++ b/synapse/python_dependencies.py @@ -75,11 +75,9 @@ REQUIREMENTS = [ "phonenumbers>=8.2.0", # we use GaugeHistogramMetric, which was added in prom-client 0.4.0. "prometheus_client>=0.4.0", - # we use attr.validators.deep_iterable, which arrived in 19.1.0 (Note: - # Fedora 31 only has 19.1, so if we want to upgrade we should wait until 33 - # is out in November.) + # we use `order`, which arrived in attrs 19.2.0. # Note: 21.1.0 broke `/sync`, see #9936 - "attrs>=19.1.0,!=21.1.0", + "attrs>=19.2.0,!=21.1.0", "netaddr>=0.7.18", "Jinja2>=2.9", "bleach>=1.4.3", diff --git a/synapse/types.py b/synapse/types.py index 0bdf32659c..8d2fa00f71 100644 --- a/synapse/types.py +++ b/synapse/types.py @@ -404,7 +404,7 @@ def map_username_to_mxid_localpart( return username.decode("ascii") -@attr.s(frozen=True, slots=True, cmp=False) +@attr.s(frozen=True, slots=True, order=False) class RoomStreamToken: """Tokens are positions between events. The token "s1" comes after event 1. diff --git a/tests/rest/client/v2_alpha/test_sync.py b/tests/rest/client/v2_alpha/test_sync.py index b52f78ba69..012910f136 100644 --- a/tests/rest/client/v2_alpha/test_sync.py +++ b/tests/rest/client/v2_alpha/test_sync.py @@ -558,3 +558,53 @@ class UnreadMessagesTestCase(unittest.HomeserverTestCase): # Store the next batch for the next request. self.next_batch = channel.json_body["next_batch"] + + +class SyncCacheTestCase(unittest.HomeserverTestCase): + servlets = [ + synapse.rest.admin.register_servlets, + login.register_servlets, + sync.register_servlets, + ] + + def test_noop_sync_does_not_tightloop(self): + """If the sync times out, we shouldn't cache the result + + Essentially a regression test for #8518. + """ + self.user_id = self.register_user("kermit", "monkey") + self.tok = self.login("kermit", "monkey") + + # we should immediately get an initial sync response + channel = self.make_request("GET", "/sync", access_token=self.tok) + self.assertEqual(channel.code, 200, channel.json_body) + + # now, make an incremental sync request, with a timeout + next_batch = channel.json_body["next_batch"] + channel = self.make_request( + "GET", + f"/sync?since={next_batch}&timeout=10000", + access_token=self.tok, + await_result=False, + ) + # that should block for 10 seconds + with self.assertRaises(TimedOutException): + channel.await_result(timeout_ms=9900) + channel.await_result(timeout_ms=200) + self.assertEqual(channel.code, 200, channel.json_body) + + # we expect the next_batch in the result to be the same as before + self.assertEqual(channel.json_body["next_batch"], next_batch) + + # another incremental sync should also block. + channel = self.make_request( + "GET", + f"/sync?since={next_batch}&timeout=10000", + access_token=self.tok, + await_result=False, + ) + # that should block for 10 seconds + with self.assertRaises(TimedOutException): + channel.await_result(timeout_ms=9900) + channel.await_result(timeout_ms=200) + self.assertEqual(channel.code, 200, channel.json_body) diff --git a/tests/server.py b/tests/server.py index 9df8cda24f..f32d8dc375 100644 --- a/tests/server.py +++ b/tests/server.py @@ -138,21 +138,19 @@ class FakeChannel: def transport(self): return self - def await_result(self, timeout: int = 100) -> None: + def await_result(self, timeout_ms: int = 1000) -> None: """ Wait until the request is finished. """ + end_time = self._reactor.seconds() + timeout_ms / 1000.0 self._reactor.run() - x = 0 while not self.is_finished(): # If there's a producer, tell it to resume producing so we get content if self._producer: self._producer.resumeProducing() - x += 1 - - if x > timeout: + if self._reactor.seconds() > end_time: raise TimedOutException("Timed out waiting for request to finish.") self._reactor.advance(0.1) -- cgit 1.5.1