From c3921251be56dbaad4abb41c53b072dbb220c04e Mon Sep 17 00:00:00 2001 From: Jonathan de Jong Date: Fri, 16 Oct 2020 15:02:14 +0200 Subject: [PATCH 1/8] change @cache_in_self to use underscore-prefixed attributes also change some usages to use .get_xyz() instead of .xyz --- synapse/handlers/identity.py | 3 ++- synapse/rest/client/v2_alpha/account.py | 6 +++--- synapse/rest/client/v2_alpha/register.py | 4 ++-- synapse/rest/key/v2/local_key_resource.py | 2 +- synapse/server.py | 14 +++++++++----- tests/handlers/test_auth.py | 6 +++--- tests/replication/_base.py | 2 +- tests/rest/client/v1/test_presence.py | 12 ++++++------ tests/rest/client/v2_alpha/test_register.py | 6 +++--- tests/utils.py | 2 +- 10 files changed, 31 insertions(+), 26 deletions(-) diff --git a/synapse/handlers/identity.py b/synapse/handlers/identity.py index bc3e9607ca82..9b3c6b4551eb 100644 --- a/synapse/handlers/identity.py +++ b/synapse/handlers/identity.py @@ -354,7 +354,8 @@ async def send_threepid_validation( raise SynapseError(500, "An error was encountered when sending the email") token_expires = ( - self.hs.clock.time_msec() + self.hs.config.email_validation_token_lifetime + self.hs.get_clock().time_msec() + + self.hs.config.email_validation_token_lifetime ) await self.store.start_or_continue_validation_session( diff --git a/synapse/rest/client/v2_alpha/account.py b/synapse/rest/client/v2_alpha/account.py index e857cff17616..517dc3117b9f 100644 --- a/synapse/rest/client/v2_alpha/account.py +++ b/synapse/rest/client/v2_alpha/account.py @@ -114,7 +114,7 @@ async def on_POST(self, request): # comments for request_token_inhibit_3pid_errors. # Also wait for some random amount of time between 100ms and 1s to make it # look like we did something. - await self.hs.clock.sleep(random.randint(1, 10) / 10) + await self.hs.get_clock().sleep(random.randint(1, 10) / 10) return 200, {"sid": random_string(16)} raise SynapseError(400, "Email not found", Codes.THREEPID_NOT_FOUND) @@ -385,7 +385,7 @@ async def on_POST(self, request): # comments for request_token_inhibit_3pid_errors. # Also wait for some random amount of time between 100ms and 1s to make it # look like we did something. - await self.hs.clock.sleep(random.randint(1, 10) / 10) + await self.hs.get_clock().sleep(random.randint(1, 10) / 10) return 200, {"sid": random_string(16)} raise SynapseError(400, "Email is already in use", Codes.THREEPID_IN_USE) @@ -460,7 +460,7 @@ async def on_POST(self, request): # comments for request_token_inhibit_3pid_errors. # Also wait for some random amount of time between 100ms and 1s to make it # look like we did something. - await self.hs.clock.sleep(random.randint(1, 10) / 10) + await self.hs.get_clock().sleep(random.randint(1, 10) / 10) return 200, {"sid": random_string(16)} raise SynapseError(400, "MSISDN is already in use", Codes.THREEPID_IN_USE) diff --git a/synapse/rest/client/v2_alpha/register.py b/synapse/rest/client/v2_alpha/register.py index 395b6a82a978..8074693d088e 100644 --- a/synapse/rest/client/v2_alpha/register.py +++ b/synapse/rest/client/v2_alpha/register.py @@ -134,7 +134,7 @@ async def on_POST(self, request): # comments for request_token_inhibit_3pid_errors. # Also wait for some random amount of time between 100ms and 1s to make it # look like we did something. - await self.hs.clock.sleep(random.randint(1, 10) / 10) + await self.hs.get_clock().sleep(random.randint(1, 10) / 10) return 200, {"sid": random_string(16)} raise SynapseError(400, "Email is already in use", Codes.THREEPID_IN_USE) @@ -209,7 +209,7 @@ async def on_POST(self, request): # comments for request_token_inhibit_3pid_errors. # Also wait for some random amount of time between 100ms and 1s to make it # look like we did something. - await self.hs.clock.sleep(random.randint(1, 10) / 10) + await self.hs.get_clock().sleep(random.randint(1, 10) / 10) return 200, {"sid": random_string(16)} raise SynapseError( diff --git a/synapse/rest/key/v2/local_key_resource.py b/synapse/rest/key/v2/local_key_resource.py index c16280f66806..d8e8e48c1c1e 100644 --- a/synapse/rest/key/v2/local_key_resource.py +++ b/synapse/rest/key/v2/local_key_resource.py @@ -66,7 +66,7 @@ class LocalKey(Resource): def __init__(self, hs): self.config = hs.config - self.clock = hs.clock + self.clock = hs.get_clock() self.update_response_body(self.clock.time_msec()) Resource.__init__(self) diff --git a/synapse/server.py b/synapse/server.py index 21a232bbd964..f3c9cf99873b 100644 --- a/synapse/server.py +++ b/synapse/server.py @@ -145,7 +145,8 @@ def cache_in_self(builder: T) -> T: "@cache_in_self can only be used on functions starting with `get_`" ) - depname = builder.__name__[len("get_") :] + # get_attr -> _attr + depname = builder.__name__[len("get") :] building = [False] @@ -233,11 +234,11 @@ def __init__( self._instance_id = random_string(5) self._instance_name = config.worker_name or "master" - self.clock = Clock(reactor) + self._clock = Clock(reactor) self.distributor = Distributor() self.registration_ratelimiter = Ratelimiter( - clock=self.clock, + clock=self._clock, rate_hz=config.rc_registration.per_second, burst_count=config.rc_registration.burst_count, ) @@ -299,8 +300,11 @@ def is_mine(self, domain_specific_string: DomainSpecificString) -> bool: def is_mine_id(self, string: str) -> bool: return string.split(":", 1)[1] == self.hostname + # The caching attribute for this is technically already set in __init__, but it's replicated for the sake of + # consistency + @cache_in_self def get_clock(self) -> Clock: - return self.clock + return Clock(self._reactor) def get_datastore(self) -> DataStore: if not self.datastores: @@ -681,7 +685,7 @@ def get_replication_streams(self) -> Dict[str, Stream]: @cache_in_self def get_federation_ratelimiter(self) -> FederationRateLimiter: - return FederationRateLimiter(self.clock, config=self.config.rc_federation) + return FederationRateLimiter(self.get_clock(), config=self.config.rc_federation) @cache_in_self def get_module_api(self) -> ModuleApi: diff --git a/tests/handlers/test_auth.py b/tests/handlers/test_auth.py index b5055e018cbd..e24ce8128425 100644 --- a/tests/handlers/test_auth.py +++ b/tests/handlers/test_auth.py @@ -52,7 +52,7 @@ def test_token_is_a_macaroon(self): self.fail("some_user was not in %s" % macaroon.inspect()) def test_macaroon_caveats(self): - self.hs.clock.now = 5000 + self.hs.get_clock().now = 5000 token = self.macaroon_generator.generate_access_token("a_user") macaroon = pymacaroons.Macaroon.deserialize(token) @@ -78,7 +78,7 @@ def verify_nonce(caveat): @defer.inlineCallbacks def test_short_term_login_token_gives_user_id(self): - self.hs.clock.now = 1000 + self.hs.get_clock().now = 1000 token = self.macaroon_generator.generate_short_term_login_token("a_user", 5000) user_id = yield defer.ensureDeferred( @@ -87,7 +87,7 @@ def test_short_term_login_token_gives_user_id(self): self.assertEqual("a_user", user_id) # when we advance the clock, the token should be rejected - self.hs.clock.now = 6000 + self.hs.get_clock().now = 6000 with self.assertRaises(synapse.api.errors.AuthError): yield defer.ensureDeferred( self.auth_handler.validate_short_term_login_token_and_get_user_id(token) diff --git a/tests/replication/_base.py b/tests/replication/_base.py index 093e2faac7bf..2e04fdc8144b 100644 --- a/tests/replication/_base.py +++ b/tests/replication/_base.py @@ -69,7 +69,7 @@ def prepare(self, reactor, clock, hs): self.worker_hs.get_datastore().db_pool = hs.get_datastore().db_pool self.test_handler = self._build_replication_data_handler() - self.worker_hs.replication_data_handler = self.test_handler + self.worker_hs._replication_data_handler = self.test_handler repl_handler = ReplicationCommandHandler(self.worker_hs) self.client = ClientReplicationStreamProtocol( diff --git a/tests/rest/client/v1/test_presence.py b/tests/rest/client/v1/test_presence.py index 3c66255daca7..a90eba1c6701 100644 --- a/tests/rest/client/v1/test_presence.py +++ b/tests/rest/client/v1/test_presence.py @@ -33,13 +33,13 @@ class PresenceTestCase(unittest.HomeserverTestCase): def make_homeserver(self, reactor, clock): + ph = Mock() + ph.set_state.return_value = defer.succeed(None) + hs = self.setup_test_homeserver( - "red", http_client=None, federation_client=Mock() + "red", http_client=None, federation_client=Mock(), presence_handler=ph ) - hs.presence_handler = Mock() - hs.presence_handler.set_state.return_value = defer.succeed(None) - return hs def test_put_presence(self): @@ -56,7 +56,7 @@ def test_put_presence(self): self.render(request) self.assertEqual(channel.code, 200) - self.assertEqual(self.hs.presence_handler.set_state.call_count, 1) + self.assertEqual(self.hs.get_presence_handler().set_state.call_count, 1) def test_put_presence_disabled(self): """ @@ -72,4 +72,4 @@ def test_put_presence_disabled(self): self.render(request) self.assertEqual(channel.code, 200) - self.assertEqual(self.hs.presence_handler.set_state.call_count, 0) + self.assertEqual(self.hs.get_presence_handler().set_state.call_count, 0) diff --git a/tests/rest/client/v2_alpha/test_register.py b/tests/rest/client/v2_alpha/test_register.py index 2fc3a60fc53d..1dd207c5080c 100644 --- a/tests/rest/client/v2_alpha/test_register.py +++ b/tests/rest/client/v2_alpha/test_register.py @@ -598,7 +598,7 @@ def create_user(self): tok = self.login("kermit", "monkey") # We need to manually add an email address otherwise the handler will do # nothing. - now = self.hs.clock.time_msec() + now = self.hs.get_clock().time_msec() self.get_success( self.store.user_add_threepid( user_id=user_id, @@ -616,7 +616,7 @@ def test_manual_email_send_expired_account(self): # We need to manually add an email address otherwise the handler will do # nothing. - now = self.hs.clock.time_msec() + now = self.hs.get_clock().time_msec() self.get_success( self.store.user_add_threepid( user_id=user_id, @@ -676,7 +676,7 @@ def test_background_job(self): self.hs.config.account_validity.startup_job_max_delta = self.max_delta - now_ms = self.hs.clock.time_msec() + now_ms = self.hs.get_clock().time_msec() self.get_success(self.store._set_expiration_date_when_missing()) res = self.get_success(self.store.get_expiration_ts_for_user(user_id)) diff --git a/tests/utils.py b/tests/utils.py index acec74e9e916..c8d3ffbabaeb 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -271,7 +271,7 @@ def setup_test_homeserver( # Install @cache_in_self attributes for key, val in kwargs.items(): - setattr(hs, key, val) + setattr(hs, "_" + key, val) # Mock TLS hs.tls_server_context_factory = Mock() From d7e24a27a5564c094ed3a3e673666cbf57d3e4b8 Mon Sep 17 00:00:00 2001 From: Jonathan de Jong Date: Fri, 16 Oct 2020 15:04:55 +0200 Subject: [PATCH 2/8] add news --- changelog.d/8565.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/8565.misc diff --git a/changelog.d/8565.misc b/changelog.d/8565.misc new file mode 100644 index 000000000000..ac6194f1dedb --- /dev/null +++ b/changelog.d/8565.misc @@ -0,0 +1 @@ +Apply some internal changes to the way the `HomeServer` caches it's internal attributes, to make it more idiomatic. \ No newline at end of file From ade638a3dbd4a057caa5c5b58e663703d6504785 Mon Sep 17 00:00:00 2001 From: Jonathan de Jong Date: Mon, 19 Oct 2020 20:03:39 +0200 Subject: [PATCH 3/8] grammar correction Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- changelog.d/8565.misc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/8565.misc b/changelog.d/8565.misc index ac6194f1dedb..9611c7409ebf 100644 --- a/changelog.d/8565.misc +++ b/changelog.d/8565.misc @@ -1 +1 @@ -Apply some internal changes to the way the `HomeServer` caches it's internal attributes, to make it more idiomatic. \ No newline at end of file +Apply some internal changes to the way the `HomeServer` caches its internal attributes, to make it more idiomatic. From a0ccb30184bebebf4479b5429cba09faccc70528 Mon Sep 17 00:00:00 2001 From: Jonathan de Jong Date: Wed, 21 Oct 2020 16:21:52 +0200 Subject: [PATCH 4/8] Make clock and registration_ratelimiter a cached attribute --- synapse/server.py | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/synapse/server.py b/synapse/server.py index f3c9cf99873b..3afc79eb3443 100644 --- a/synapse/server.py +++ b/synapse/server.py @@ -234,15 +234,8 @@ def __init__( self._instance_id = random_string(5) self._instance_name = config.worker_name or "master" - self._clock = Clock(reactor) self.distributor = Distributor() - self.registration_ratelimiter = Ratelimiter( - clock=self._clock, - rate_hz=config.rc_registration.per_second, - burst_count=config.rc_registration.burst_count, - ) - self.version_string = version_string self.datastores = None # type: Optional[Databases] @@ -300,8 +293,6 @@ def is_mine(self, domain_specific_string: DomainSpecificString) -> bool: def is_mine_id(self, string: str) -> bool: return string.split(":", 1)[1] == self.hostname - # The caching attribute for this is technically already set in __init__, but it's replicated for the sake of - # consistency @cache_in_self def get_clock(self) -> Clock: return Clock(self._reactor) @@ -324,8 +315,13 @@ def get_config(self) -> HomeServerConfig: def get_distributor(self) -> Distributor: return self.distributor + @cache_in_self def get_registration_ratelimiter(self) -> Ratelimiter: - return self.registration_ratelimiter + return Ratelimiter( + clock=self.get_clock(), + rate_hz=self.config.rc_registration.per_second, + burst_count=self.config.rc_registration.burst_count, + ) @cache_in_self def get_federation_client(self) -> FederationClient: From b2f0793ca4dda510516b436f4eb66d9f38f5bd4b Mon Sep 17 00:00:00 2001 From: Jonathan de Jong Date: Wed, 21 Oct 2020 21:55:22 +0200 Subject: [PATCH 5/8] change ph to presence_handler --- tests/rest/client/v1/test_presence.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/rest/client/v1/test_presence.py b/tests/rest/client/v1/test_presence.py index a90eba1c6701..933540a9c922 100644 --- a/tests/rest/client/v1/test_presence.py +++ b/tests/rest/client/v1/test_presence.py @@ -33,11 +33,11 @@ class PresenceTestCase(unittest.HomeserverTestCase): def make_homeserver(self, reactor, clock): - ph = Mock() - ph.set_state.return_value = defer.succeed(None) + presence_handler = Mock() + presence_handler.set_state.return_value = defer.succeed(None) hs = self.setup_test_homeserver( - "red", http_client=None, federation_client=Mock(), presence_handler=ph + "red", http_client=None, federation_client=Mock(), presence_handler=presence_handler ) return hs From 265b6e49c078fd9c9bbe273dd70dd36b7bd62e79 Mon Sep 17 00:00:00 2001 From: Jonathan de Jong Date: Wed, 21 Oct 2020 22:02:30 +0200 Subject: [PATCH 6/8] Please The Linter --- tests/rest/client/v1/test_presence.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/rest/client/v1/test_presence.py b/tests/rest/client/v1/test_presence.py index 933540a9c922..6f85bbf8717d 100644 --- a/tests/rest/client/v1/test_presence.py +++ b/tests/rest/client/v1/test_presence.py @@ -37,7 +37,10 @@ def make_homeserver(self, reactor, clock): presence_handler.set_state.return_value = defer.succeed(None) hs = self.setup_test_homeserver( - "red", http_client=None, federation_client=Mock(), presence_handler=presence_handler + "red", + http_client=None, + federation_client=Mock(), + presence_handler=presence_handler, ) return hs From bca1e4982b10ce26528634ba557d7598c0771f96 Mon Sep 17 00:00:00 2001 From: Jonathan de Jong Date: Mon, 2 Nov 2020 08:15:20 +0000 Subject: [PATCH 7/8] Apply feedback --- synapse/server.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/synapse/server.py b/synapse/server.py index 3afc79eb3443..62c2079c31cc 100644 --- a/synapse/server.py +++ b/synapse/server.py @@ -234,8 +234,6 @@ def __init__( self._instance_id = random_string(5) self._instance_name = config.worker_name or "master" - self.distributor = Distributor() - self.version_string = version_string self.datastores = None # type: Optional[Databases] @@ -312,8 +310,9 @@ def get_datastores(self) -> Databases: def get_config(self) -> HomeServerConfig: return self.config + @cache_in_self def get_distributor(self) -> Distributor: - return self.distributor + return Distributor() @cache_in_self def get_registration_ratelimiter(self) -> Ratelimiter: From c950d7ebd37124c263c7ac85596f1f66b6f284ce Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 30 Nov 2020 11:02:01 -0500 Subject: [PATCH 8/8] Clarify changelog. --- changelog.d/8565.misc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/8565.misc b/changelog.d/8565.misc index 9611c7409ebf..7bef4226186c 100644 --- a/changelog.d/8565.misc +++ b/changelog.d/8565.misc @@ -1 +1 @@ -Apply some internal changes to the way the `HomeServer` caches its internal attributes, to make it more idiomatic. +Simplify the way the `HomeServer` object caches its internal attributes.