From 6e10ffac4b25cda34b988aa4c5b79cea57b0b577 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Tue, 2 Jun 2020 16:14:38 +0100 Subject: [PATCH 1/5] Replace instances of reactor manipulation with get_success. This has the benefit of working with coroutines, as well as checking that method execution completely successfully. --- tests/storage/test_monthly_active_users.py | 267 ++++++++++++--------- tests/test_mau.py | 5 +- 2 files changed, 151 insertions(+), 121 deletions(-) diff --git a/tests/storage/test_monthly_active_users.py b/tests/storage/test_monthly_active_users.py index 447fcb3a1c53..162fd6f4471e 100644 --- a/tests/storage/test_monthly_active_users.py +++ b/tests/storage/test_monthly_active_users.py @@ -61,21 +61,27 @@ def test_initialise_reserved_users(self): user2_email = threepids[1]["address"] user3 = "@user3:server" - self.store.register_user(user_id=user1) - self.store.register_user(user_id=user2) - self.store.register_user(user_id=user3, user_type=UserTypes.SUPPORT) - self.pump() + self.get_success(self.store.register_user(user_id=user1)) + self.get_success(self.store.register_user(user_id=user2)) + self.get_success( + self.store.register_user(user_id=user3, user_type=UserTypes.SUPPORT) + ) now = int(self.hs.get_clock().time_msec()) - self.store.user_add_threepid(user1, "email", user1_email, now, now) - self.store.user_add_threepid(user2, "email", user2_email, now, now) + self.get_success( + self.store.user_add_threepid(user1, "email", user1_email, now, now) + ) + self.get_success( + self.store.user_add_threepid(user2, "email", user2_email, now, now) + ) # XXX why are we doing this here? this function is only run at startup # so it is odd to re-run it here. - self.store.db.runInteraction( - "initialise", self.store._initialise_reserved_users, threepids + self.get_success( + self.store.db.runInteraction( + "initialise", self.store._initialise_reserved_users, threepids + ) ) - self.pump() # the number of users we expect will be counted against the mau limit # -1 because user3 is a support user and does not count @@ -83,13 +89,13 @@ def test_initialise_reserved_users(self): # Check the number of active users. Ensure user3 (support user) is not counted active_count = self.get_success(self.store.get_monthly_active_count()) - self.assertEquals(active_count, user_num) + self.assertEqual(active_count, user_num) # Test each of the registered users is marked as active - timestamp = self.store.user_last_seen_monthly_active(user1) - self.assertTrue(self.get_success(timestamp)) - timestamp = self.store.user_last_seen_monthly_active(user2) - self.assertTrue(self.get_success(timestamp)) + timestamp = self.get_success(self.store.user_last_seen_monthly_active(user1)) + self.assertGreater(timestamp, 0) + timestamp = self.get_success(self.store.user_last_seen_monthly_active(user2)) + self.assertGreater(timestamp, 0) # Test that users with reserved 3pids are not removed from the MAU table # XXX some of this is redundant. poking things into the config shouldn't @@ -98,77 +104,79 @@ def test_initialise_reserved_users(self): self.hs.config.max_mau_value = 0 self.reactor.advance(FORTY_DAYS) self.hs.config.max_mau_value = 5 - self.store.reap_monthly_active_users() - self.pump() - active_count = self.store.get_monthly_active_count() - self.assertEquals(self.get_success(active_count), user_num) + self.get_success(self.store.reap_monthly_active_users()) + + active_count = self.get_success(self.store.get_monthly_active_count()) + self.assertEqual(active_count, user_num) # Add some more users and check they are counted as active ru_count = 2 - self.store.upsert_monthly_active_user("@ru1:server") - self.store.upsert_monthly_active_user("@ru2:server") - self.pump() - active_count = self.store.get_monthly_active_count() - self.assertEqual(self.get_success(active_count), user_num + ru_count) + + self.get_success(self.store.upsert_monthly_active_user("@ru1:server")) + self.get_success(self.store.upsert_monthly_active_user("@ru2:server")) + + active_count = self.get_success(self.store.get_monthly_active_count()) + self.assertEqual(active_count, user_num + ru_count) # now run the reaper and check that the number of active users is reduced # to max_mau_value - self.store.reap_monthly_active_users() - self.pump() + self.get_success(self.store.reap_monthly_active_users()) - active_count = self.store.get_monthly_active_count() - self.assertEquals(self.get_success(active_count), 3) + active_count = self.get_success(self.store.get_monthly_active_count()) + self.assertEqual(active_count, 3) def test_can_insert_and_count_mau(self): - count = self.store.get_monthly_active_count() - self.assertEqual(0, self.get_success(count)) + count = self.get_success(self.store.get_monthly_active_count()) + self.assertEqual(count, 0) - self.store.upsert_monthly_active_user("@user:server") - self.pump() + d = self.store.upsert_monthly_active_user("@user:server") + self.get_success(d) - count = self.store.get_monthly_active_count() - self.assertEqual(1, self.get_success(count)) + count = self.get_success(self.store.get_monthly_active_count()) + self.assertEqual(count, 1) def test_user_last_seen_monthly_active(self): user_id1 = "@user1:server" user_id2 = "@user2:server" user_id3 = "@user3:server" - result = self.store.user_last_seen_monthly_active(user_id1) - self.assertFalse(self.get_success(result) == 0) + result = self.get_success(self.store.user_last_seen_monthly_active(user_id1)) + self.assertNotEqual(result, 0) - self.store.upsert_monthly_active_user(user_id1) - self.store.upsert_monthly_active_user(user_id2) - self.pump() + self.get_success(self.store.upsert_monthly_active_user(user_id1)) + self.get_success(self.store.upsert_monthly_active_user(user_id2)) - result = self.store.user_last_seen_monthly_active(user_id1) - self.assertGreater(self.get_success(result), 0) + result = self.get_success(self.store.user_last_seen_monthly_active(user_id1)) + self.assertGreater(result, 0) - result = self.store.user_last_seen_monthly_active(user_id3) - self.assertNotEqual(self.get_success(result), 0) + result = self.get_success(self.store.user_last_seen_monthly_active(user_id3)) + self.assertNotEqual(result, 0) @override_config({"max_mau_value": 5}) def test_reap_monthly_active_users(self): initial_users = 10 for i in range(initial_users): - self.store.upsert_monthly_active_user("@user%d:server" % i) - self.pump() + self.get_success( + self.store.upsert_monthly_active_user("@user%d:server" % i) + ) - count = self.store.get_monthly_active_count() - self.assertTrue(self.get_success(count), initial_users) + count = self.get_success(self.store.get_monthly_active_count()) + self.assertEqual(count, initial_users) - self.store.reap_monthly_active_users() - self.pump() - count = self.store.get_monthly_active_count() - self.assertEquals(self.get_success(count), self.hs.config.max_mau_value) + d = self.store.reap_monthly_active_users() + self.get_success(d) + + count = self.get_success(self.store.get_monthly_active_count()) + self.assertEqual(count, self.hs.config.max_mau_value) self.reactor.advance(FORTY_DAYS) - self.store.reap_monthly_active_users() - self.pump() - count = self.store.get_monthly_active_count() - self.assertEquals(self.get_success(count), 0) + d = self.store.reap_monthly_active_users() + self.get_success(d) + + count = self.get_success(self.store.get_monthly_active_count()) + self.assertEqual(count, 0) # Note that below says mau_limit (no s), this is the name of the config # value, although it gets stored on the config object as mau_limits. @@ -182,7 +190,9 @@ def test_reap_monthly_active_users_reserved_users(self): for i in range(initial_users): user = "@user%d:server" % i email = "user%d@matrix.org" % i + self.get_success(self.store.upsert_monthly_active_user(user)) + # Need to ensure that the most recent entries in the # monthly_active_users table are reserved now = int(self.hs.get_clock().time_msec()) @@ -194,26 +204,37 @@ def test_reap_monthly_active_users_reserved_users(self): self.store.user_add_threepid(user, "email", email, now, now) ) - self.store.db.runInteraction( + d = self.store.db.runInteraction( "initialise", self.store._initialise_reserved_users, threepids ) - count = self.store.get_monthly_active_count() - self.assertTrue(self.get_success(count), initial_users) + self.get_success(d) - users = self.store.get_registered_reserved_users() - self.assertEquals(len(self.get_success(users)), reserved_user_number) + count = self.get_success(self.store.get_monthly_active_count()) + self.assertTrue(count, initial_users) - self.get_success(self.store.reap_monthly_active_users()) - count = self.store.get_monthly_active_count() - self.assertEquals(self.get_success(count), self.hs.config.max_mau_value) + users = self.get_success(self.store.get_registered_reserved_users()) + self.assertEqual(len(users), reserved_user_number) + + d = self.store.reap_monthly_active_users() + self.get_success(d) + + count = self.get_success(self.store.get_monthly_active_count()) + self.assertEqual(count, self.hs.config.max_mau_value) def test_populate_monthly_users_is_guest(self): # Test that guest users are not added to mau list user_id = "@user_id:host" - self.store.register_user(user_id=user_id, password_hash=None, make_guest=True) + + d = self.store.register_user( + user_id=user_id, password_hash=None, make_guest=True + ) + self.get_success(d) + self.store.upsert_monthly_active_user = Mock() - self.store.populate_monthly_active_users(user_id) - self.pump() + + d = self.store.populate_monthly_active_users(user_id) + self.get_success(d) + self.store.upsert_monthly_active_user.assert_not_called() def test_populate_monthly_users_should_update(self): @@ -224,8 +245,9 @@ def test_populate_monthly_users_should_update(self): self.store.user_last_seen_monthly_active = Mock( return_value=defer.succeed(None) ) - self.store.populate_monthly_active_users("user_id") - self.pump() + d = self.store.populate_monthly_active_users("user_id") + self.get_success(d) + self.store.upsert_monthly_active_user.assert_called_once() def test_populate_monthly_users_should_not_update(self): @@ -235,16 +257,18 @@ def test_populate_monthly_users_should_not_update(self): self.store.user_last_seen_monthly_active = Mock( return_value=defer.succeed(self.hs.get_clock().time_msec()) ) - self.store.populate_monthly_active_users("user_id") - self.pump() + + d = self.store.populate_monthly_active_users("user_id") + self.get_success(d) + self.store.upsert_monthly_active_user.assert_not_called() def test_get_reserved_real_user_account(self): # Test no reserved users, or reserved threepids users = self.get_success(self.store.get_registered_reserved_users()) - self.assertEquals(len(users), 0) - # Test reserved users but no registered users + self.assertEqual(len(users), 0) + # Test reserved users but no registered users user1 = "@user1:example.com" user2 = "@user2:example.com" @@ -254,63 +278,64 @@ def test_get_reserved_real_user_account(self): {"medium": "email", "address": user1_email}, {"medium": "email", "address": user2_email}, ] + self.hs.config.mau_limits_reserved_threepids = threepids - self.store.db.runInteraction( + d = self.store.db.runInteraction( "initialise", self.store._initialise_reserved_users, threepids ) + self.get_success(d) - self.pump() users = self.get_success(self.store.get_registered_reserved_users()) - self.assertEquals(len(users), 0) + self.assertEqual(len(users), 0) - # Test reserved registed users - self.store.register_user(user_id=user1, password_hash=None) - self.store.register_user(user_id=user2, password_hash=None) - self.pump() + # Test reserved registered users + self.get_success(self.store.register_user(user_id=user1, password_hash=None)) + self.get_success(self.store.register_user(user_id=user2, password_hash=None)) now = int(self.hs.get_clock().time_msec()) self.store.user_add_threepid(user1, "email", user1_email, now, now) self.store.user_add_threepid(user2, "email", user2_email, now, now) users = self.get_success(self.store.get_registered_reserved_users()) - self.assertEquals(len(users), len(threepids)) + self.assertEqual(len(users), len(threepids)) def test_support_user_not_add_to_mau_limits(self): support_user_id = "@support:test" - count = self.store.get_monthly_active_count() - self.pump() - self.assertEqual(self.get_success(count), 0) - self.store.register_user( + count = self.get_success(self.store.get_monthly_active_count()) + self.assertEqual(count, 0) + + d = self.store.register_user( user_id=support_user_id, password_hash=None, user_type=UserTypes.SUPPORT ) + self.get_success(d) - self.store.upsert_monthly_active_user(support_user_id) - count = self.store.get_monthly_active_count() - self.pump() - self.assertEqual(self.get_success(count), 0) + d = self.store.upsert_monthly_active_user(support_user_id) + self.get_success(d) + + d = self.store.get_monthly_active_count() + count = self.get_success(d) + self.assertEqual(count, 0) # Note that the max_mau_value setting should not matter. @override_config( {"limit_usage_by_mau": False, "mau_stats_only": True, "max_mau_value": 1} ) def test_track_monthly_users_without_cap(self): - count = self.store.get_monthly_active_count() - self.assertEqual(0, self.get_success(count)) + count = self.get_success(self.store.get_monthly_active_count()) + self.assertEqual(0, count) - self.store.upsert_monthly_active_user("@user1:server") - self.store.upsert_monthly_active_user("@user2:server") - self.pump() + self.get_success(self.store.upsert_monthly_active_user("@user1:server")) + self.get_success(self.store.upsert_monthly_active_user("@user2:server")) - count = self.store.get_monthly_active_count() - self.assertEqual(2, self.get_success(count)) + count = self.get_success(self.store.get_monthly_active_count()) + self.assertEqual(2, count) @override_config({"limit_usage_by_mau": False, "mau_stats_only": False}) def test_no_users_when_not_tracking(self): self.store.upsert_monthly_active_user = Mock() - self.store.populate_monthly_active_users("@user:sever") - self.pump() + self.get_success(self.store.populate_monthly_active_users("@user:sever")) self.store.upsert_monthly_active_user.assert_not_called() @@ -325,33 +350,39 @@ def test_get_monthly_active_count_by_service(self): service2 = "service2" native = "native" - self.store.register_user( - user_id=appservice1_user1, password_hash=None, appservice_id=service1 + self.get_success( + self.store.register_user( + user_id=appservice1_user1, password_hash=None, appservice_id=service1 + ) + ) + self.get_success( + self.store.register_user( + user_id=appservice1_user2, password_hash=None, appservice_id=service1 + ) ) - self.store.register_user( - user_id=appservice1_user2, password_hash=None, appservice_id=service1 + self.get_success( + self.store.register_user( + user_id=appservice2_user1, password_hash=None, appservice_id=service2 + ) ) - self.store.register_user( - user_id=appservice2_user1, password_hash=None, appservice_id=service2 + self.get_success( + self.store.register_user(user_id=native_user1, password_hash=None) ) - self.store.register_user(user_id=native_user1, password_hash=None) - self.pump() - count = self.store.get_monthly_active_count_by_service() - self.assertEqual({}, self.get_success(count)) + count = self.get_success(self.store.get_monthly_active_count_by_service()) + self.assertEqual(count, {}) - self.store.upsert_monthly_active_user(native_user1) - self.store.upsert_monthly_active_user(appservice1_user1) - self.store.upsert_monthly_active_user(appservice1_user2) - self.store.upsert_monthly_active_user(appservice2_user1) - self.pump() + self.get_success(self.store.upsert_monthly_active_user(native_user1)) + self.get_success(self.store.upsert_monthly_active_user(appservice1_user1)) + self.get_success(self.store.upsert_monthly_active_user(appservice1_user2)) + self.get_success(self.store.upsert_monthly_active_user(appservice2_user1)) - count = self.store.get_monthly_active_count() - self.assertEqual(4, self.get_success(count)) + count = self.get_success(self.store.get_monthly_active_count()) + self.assertEqual(count, 4) - count = self.store.get_monthly_active_count_by_service() - result = self.get_success(count) + d = self.store.get_monthly_active_count_by_service() + result = self.get_success(d) - self.assertEqual(2, result[service1]) - self.assertEqual(1, result[service2]) - self.assertEqual(1, result[native]) + self.assertEqual(result[service1], 2) + self.assertEqual(result[service2], 1) + self.assertEqual(result[native], 1) diff --git a/tests/test_mau.py b/tests/test_mau.py index 8a97f0998dbc..49667ed7f477 100644 --- a/tests/test_mau.py +++ b/tests/test_mau.py @@ -85,7 +85,7 @@ def test_allowed_after_a_month_mau(self): # Advance time by 31 days self.reactor.advance(31 * 24 * 60 * 60) - self.store.reap_monthly_active_users() + self.get_success(self.store.reap_monthly_active_users()) self.reactor.advance(0) @@ -147,8 +147,7 @@ def test_trial_users_cant_come_back(self): # Advance by 2 months so everyone falls out of MAU self.reactor.advance(60 * 24 * 60 * 60) - self.store.reap_monthly_active_users() - self.reactor.advance(0) + self.get_success(self.store.reap_monthly_active_users()) # We can create as many new users as we want token4 = self.create_user("kermit4") From 6386fd0b09676c0bb62c0424bc23fa8bd2e1144f Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Tue, 2 Jun 2020 16:27:22 +0100 Subject: [PATCH 2/5] get_registered_reserved_users, reap_monthly_active_users --- .../data_stores/main/monthly_active_users.py | 25 +++++++++---------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/synapse/storage/data_stores/main/monthly_active_users.py b/synapse/storage/data_stores/main/monthly_active_users.py index 1310d3906934..e459cf49a0b1 100644 --- a/synapse/storage/data_stores/main/monthly_active_users.py +++ b/synapse/storage/data_stores/main/monthly_active_users.py @@ -13,6 +13,7 @@ # See the License for the specific language governing permissions and # limitations under the License. import logging +from typing import List from twisted.internet import defer @@ -77,20 +78,19 @@ def _count_users_by_service(txn): return self.db.runInteraction("count_users_by_service", _count_users_by_service) - @defer.inlineCallbacks - def get_registered_reserved_users(self): - """Of the reserved threepids defined in config, which are associated - with registered users? + async def get_registered_reserved_users(self) -> List[str]: + """Of the reserved threepids defined in config, retrieve those that are associated + with registered users Returns: - Defered[list]: Real reserved users + User IDs of actual users that are reserved """ users = [] for tp in self.hs.config.mau_limits_reserved_threepids[ : self.hs.config.max_mau_value ]: - user_id = yield self.hs.get_datastore().get_user_id_by_threepid( + user_id = await self.hs.get_datastore().get_user_id_by_threepid( tp["medium"], tp["address"] ) if user_id: @@ -171,13 +171,9 @@ def _initialise_reserved_users(self, txn, threepids): else: logger.warning("mau limit reserved threepid %s not found in db" % tp) - @defer.inlineCallbacks - def reap_monthly_active_users(self): + async def reap_monthly_active_users(self): """Cleans out monthly active user table to ensure that no stale entries exist. - - Returns: - Deferred[] """ def _reap_users(txn, reserved_users): @@ -249,8 +245,8 @@ def _reap_users(txn, reserved_users): ) self._invalidate_cache_and_stream(txn, self.get_monthly_active_count, ()) - reserved_users = yield self.get_registered_reserved_users() - yield self.db.runInteraction( + reserved_users = await self.get_registered_reserved_users() + await self.db.runInteraction( "reap_monthly_active_users", _reap_users, reserved_users ) @@ -261,6 +257,9 @@ def upsert_monthly_active_user(self, user_id): Args: user_id (str): user to add/update + + Returns: + Deferred """ # Support user never to be included in MAU stats. Note I can't easily call this # from upsert_monthly_active_user_txn because then I need a _txn form of From d08985527e90753f5ecafa3382977921eb108a64 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Tue, 2 Jun 2020 16:26:58 +0100 Subject: [PATCH 3/5] generate_monthly_active_users --- synapse/app/homeserver.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/synapse/app/homeserver.py b/synapse/app/homeserver.py index 93a5ba2100b9..730a2c015b9e 100644 --- a/synapse/app/homeserver.py +++ b/synapse/app/homeserver.py @@ -617,18 +617,17 @@ def reap_monthly_active_users(): clock.looping_call(reap_monthly_active_users, 1000 * 60 * 60) reap_monthly_active_users() - @defer.inlineCallbacks - def generate_monthly_active_users(): + async def generate_monthly_active_users(): current_mau_count = 0 current_mau_count_by_service = {} reserved_users = () store = hs.get_datastore() if hs.config.limit_usage_by_mau or hs.config.mau_stats_only: - current_mau_count = yield store.get_monthly_active_count() + current_mau_count = await store.get_monthly_active_count() current_mau_count_by_service = ( - yield store.get_monthly_active_count_by_service() + await store.get_monthly_active_count_by_service() ) - reserved_users = yield store.get_registered_reserved_users() + reserved_users = await store.get_registered_reserved_users() current_mau_gauge.set(float(current_mau_count)) for app_service, count in current_mau_count_by_service.items(): From 968e2874471a905283ed7b9c8efe891b9f351372 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Tue, 2 Jun 2020 16:26:41 +0100 Subject: [PATCH 4/5] get_user_id_by_threepid --- .../storage/data_stores/main/registration.py | 22 ++++++++++--------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/synapse/storage/data_stores/main/registration.py b/synapse/storage/data_stores/main/registration.py index efcdd2100b8f..9768981891fd 100644 --- a/synapse/storage/data_stores/main/registration.py +++ b/synapse/storage/data_stores/main/registration.py @@ -17,6 +17,7 @@ import logging import re +from typing import Optional from six import iterkeys @@ -342,7 +343,7 @@ def is_real_user(self, user_id): ) return res - @cachedInlineCallbacks() + @cached() def is_support_user(self, user_id): """Determines if the user is of type UserTypes.SUPPORT @@ -352,10 +353,9 @@ def is_support_user(self, user_id): Returns: Deferred[bool]: True if user is of type UserTypes.SUPPORT """ - res = yield self.db.runInteraction( + return self.db.runInteraction( "is_support_user", self.is_support_user_txn, user_id ) - return res def is_real_user_txn(self, txn, user_id): res = self.db.simple_select_one_onecol_txn( @@ -516,18 +516,17 @@ def _find_next_generated_user_id(txn): ) ) - @defer.inlineCallbacks - def get_user_id_by_threepid(self, medium, address): + async def get_user_id_by_threepid(self, medium: str, address: str) -> Optional[str]: """Returns user id from threepid Args: - medium (str): threepid medium e.g. email - address (str): threepid address e.g. me@example.com + medium: threepid medium e.g. email + address: threepid address e.g. me@example.com Returns: - Deferred[str|None]: user id or None if no user id/threepid mapping exists + The user ID or None if no user id/threepid mapping exists """ - user_id = yield self.db.runInteraction( + user_id = await self.db.runInteraction( "get_user_id_by_threepid", self.get_user_id_by_threepid_txn, medium, address ) return user_id @@ -993,7 +992,7 @@ def register_user( Args: user_id (str): The desired user ID to register. - password_hash (str): Optional. The password hash for this user. + password_hash (str|None): Optional. The password hash for this user. was_guest (bool): Optional. Whether this is a guest account being upgraded to a non-guest account. make_guest (boolean): True if the the new user should be guest, @@ -1007,6 +1006,9 @@ def register_user( Raises: StoreError if the user_id could not be registered. + + Returns: + Deferred """ return self.db.runInteraction( "register_user", From f9a5111569911717a7825a8f4bf4db80740e4891 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Tue, 2 Jun 2020 16:59:32 +0100 Subject: [PATCH 5/5] changelog --- changelog.d/7620.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/7620.misc diff --git a/changelog.d/7620.misc b/changelog.d/7620.misc new file mode 100644 index 000000000000..f8357ee3b61f --- /dev/null +++ b/changelog.d/7620.misc @@ -0,0 +1 @@ +Convert `get_user_id_by_threepid` to async/await. \ No newline at end of file