Remove unnecessary reactor reference from _PerHostRatelimiter (#14842)

Fix up #14812 to avoid introducing a reference to the reactor.

Signed-off-by: Sean Quah <[email protected]>

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.

synapse/rest/client/register.py

@@ -310,7 +310,6 @@ def __init__(self, hs: "HomeServer"):
         self.hs = hs
         self.registration_handler = hs.get_registration_handler()
         self.ratelimiter = FederationRateLimiter(
-            hs.get_reactor(),
             hs.get_clock(),
             FederationRatelimitSettings(
                 # Time window of 2s

synapse/server.py

@@ -768,7 +768,6 @@ def get_replication_streams(self) -> Dict[str, Stream]:
     @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",

synapse/util/ratelimitutils.py

@@ -34,7 +34,6 @@
 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 @@ def __init__(
 
         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,22 +194,19 @@ def ratelimit(self, host: str) -> "_GeneratorContextManager[defer.Deferred[None]
 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
                 from the rest in the metrics. If `None`, we don't track metrics
                 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 @@ def start_next_request() -> None:
             except KeyError:
                 pass
 
-        self.reactor.callLater(0.0, start_next_request)
+        self.clock.call_later(0.0, start_next_request)

tests/util/test_ratelimitutils.py

@@ -30,7 +30,7 @@ def test_ratelimit(self) -> None:
         """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 @@ def test_concurrent_limit(self) -> None:
         """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 @@ def test_sleep_limit(self) -> None:
         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 @@ def test_lots_of_queued_things(self) -> None:
                 }
             }
         )
-        ratelimiter = FederationRateLimiter(reactor, clock, rc_config)
+        ratelimiter = FederationRateLimiter(clock, rc_config)
 
         with ratelimiter.ratelimit("testhost") as d:
             # shouldn't block