From a302d3ecf75493f84fc5be616fee7d199ed12394 Mon Sep 17 00:00:00 2001 From: Sean Quah <8349537+squahtx@users.noreply.github.com> Date: Mon, 16 Jan 2023 13:16:19 +0000 Subject: [PATCH] Remove unnecessary reactor reference from `_PerHostRatelimiter` (#14842) Fix up #14812 to avoid introducing a reference to the reactor. Signed-off-by: Sean Quah --- changelog.d/14842.bugfix | 1 + synapse/rest/client/register.py | 1 - synapse/server.py | 1 - synapse/util/ratelimitutils.py | 10 ++-------- tests/util/test_ratelimitutils.py | 8 ++++---- 5 files changed, 7 insertions(+), 14 deletions(-) create mode 100644 changelog.d/14842.bugfix diff --git a/changelog.d/14842.bugfix b/changelog.d/14842.bugfix new file mode 100644 index 000000000..94e0d70cb --- /dev/null +++ b/changelog.d/14842.bugfix @@ -0,0 +1 @@ +Fix a long-standing bug where Synapse would exhaust the stack when processing many federation requests where the remote homeserver has disconencted early. diff --git a/synapse/rest/client/register.py b/synapse/rest/client/register.py index be696c304..3cb1e7e37 100644 --- a/synapse/rest/client/register.py +++ b/synapse/rest/client/register.py @@ -310,7 +310,6 @@ class UsernameAvailabilityRestServlet(RestServlet): self.hs = hs self.registration_handler = hs.get_registration_handler() self.ratelimiter = FederationRateLimiter( - hs.get_reactor(), hs.get_clock(), FederationRatelimitSettings( # Time window of 2s diff --git a/synapse/server.py b/synapse/server.py index c8752baa5..f4ab94c4f 100644 --- a/synapse/server.py +++ b/synapse/server.py @@ -768,7 +768,6 @@ class HomeServer(metaclass=abc.ABCMeta): @cache_in_self def get_federation_ratelimiter(self) -> FederationRateLimiter: return FederationRateLimiter( - self.get_reactor(), self.get_clock(), config=self.config.ratelimiting.rc_federation, metrics_name="federation_servlets", diff --git a/synapse/util/ratelimitutils.py b/synapse/util/ratelimitutils.py index bd72947bf..f262bf95a 100644 --- a/synapse/util/ratelimitutils.py +++ b/synapse/util/ratelimitutils.py @@ -34,7 +34,6 @@ from prometheus_client.core import Counter from typing_extensions import ContextManager from twisted.internet import defer -from twisted.internet.interfaces import IReactorTime from synapse.api.errors import LimitExceededError from synapse.config.ratelimiting import FederationRatelimitSettings @@ -147,14 +146,12 @@ class FederationRateLimiter: def __init__( self, - reactor: IReactorTime, clock: Clock, config: FederationRatelimitSettings, metrics_name: Optional[str] = None, ): """ Args: - reactor clock config metrics_name: The name of the rate limiter so we can differentiate it @@ -166,7 +163,7 @@ class FederationRateLimiter: def new_limiter() -> "_PerHostRatelimiter": return _PerHostRatelimiter( - reactor=reactor, clock=clock, config=config, metrics_name=metrics_name + clock=clock, config=config, metrics_name=metrics_name ) self.ratelimiters: DefaultDict[ @@ -197,14 +194,12 @@ class FederationRateLimiter: class _PerHostRatelimiter: def __init__( self, - reactor: IReactorTime, clock: Clock, config: FederationRatelimitSettings, metrics_name: Optional[str] = None, ): """ Args: - reactor clock config metrics_name: The name of the rate limiter so we can differentiate it @@ -212,7 +207,6 @@ class _PerHostRatelimiter: for this rate limiter. from the rest in the metrics """ - self.reactor = reactor self.clock = clock self.metrics_name = metrics_name @@ -388,4 +382,4 @@ class _PerHostRatelimiter: except KeyError: pass - self.reactor.callLater(0.0, start_next_request) + self.clock.call_later(0.0, start_next_request) diff --git a/tests/util/test_ratelimitutils.py b/tests/util/test_ratelimitutils.py index 2f3ea15b9..fe4961dcf 100644 --- a/tests/util/test_ratelimitutils.py +++ b/tests/util/test_ratelimitutils.py @@ -30,7 +30,7 @@ class FederationRateLimiterTestCase(TestCase): """A simple test with the default values""" reactor, clock = get_clock() rc_config = build_rc_config() - ratelimiter = FederationRateLimiter(reactor, clock, rc_config) + ratelimiter = FederationRateLimiter(clock, rc_config) with ratelimiter.ratelimit("testhost") as d1: # shouldn't block @@ -40,7 +40,7 @@ class FederationRateLimiterTestCase(TestCase): """Test what happens when we hit the concurrent limit""" reactor, clock = get_clock() rc_config = build_rc_config({"rc_federation": {"concurrent": 2}}) - ratelimiter = FederationRateLimiter(reactor, clock, rc_config) + ratelimiter = FederationRateLimiter(clock, rc_config) with ratelimiter.ratelimit("testhost") as d1: # shouldn't block @@ -67,7 +67,7 @@ class FederationRateLimiterTestCase(TestCase): rc_config = build_rc_config( {"rc_federation": {"sleep_limit": 2, "sleep_delay": 500}} ) - ratelimiter = FederationRateLimiter(reactor, clock, rc_config) + ratelimiter = FederationRateLimiter(clock, rc_config) with ratelimiter.ratelimit("testhost") as d1: # shouldn't block @@ -98,7 +98,7 @@ class FederationRateLimiterTestCase(TestCase): } } ) - ratelimiter = FederationRateLimiter(reactor, clock, rc_config) + ratelimiter = FederationRateLimiter(clock, rc_config) with ratelimiter.ratelimit("testhost") as d: # shouldn't block