From d2d1dd91c07f632a44e7750afaeedbe4c4dc3850 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tadeusz=20So=C5=9Bnierz?= Date: Tue, 10 May 2022 12:35:56 +0200 Subject: [PATCH 01/11] Allow configuring maximum interval between AS transaction retries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Tadeusz Sośnierz --- synapse/appservice/scheduler.py | 27 ++++++++++++++++++++++++--- synapse/config/appservice.py | 7 +++++++ 2 files changed, 31 insertions(+), 3 deletions(-) diff --git a/synapse/appservice/scheduler.py b/synapse/appservice/scheduler.py index 3b49e6071677..352736550771 100644 --- a/synapse/appservice/scheduler.py +++ b/synapse/appservice/scheduler.py @@ -59,6 +59,7 @@ Optional, Set, Tuple, + Union, ) from synapse.appservice import ( @@ -68,6 +69,7 @@ TransactionUnusedFallbackKeys, ) from synapse.appservice.api import ApplicationServiceApi +from synapse.config.appservice import AppServiceConfig from synapse.events import EventBase from synapse.logging.context import run_in_background from synapse.metrics.background_process_metrics import run_as_background_process @@ -102,7 +104,9 @@ def __init__(self, hs: "HomeServer"): self.store = hs.get_datastores().main self.as_api = hs.get_application_service_api() - self.txn_ctrl = _TransactionController(self.clock, self.store, self.as_api) + self.txn_ctrl = _TransactionController( + self.clock, self.store, self.as_api, hs.config.appservice + ) self.queuer = _ServiceQueuer(self.txn_ctrl, self.clock, hs) async def start(self) -> None: @@ -347,10 +351,17 @@ class _TransactionController: (Note we have only have one of these in the homeserver.) """ - def __init__(self, clock: Clock, store: DataStore, as_api: ApplicationServiceApi): + def __init__( + self, + clock: Clock, + store: DataStore, + as_api: ApplicationServiceApi, + as_config: AppServiceConfig, + ): self.clock = clock self.store = store self.as_api = as_api + self.as_config = as_config # map from service id to recoverer instance self.recoverers: Dict[str, "_Recoverer"] = {} @@ -430,7 +441,12 @@ def start_recoverer(self, service: ApplicationService) -> None: logger.info("Starting recoverer for AS ID %s", service.id) assert service.id not in self.recoverers recoverer = self.RECOVERER_CLASS( - self.clock, self.store, self.as_api, service, self.on_recovered + self.clock, + self.store, + self.as_api, + service, + self.on_recovered, + self.as_config.appservice_max_backoff, ) self.recoverers[service.id] = recoverer recoverer.recover() @@ -452,6 +468,7 @@ class _Recoverer: as_api (synapse.appservice.api.ApplicationServiceApi): service (synapse.appservice.ApplicationService): the service we are managing callback (callable[_Recoverer]): called once the service recovers. + max_backoff (int|None): maximum interval between retries """ def __init__( @@ -461,6 +478,7 @@ def __init__( as_api: ApplicationServiceApi, service: ApplicationService, callback: Callable[["_Recoverer"], Awaitable[None]], + max_backoff: Union[int, None], ): self.clock = clock self.store = store @@ -468,6 +486,7 @@ def __init__( self.service = service self.callback = callback self.backoff_counter = 1 + self.max_backoff = max_backoff def recover(self) -> None: def _retry() -> None: @@ -476,6 +495,8 @@ def _retry() -> None: ) delay = 2**self.backoff_counter + if self.max_backoff is not None and delay > self.max_backoff: + delay = self.max_backoff logger.info("Scheduling retries on %s in %fs", self.service.id, delay) self.clock.call_later(delay, _retry) diff --git a/synapse/config/appservice.py b/synapse/config/appservice.py index 24498e794433..6ddc6d9de2d2 100644 --- a/synapse/config/appservice.py +++ b/synapse/config/appservice.py @@ -34,6 +34,7 @@ class AppServiceConfig(Config): def read_config(self, config: JsonDict, **kwargs: Any) -> None: self.app_service_config_files = config.get("app_service_config_files", []) self.track_appservice_user_ips = config.get("track_appservice_user_ips", False) + self.appservice_max_backoff = config.get("appservice_max_backoff", None) def generate_config_section(cls, **kwargs: Any) -> str: return """\ @@ -47,6 +48,12 @@ def generate_config_section(cls, **kwargs: Any) -> str: # enables MAU tracking for application service users. # #track_appservice_user_ips: true + + # Set to establish maximum backoff (in seconds) between HS -> AS connection attempts. + # Upon failing to push appservice events the homeserver will wait + # an increasing amount of seconds between retries. This sets an upper limit of that (in seconds) + # + #appservice_max_backoff: 60 """ From 9c578f064f27f81a6aece9e3f6533dea75043995 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tadeusz=20So=C5=9Bnierz?= Date: Tue, 10 May 2022 13:05:38 +0200 Subject: [PATCH 02/11] Update sample config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Tadeusz Sośnierz --- docs/sample_config.yaml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index a803b8261dcd..cfcb85cf0623 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -1594,6 +1594,12 @@ room_prejoin_state: # #track_appservice_user_ips: true +# Set to establish maximum backoff (in seconds) between HS -> AS connection attempts. +# Upon failing to push appservice events the homeserver will wait +# an increasing amount of seconds between retries. This sets an upper limit of that (in seconds) +# +#appservice_max_backoff: 60 + # a secret which is used to sign access tokens. If none is specified, # the registration_shared_secret is used, if one is given; otherwise, From a7981b68aeb6f3563f77eb45f1576a01e8e93814 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tadeusz=20So=C5=9Bnierz?= Date: Tue, 10 May 2022 13:07:38 +0200 Subject: [PATCH 03/11] Add a changelog entry for #12685 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Tadeusz Sośnierz --- changelog.d/12685.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/12685.feature diff --git a/changelog.d/12685.feature b/changelog.d/12685.feature new file mode 100644 index 000000000000..13ad57f0d450 --- /dev/null +++ b/changelog.d/12685.feature @@ -0,0 +1 @@ +Allow configuring maximum interval between AS transaction retries. From 88f97de05825191556f1a3fb9c319edd3d568974 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tadeusz=20So=C5=9Bnierz?= Date: Tue, 10 May 2022 15:47:28 +0200 Subject: [PATCH 04/11] Fix appservice.scheduler tests --- tests/appservice/test_scheduler.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/appservice/test_scheduler.py b/tests/appservice/test_scheduler.py index 0b22afdc7598..f1653716751b 100644 --- a/tests/appservice/test_scheduler.py +++ b/tests/appservice/test_scheduler.py @@ -22,6 +22,7 @@ _Recoverer, _TransactionController, ) +from synapse.config.appservice import AppServiceConfig from synapse.logging.context import make_deferred_yieldable from synapse.server import HomeServer from synapse.types import DeviceListUpdates @@ -43,8 +44,10 @@ def setUp(self): self.as_api = Mock() self.recoverer = Mock() self.recoverer_fn = Mock(return_value=self.recoverer) + as_config = AppServiceConfig() + as_config.read_config({}) self.txnctrl = _TransactionController( - clock=self.clock, store=self.store, as_api=self.as_api + clock=self.clock, store=self.store, as_api=self.as_api, as_config=as_config ) self.txnctrl.RECOVERER_CLASS = self.recoverer_fn @@ -151,6 +154,7 @@ def setUp(self): store=self.store, service=self.service, callback=self.callback, + max_backoff=None, ) def test_recover_single_txn(self): From 04f1c9e7b804aee6ccb3c79a9e3514e80cdfd071 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tadeusz=20So=C5=9Bnierz?= Date: Fri, 13 May 2022 11:54:03 +0200 Subject: [PATCH 05/11] Use Optional instead of a Union Co-authored-by: David Robertson --- synapse/appservice/scheduler.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/appservice/scheduler.py b/synapse/appservice/scheduler.py index 352736550771..71b2cb67989a 100644 --- a/synapse/appservice/scheduler.py +++ b/synapse/appservice/scheduler.py @@ -478,7 +478,7 @@ def __init__( as_api: ApplicationServiceApi, service: ApplicationService, callback: Callable[["_Recoverer"], Awaitable[None]], - max_backoff: Union[int, None], + max_backoff: Optional[int] ): self.clock = clock self.store = store From 49cc2b7c33a8827826bbd56fdab36c3e43ed42ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tadeusz=20So=C5=9Bnierz?= Date: Fri, 13 May 2022 12:01:13 +0200 Subject: [PATCH 06/11] Improve wording for appservice_max_backoff Co-authored-by: David Robertson --- synapse/config/appservice.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/synapse/config/appservice.py b/synapse/config/appservice.py index 6ddc6d9de2d2..4963a862cf98 100644 --- a/synapse/config/appservice.py +++ b/synapse/config/appservice.py @@ -49,9 +49,10 @@ def generate_config_section(cls, **kwargs: Any) -> str: # #track_appservice_user_ips: true - # Set to establish maximum backoff (in seconds) between HS -> AS connection attempts. - # Upon failing to push appservice events the homeserver will wait - # an increasing amount of seconds between retries. This sets an upper limit of that (in seconds) + # Set to establish a maximum backoff (in seconds) between HS -> AS connection attempts. + # Upon failing to push appservice events, the homeserver will reattempt connection to the + # application service after a delay. The delay increases with subsequent retries. + # This value sets an upper limit on that delay. # #appservice_max_backoff: 60 """ From cf22683680a2fda657cc20956a9a29d660f9d687 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tadeusz=20So=C5=9Bnierz?= Date: Fri, 13 May 2022 12:00:50 +0200 Subject: [PATCH 07/11] Fix linting in appservice.scheduler --- synapse/appservice/scheduler.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/synapse/appservice/scheduler.py b/synapse/appservice/scheduler.py index 71b2cb67989a..2178aa23433f 100644 --- a/synapse/appservice/scheduler.py +++ b/synapse/appservice/scheduler.py @@ -59,7 +59,6 @@ Optional, Set, Tuple, - Union, ) from synapse.appservice import ( @@ -478,7 +477,7 @@ def __init__( as_api: ApplicationServiceApi, service: ApplicationService, callback: Callable[["_Recoverer"], Awaitable[None]], - max_backoff: Optional[int] + max_backoff: Optional[int], ): self.clock = clock self.store = store From 727bbf8559b2048026e5f0651df4c4aed3b2db14 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tadeusz=20So=C5=9Bnierz?= Date: Fri, 13 May 2022 12:08:42 +0200 Subject: [PATCH 08/11] Document the default behaviour of appservice_max_backoff --- synapse/config/appservice.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/synapse/config/appservice.py b/synapse/config/appservice.py index 4963a862cf98..8c010713c74a 100644 --- a/synapse/config/appservice.py +++ b/synapse/config/appservice.py @@ -54,6 +54,8 @@ def generate_config_section(cls, **kwargs: Any) -> str: # application service after a delay. The delay increases with subsequent retries. # This value sets an upper limit on that delay. # + # Regardless of this setting, the delay will never be longer than 512 seconds (about 8.5 minutes). + # #appservice_max_backoff: 60 """ From bf6b698c67c79efd88c22686b3c5d8cde4d36535 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tadeusz=20So=C5=9Bnierz?= Date: Fri, 20 May 2022 09:58:00 +0200 Subject: [PATCH 09/11] Update sample config with appservice_max_backoff wording changes --- docs/sample_config.yaml | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index cfcb85cf0623..283d4b0adeb8 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -1594,9 +1594,12 @@ room_prejoin_state: # #track_appservice_user_ips: true -# Set to establish maximum backoff (in seconds) between HS -> AS connection attempts. -# Upon failing to push appservice events the homeserver will wait -# an increasing amount of seconds between retries. This sets an upper limit of that (in seconds) +# Set to establish a maximum backoff (in seconds) between HS -> AS connection attempts. +# Upon failing to push appservice events, the homeserver will reattempt connection to the +# application service after a delay. The delay increases with subsequent retries. +# This value sets an upper limit on that delay. +# +# Regardless of this setting, the delay will never be longer than 512 seconds (about 8.5 minutes). # #appservice_max_backoff: 60 From 548778ae08c383074386c0cbf1bab8597c5842bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tadeusz=20So=C5=9Bnierz?= Date: Fri, 20 May 2022 10:01:01 +0200 Subject: [PATCH 10/11] Rename (appservice_)max_backoff to max_backoff_s --- docs/sample_config.yaml | 2 +- synapse/appservice/scheduler.py | 12 ++++++------ synapse/config/appservice.py | 4 ++-- tests/appservice/test_scheduler.py | 2 +- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index 283d4b0adeb8..8213b95e6156 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -1601,7 +1601,7 @@ room_prejoin_state: # # Regardless of this setting, the delay will never be longer than 512 seconds (about 8.5 minutes). # -#appservice_max_backoff: 60 +#appservice_max_backoff_s: 60 # a secret which is used to sign access tokens. If none is specified, diff --git a/synapse/appservice/scheduler.py b/synapse/appservice/scheduler.py index 2178aa23433f..5cd96de4e393 100644 --- a/synapse/appservice/scheduler.py +++ b/synapse/appservice/scheduler.py @@ -445,7 +445,7 @@ def start_recoverer(self, service: ApplicationService) -> None: self.as_api, service, self.on_recovered, - self.as_config.appservice_max_backoff, + self.as_config.appservice_max_backoff_s, ) self.recoverers[service.id] = recoverer recoverer.recover() @@ -467,7 +467,7 @@ class _Recoverer: as_api (synapse.appservice.api.ApplicationServiceApi): service (synapse.appservice.ApplicationService): the service we are managing callback (callable[_Recoverer]): called once the service recovers. - max_backoff (int|None): maximum interval between retries + max_backoff_s (int|None): maximum interval between retries """ def __init__( @@ -477,7 +477,7 @@ def __init__( as_api: ApplicationServiceApi, service: ApplicationService, callback: Callable[["_Recoverer"], Awaitable[None]], - max_backoff: Optional[int], + max_backoff_s: Optional[int], ): self.clock = clock self.store = store @@ -485,7 +485,7 @@ def __init__( self.service = service self.callback = callback self.backoff_counter = 1 - self.max_backoff = max_backoff + self.max_backoff_s = max_backoff_s def recover(self) -> None: def _retry() -> None: @@ -494,8 +494,8 @@ def _retry() -> None: ) delay = 2**self.backoff_counter - if self.max_backoff is not None and delay > self.max_backoff: - delay = self.max_backoff + if self.max_backoff_s is not None and delay > self.max_backoff_s: + delay = self.max_backoff_s logger.info("Scheduling retries on %s in %fs", self.service.id, delay) self.clock.call_later(delay, _retry) diff --git a/synapse/config/appservice.py b/synapse/config/appservice.py index 8c010713c74a..f4650d8e3655 100644 --- a/synapse/config/appservice.py +++ b/synapse/config/appservice.py @@ -34,7 +34,7 @@ class AppServiceConfig(Config): def read_config(self, config: JsonDict, **kwargs: Any) -> None: self.app_service_config_files = config.get("app_service_config_files", []) self.track_appservice_user_ips = config.get("track_appservice_user_ips", False) - self.appservice_max_backoff = config.get("appservice_max_backoff", None) + self.appservice_max_backoff_s = config.get("appservice_max_backoff_s", None) def generate_config_section(cls, **kwargs: Any) -> str: return """\ @@ -56,7 +56,7 @@ def generate_config_section(cls, **kwargs: Any) -> str: # # Regardless of this setting, the delay will never be longer than 512 seconds (about 8.5 minutes). # - #appservice_max_backoff: 60 + #appservice_max_backoff_s: 60 """ diff --git a/tests/appservice/test_scheduler.py b/tests/appservice/test_scheduler.py index f1653716751b..377ec9d2bbd4 100644 --- a/tests/appservice/test_scheduler.py +++ b/tests/appservice/test_scheduler.py @@ -154,7 +154,7 @@ def setUp(self): store=self.store, service=self.service, callback=self.callback, - max_backoff=None, + max_backoff_s=None, ) def test_recover_single_txn(self): From 08f7d323b7f454f747fb6af86f5b9c76f3edea83 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tadeusz=20So=C5=9Bnierz?= Date: Fri, 20 May 2022 10:15:09 +0200 Subject: [PATCH 11/11] Document appservice_max_backoff_s in config documentation --- docs/usage/configuration/config_documentation.md | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/docs/usage/configuration/config_documentation.md b/docs/usage/configuration/config_documentation.md index 21dad0ac41e2..9173c5363c59 100644 --- a/docs/usage/configuration/config_documentation.md +++ b/docs/usage/configuration/config_documentation.md @@ -2292,6 +2292,21 @@ Example configuration: track_appservice_user_ips: true ``` --- +Config option: `appservice_max_backoff_s` + +Set to establish a maximum backoff (in seconds) between HS -> AS connection attempts. +Upon failing to push appservice events, the homeserver will reattempt connection to the +application service after a delay. The delay increases with subsequent retries. +This value sets an upper limit on that delay. + +Regardless of this setting, the delay will never be longer than 512 seconds (about 8.5 minutes), +which is the default behaviour if this option is not set. + +Example configuration: +```yaml +appservice_max_backoff_s: 1 +``` +--- Config option: `macaroon_secret_key` A secret which is used to sign access tokens. If none is specified,