Skip to content

Commit

Permalink
Add a short sleep if the request is rate-limited (#17210)
Browse files Browse the repository at this point in the history
This helps prevent clients from "tight-looping" retrying their request.
  • Loading branch information
erikjohnston committed May 18, 2024
1 parent 38f03a0 commit 52af16c
Show file tree
Hide file tree
Showing 6 changed files with 15 additions and 4 deletions.
1 change: 1 addition & 0 deletions changelog.d/17210.misc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add a short pause when rate-limiting a request.
4 changes: 4 additions & 0 deletions synapse/api/ratelimiting.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)),
Expand Down
5 changes: 3 additions & 2 deletions tests/api/test_ratelimiting.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)

Expand Down
1 change: 1 addition & 0 deletions tests/handlers/test_federation.py
Original file line number Diff line number Diff line change
Expand Up @@ -483,6 +483,7 @@ def create_invite() -> EventBase:
event.room_version,
),
exc=LimitExceededError,
by=0.5,
)

def _build_and_send_join_event(
Expand Down
4 changes: 4 additions & 0 deletions tests/handlers/test_room_member.py
Original file line number Diff line number Diff line change
Expand Up @@ -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}})
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -273,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
Expand All @@ -285,6 +288,7 @@ def test_local_users_joining_on_another_worker_contribute_to_rate_limit(
action=Membership.JOIN,
),
LimitExceededError,
by=0.5,
)


Expand Down
4 changes: 2 additions & 2 deletions tests/unittest.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down

0 comments on commit 52af16c

Please sign in to comment.