From 5ca65bcbe539364b0d1767e84051ddb1745bf0b5 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 17 May 2024 09:47:57 +0100 Subject: [PATCH 1/4] Add a short sleep if the request is rate-limited This helps prevent clients from "tight-looping" retrying their request. --- synapse/api/ratelimiting.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/synapse/api/ratelimiting.py b/synapse/api/ratelimiting.py index a73626bc869..a99a9e09fcb 100644 --- a/synapse/api/ratelimiting.py +++ b/synapse/api/ratelimiting.py @@ -316,6 +316,10 @@ async def ratelimit( ) if not allowed: + # We pause for a bit here to stop clients from "tight-looping" on + # retrying their request. + await self.clock.sleep(0.5) + raise LimitExceededError( limiter_name=self._limiter_name, retry_after_ms=int(1000 * (time_allowed - time_now_s)), From 41187779aef8a5e446b359f484b99f1d736a27f0 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 17 May 2024 09:48:46 +0100 Subject: [PATCH 2/4] Newsfile --- changelog.d/17210.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/17210.misc diff --git a/changelog.d/17210.misc b/changelog.d/17210.misc new file mode 100644 index 00000000000..2059ebea7be --- /dev/null +++ b/changelog.d/17210.misc @@ -0,0 +1 @@ +Add a short pause when rate-limiting a request. From a1b3a7c06229ad60dcbe51e62629c84e7e41fc9c Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 17 May 2024 12:33:18 +0100 Subject: [PATCH 3/4] Fix tests --- tests/api/test_ratelimiting.py | 5 +++-- tests/handlers/test_federation.py | 1 + tests/handlers/test_room_member.py | 2 ++ tests/unittest.py | 4 ++-- 4 files changed, 8 insertions(+), 4 deletions(-) diff --git a/tests/api/test_ratelimiting.py b/tests/api/test_ratelimiting.py index a24638c9eff..a59e168db1c 100644 --- a/tests/api/test_ratelimiting.py +++ b/tests/api/test_ratelimiting.py @@ -116,8 +116,9 @@ def test_allowed_via_ratelimit(self) -> None: # Should raise with self.assertRaises(LimitExceededError) as context: self.get_success_or_raise( - limiter.ratelimit(None, key="test_id", _time_now_s=5) + limiter.ratelimit(None, key="test_id", _time_now_s=5), by=0.5 ) + self.assertEqual(context.exception.retry_after_ms, 5000) # Shouldn't raise @@ -192,7 +193,7 @@ def test_allowed_via_ratelimit_and_overriding_parameters(self) -> None: # Second attempt, 1s later, will fail with self.assertRaises(LimitExceededError) as context: self.get_success_or_raise( - limiter.ratelimit(None, key=("test_id",), _time_now_s=1) + limiter.ratelimit(None, key=("test_id",), _time_now_s=1), by=0.5 ) self.assertEqual(context.exception.retry_after_ms, 9000) diff --git a/tests/handlers/test_federation.py b/tests/handlers/test_federation.py index b819b60c5d8..3fe5b0a1b45 100644 --- a/tests/handlers/test_federation.py +++ b/tests/handlers/test_federation.py @@ -483,6 +483,7 @@ def create_invite() -> EventBase: event.room_version, ), exc=LimitExceededError, + by=0.5, ) def _build_and_send_join_event( diff --git a/tests/handlers/test_room_member.py b/tests/handlers/test_room_member.py index 3e28117e2c0..2f7ebef7fe3 100644 --- a/tests/handlers/test_room_member.py +++ b/tests/handlers/test_room_member.py @@ -70,6 +70,7 @@ def test_local_user_local_joins_contribute_to_limit_and_are_limited(self) -> Non action=Membership.JOIN, ), LimitExceededError, + by=0.5, ) @override_config({"rc_joins_per_room": {"per_second": 0, "burst_count": 2}}) @@ -206,6 +207,7 @@ def test_remote_joins_contribute_to_rate_limit(self) -> None: remote_room_hosts=[self.OTHER_SERVER_NAME], ), LimitExceededError, + by=0.5, ) # TODO: test that remote joins to a room are rate limited. diff --git a/tests/unittest.py b/tests/unittest.py index e6aad9ed40b..18963b9e32d 100644 --- a/tests/unittest.py +++ b/tests/unittest.py @@ -637,13 +637,13 @@ def get_success(self, d: Awaitable[TV], by: float = 0.0) -> TV: return self.successResultOf(deferred) def get_failure( - self, d: Awaitable[Any], exc: Type[_ExcType] + self, d: Awaitable[Any], exc: Type[_ExcType], by: float = 0.0 ) -> _TypedFailure[_ExcType]: """ Run a Deferred and get a Failure from it. The failure must be of the type `exc`. """ deferred: Deferred[Any] = ensureDeferred(d) # type: ignore[arg-type] - self.pump() + self.pump(by) return self.failureResultOf(deferred, exc) def get_success_or_raise(self, d: Awaitable[TV], by: float = 0.0) -> TV: From e4982add7a416928698f6b996425102371637c0b Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 17 May 2024 13:06:48 +0100 Subject: [PATCH 4/4] Fix tests take 2 --- tests/handlers/test_room_member.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/handlers/test_room_member.py b/tests/handlers/test_room_member.py index 2f7ebef7fe3..df43ce581ce 100644 --- a/tests/handlers/test_room_member.py +++ b/tests/handlers/test_room_member.py @@ -275,6 +275,7 @@ def test_local_users_joining_on_another_worker_contribute_to_rate_limit( action=Membership.JOIN, ), LimitExceededError, + by=0.5, ) # Try to join as Chris on the original worker. Should get denied because Alice @@ -287,6 +288,7 @@ def test_local_users_joining_on_another_worker_contribute_to_rate_limit( action=Membership.JOIN, ), LimitExceededError, + by=0.5, )