From ef0d3d7bd941b497ad8291c58bcc53700e08b999 Mon Sep 17 00:00:00 2001 From: Mathieu Velten Date: Wed, 14 Jun 2023 11:55:09 +0200 Subject: [PATCH] Revert "Allow for the configuration of max request retries and min/max retry delays in the matrix federation client (#12504)" This reverts commit d84e66144dc12dacf71c987a2ba802dd59c0b68e. --- CHANGES.md | 1 - .../configuration/config_documentation.md | 26 ------------------- synapse/config/federation.py | 10 ------- synapse/http/matrixfederationclient.py | 21 +++++++-------- tests/http/test_matrixfederationclient.py | 20 +------------- 5 files changed, 10 insertions(+), 68 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 5412581eef..d898593664 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -30,7 +30,6 @@ Improved Documentation Internal Changes ---------------- -- Allow for the configuration of max request retries and min/max retry delays in the matrix federation client. ([\#12504](https://github.com/matrix-org/synapse/issues/12504)) - Log when events are (maybe unexpectedly) filtered out of responses in tests. ([\#14213](https://github.com/matrix-org/synapse/issues/14213)) - Read from column `full_user_id` rather than `user_id` of tables `profiles` and `user_filters`. ([\#15649](https://github.com/matrix-org/synapse/issues/15649)) - Add support for tracing functions which return `Awaitable`s. ([\#15650](https://github.com/matrix-org/synapse/issues/15650)) diff --git a/docs/usage/configuration/config_documentation.md b/docs/usage/configuration/config_documentation.md index 8426de0417..0cf6e075ff 100644 --- a/docs/usage/configuration/config_documentation.md +++ b/docs/usage/configuration/config_documentation.md @@ -1196,32 +1196,6 @@ Example configuration: allow_device_name_lookup_over_federation: true ``` --- -### `federation` - -The federation section defines some sub-options related to federation. - -The following options are related to configuring timeout and retry logic for one request, -independently of the others. -Short retry algorithm is used when something or someone will wait for the request to have an -answer, while long retry is used for requests that happen in the background, -like sending a federation transaction. - -* `client_timeout`: timeout for the federation requests in seconds. Default to 60s. -* `max_short_retry_delay`: maximum delay to be used for the short retry algo in seconds. Default to 2s. -* `max_long_retry_delay`: maximum delay to be used for the short retry algo in seconds. Default to 60s. -* `max_short_retries`: maximum number of retries for the short retry algo. Default to 3 attempts. -* `max_long_retries`: maximum number of retries for the long retry algo. Default to 10 attempts. - -Example configuration: -```yaml -federation: - client_timeout: 180 - max_short_retry_delay: 7 - max_long_retry_delay: 100 - max_short_retries: 5 - max_long_retries: 20 -``` ---- ## Caching Options related to caching. diff --git a/synapse/config/federation.py b/synapse/config/federation.py index d21f7fd02a..336fca578a 100644 --- a/synapse/config/federation.py +++ b/synapse/config/federation.py @@ -22,8 +22,6 @@ class FederationConfig(Config): section = "federation" def read_config(self, config: JsonDict, **kwargs: Any) -> None: - federation_config = config.setdefault("federation", {}) - # FIXME: federation_domain_whitelist needs sytests self.federation_domain_whitelist: Optional[dict] = None federation_domain_whitelist = config.get("federation_domain_whitelist", None) @@ -51,13 +49,5 @@ class FederationConfig(Config): "allow_device_name_lookup_over_federation", False ) - # Allow for the configuration of timeout, max request retries - # and min/max retry delays in the matrix federation client. - self.client_timeout = federation_config.get("client_timeout", 60) - self.max_long_retry_delay = federation_config.get("max_long_retry_delay", 60) - self.max_short_retry_delay = federation_config.get("max_short_retry_delay", 2) - self.max_long_retries = federation_config.get("max_long_retries", 10) - self.max_short_retries = federation_config.get("max_short_retries", 3) - _METRICS_FOR_DOMAINS_SCHEMA = {"type": "array", "items": {"type": "string"}} diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index ed36825b67..abb5ae5815 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -95,6 +95,8 @@ incoming_responses_counter = Counter( ) +MAX_LONG_RETRIES = 10 +MAX_SHORT_RETRIES = 3 MAXINT = sys.maxsize @@ -404,12 +406,7 @@ class MatrixFederationHttpClient: self.clock = hs.get_clock() self._store = hs.get_datastores().main self.version_string_bytes = hs.version_string.encode("ascii") - self.default_timeout = hs.config.federation.client_timeout - - self.max_long_retry_delay = hs.config.federation.max_long_retry_delay - self.max_short_retry_delay = hs.config.federation.max_short_retry_delay - self.max_long_retries = hs.config.federation.max_long_retries - self.max_short_retries = hs.config.federation.max_short_retries + self.default_timeout = 60 self._cooperator = Cooperator(scheduler=_make_scheduler(self.reactor)) @@ -586,9 +583,9 @@ class MatrixFederationHttpClient: # XXX: Would be much nicer to retry only at the transaction-layer # (once we have reliable transactions in place) if long_retries: - retries_left = self.max_long_retries + retries_left = MAX_LONG_RETRIES else: - retries_left = self.max_short_retries + retries_left = MAX_SHORT_RETRIES url_bytes = request.uri url_str = url_bytes.decode("ascii") @@ -733,12 +730,12 @@ class MatrixFederationHttpClient: if retries_left and not timeout: if long_retries: - delay = 4 ** (self.max_long_retries + 1 - retries_left) - delay = min(delay, self.max_long_retry_delay) + delay = 4 ** (MAX_LONG_RETRIES + 1 - retries_left) + delay = min(delay, 60) delay *= random.uniform(0.8, 1.4) else: - delay = 0.5 * 2 ** (self.max_short_retries - retries_left) - delay = min(delay, self.max_short_retry_delay) + delay = 0.5 * 2 ** (MAX_SHORT_RETRIES - retries_left) + delay = min(delay, 2) delay *= random.uniform(0.8, 1.4) logger.debug( diff --git a/tests/http/test_matrixfederationclient.py b/tests/http/test_matrixfederationclient.py index 8565f8ac64..0dfc03ce50 100644 --- a/tests/http/test_matrixfederationclient.py +++ b/tests/http/test_matrixfederationclient.py @@ -40,7 +40,7 @@ from synapse.server import HomeServer from synapse.util import Clock from tests.server import FakeTransport -from tests.unittest import HomeserverTestCase, override_config +from tests.unittest import HomeserverTestCase def check_logcontext(context: LoggingContextOrSentinel) -> None: @@ -640,21 +640,3 @@ class FederationClientTests(HomeserverTestCase): self.cl.build_auth_headers( b"", b"GET", b"https://example.com", destination_is=b"" ) - - @override_config( - { - "federation": { - "client_timeout": 180, - "max_long_retry_delay": 100, - "max_short_retry_delay": 7, - "max_long_retries": 20, - "max_short_retries": 5, - } - } - ) - def test_configurable_retry_and_delay_values(self) -> None: - self.assertEqual(self.cl.default_timeout, 180) - self.assertEqual(self.cl.max_long_retry_delay, 100) - self.assertEqual(self.cl.max_short_retry_delay, 7) - self.assertEqual(self.cl.max_long_retries, 20) - self.assertEqual(self.cl.max_short_retries, 5)