From 0491be6d97b08d42d89b92ea7d540e1c16aed3fb Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 4 Oct 2021 18:40:30 +0100 Subject: [PATCH 1/5] Pull out `wait_for_background_update` --- tests/handlers/test_stats.py | 21 +++------------------ tests/storage/databases/main/test_room.py | 7 +------ tests/storage/test_cleanup_extrems.py | 7 +------ tests/storage/test_client_ips.py | 21 +++------------------ tests/storage/test_event_chain.py | 14 ++------------ tests/storage/test_roommember.py | 14 ++------------ tests/storage/test_user_directory.py | 7 +------ tests/unittest.py | 9 +++++++++ 8 files changed, 22 insertions(+), 78 deletions(-) diff --git a/tests/handlers/test_stats.py b/tests/handlers/test_stats.py index 24b7ef6efcf8..56207f4db6f6 100644 --- a/tests/handlers/test_stats.py +++ b/tests/handlers/test_stats.py @@ -103,12 +103,7 @@ def _perform_background_initial_update(self): # Do the initial population of the stats via the background update self._add_background_updates() - while not self.get_success( - self.store.db_pool.updates.has_completed_background_updates() - ): - self.get_success( - self.store.db_pool.updates.do_next_background_update(100), by=0.1 - ) + self.wait_for_background_updates() def test_initial_room(self): """ @@ -140,12 +135,7 @@ def test_initial_room(self): # Do the initial population of the user directory via the background update self._add_background_updates() - while not self.get_success( - self.store.db_pool.updates.has_completed_background_updates() - ): - self.get_success( - self.store.db_pool.updates.do_next_background_update(100), by=0.1 - ) + self.wait_for_background_updates() r = self.get_success(self.get_all_room_state()) @@ -568,12 +558,7 @@ def test_incomplete_stats(self): ) ) - while not self.get_success( - self.store.db_pool.updates.has_completed_background_updates() - ): - self.get_success( - self.store.db_pool.updates.do_next_background_update(100), by=0.1 - ) + self.wait_for_background_updates() r1stats_complete = self._get_current_stats("room", r1) u1stats_complete = self._get_current_stats("user", u1) diff --git a/tests/storage/databases/main/test_room.py b/tests/storage/databases/main/test_room.py index ffee70715342..7496974da3a8 100644 --- a/tests/storage/databases/main/test_room.py +++ b/tests/storage/databases/main/test_room.py @@ -79,12 +79,7 @@ def test_background_populate_rooms_creator_column(self): self.store.db_pool.updates._all_done = False # Now let's actually drive the updates to completion - while not self.get_success( - self.store.db_pool.updates.has_completed_background_updates() - ): - self.get_success( - self.store.db_pool.updates.do_next_background_update(100), by=0.1 - ) + self.wait_for_background_updates() # Make sure the background update filled in the room creator room_creator_after = self.get_success( diff --git a/tests/storage/test_cleanup_extrems.py b/tests/storage/test_cleanup_extrems.py index 7cc5e621ba9a..a59c28f89681 100644 --- a/tests/storage/test_cleanup_extrems.py +++ b/tests/storage/test_cleanup_extrems.py @@ -66,12 +66,7 @@ def run_delta_file(txn): # Ugh, have to reset this flag self.store.db_pool.updates._all_done = False - while not self.get_success( - self.store.db_pool.updates.has_completed_background_updates() - ): - self.get_success( - self.store.db_pool.updates.do_next_background_update(100), by=0.1 - ) + self.wait_for_background_updates() def test_soft_failed_extremities_handled_correctly(self): """Test that extremities are correctly calculated in the presence of diff --git a/tests/storage/test_client_ips.py b/tests/storage/test_client_ips.py index 1c2df54ecc53..c4f0a491e90e 100644 --- a/tests/storage/test_client_ips.py +++ b/tests/storage/test_client_ips.py @@ -208,12 +208,7 @@ def test_updating_monthly_active_user_when_space(self): def test_devices_last_seen_bg_update(self): # First make sure we have completed all updates. - while not self.get_success( - self.store.db_pool.updates.has_completed_background_updates() - ): - self.get_success( - self.store.db_pool.updates.do_next_background_update(100), by=0.1 - ) + self.wait_for_background_updates() user_id = "@user:id" device_id = "MY_DEVICE" @@ -277,12 +272,7 @@ def test_devices_last_seen_bg_update(self): self.store.db_pool.updates._all_done = False # Now let's actually drive the updates to completion - while not self.get_success( - self.store.db_pool.updates.has_completed_background_updates() - ): - self.get_success( - self.store.db_pool.updates.do_next_background_update(100), by=0.1 - ) + self.wait_for_background_updates() # We should now get the correct result again result = self.get_success( @@ -303,12 +293,7 @@ def test_devices_last_seen_bg_update(self): def test_old_user_ips_pruned(self): # First make sure we have completed all updates. - while not self.get_success( - self.store.db_pool.updates.has_completed_background_updates() - ): - self.get_success( - self.store.db_pool.updates.do_next_background_update(100), by=0.1 - ) + self.wait_for_background_updates() user_id = "@user:id" device_id = "MY_DEVICE" diff --git a/tests/storage/test_event_chain.py b/tests/storage/test_event_chain.py index 93136f071793..b31c5eb5ecc6 100644 --- a/tests/storage/test_event_chain.py +++ b/tests/storage/test_event_chain.py @@ -578,12 +578,7 @@ def test_background_update_single_room(self): # Ugh, have to reset this flag self.store.db_pool.updates._all_done = False - while not self.get_success( - self.store.db_pool.updates.has_completed_background_updates() - ): - self.get_success( - self.store.db_pool.updates.do_next_background_update(100), by=0.1 - ) + self.wait_for_background_updates() # Test that the `has_auth_chain_index` has been set self.assertTrue(self.get_success(self.store.has_auth_chain_index(room_id))) @@ -619,12 +614,7 @@ def test_background_update_multiple_rooms(self): # Ugh, have to reset this flag self.store.db_pool.updates._all_done = False - while not self.get_success( - self.store.db_pool.updates.has_completed_background_updates() - ): - self.get_success( - self.store.db_pool.updates.do_next_background_update(100), by=0.1 - ) + self.wait_for_background_updates() # Test that the `has_auth_chain_index` has been set self.assertTrue(self.get_success(self.store.has_auth_chain_index(room_id1))) diff --git a/tests/storage/test_roommember.py b/tests/storage/test_roommember.py index c72dc40510a4..2873e22ccf8c 100644 --- a/tests/storage/test_roommember.py +++ b/tests/storage/test_roommember.py @@ -169,12 +169,7 @@ def prepare(self, reactor, clock, homeserver): def test_can_rerun_update(self): # First make sure we have completed all updates. - while not self.get_success( - self.store.db_pool.updates.has_completed_background_updates() - ): - self.get_success( - self.store.db_pool.updates.do_next_background_update(100), by=0.1 - ) + self.wait_for_background_updates() # Now let's create a room, which will insert a membership user = UserID("alice", "test") @@ -197,9 +192,4 @@ def test_can_rerun_update(self): self.store.db_pool.updates._all_done = False # Now let's actually drive the updates to completion - while not self.get_success( - self.store.db_pool.updates.has_completed_background_updates() - ): - self.get_success( - self.store.db_pool.updates.do_next_background_update(100), by=0.1 - ) + self.wait_for_background_updates() diff --git a/tests/storage/test_user_directory.py b/tests/storage/test_user_directory.py index 6884ca9b7a3f..edf68c61b55c 100644 --- a/tests/storage/test_user_directory.py +++ b/tests/storage/test_user_directory.py @@ -175,12 +175,7 @@ def _purge_and_rebuild_user_dir(self) -> None: ) ) - while not self.get_success( - self.store.db_pool.updates.has_completed_background_updates() - ): - self.get_success( - self.store.db_pool.updates.do_next_background_update(100), by=0.1 - ) + self.wait_for_background_updates() def test_initial(self) -> None: """ diff --git a/tests/unittest.py b/tests/unittest.py index ae393ee53eee..506fa8364644 100644 --- a/tests/unittest.py +++ b/tests/unittest.py @@ -317,6 +317,15 @@ def wait_on_thread(self, deferred, timeout=10): self.reactor.advance(0.01) time.sleep(0.01) + def wait_for_background_updates(self) -> None: + """Block until we've written completed all background updates.""" + while not self.get_success( + self.store.db_pool.updates.has_completed_background_updates() + ): + self.get_success( + self.store.db_pool.updates.do_next_background_update(100), by=0.1 + ) + def make_homeserver(self, reactor, clock): """ Make and return a homeserver. From ce91ef5e8196fe83f4fda2bb80a4c84a13514534 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 4 Oct 2021 18:43:11 +0100 Subject: [PATCH 2/5] Don't drop user dir deltas when server leaves room Fix a long-standing bug where a batch of user directory changes would be silently dropped if the server left a room early in the batch. --- synapse/handlers/user_directory.py | 2 +- tests/handlers/test_user_directory.py | 39 +++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/synapse/handlers/user_directory.py b/synapse/handlers/user_directory.py index 18d8c8744e75..97f60b58068a 100644 --- a/synapse/handlers/user_directory.py +++ b/synapse/handlers/user_directory.py @@ -220,7 +220,7 @@ async def _handle_deltas(self, deltas: List[Dict[str, Any]]) -> None: for user_id in user_ids: await self._handle_remove_user(room_id, user_id) - return + continue else: logger.debug("Server is still in room: %r", room_id) diff --git a/tests/handlers/test_user_directory.py b/tests/handlers/test_user_directory.py index b3c3af113b28..03fd5a3e2c52 100644 --- a/tests/handlers/test_user_directory.py +++ b/tests/handlers/test_user_directory.py @@ -363,6 +363,45 @@ def test_reactivation_makes_regular_user_searchable(self) -> None: self.assertEqual(len(s["results"]), 1) self.assertEqual(s["results"][0]["user_id"], user) + def test_process_join_after_server_leaves_room(self) -> None: + alice = self.register_user("alice", "pass") + alice_token = self.login(alice, "pass") + bob = self.register_user("bob", "pass") + bob_token = self.login(bob, "pass") + + # Alice makes two rooms. Bob joins one of them. + room1 = self.helper.create_room_as(alice, tok=alice_token) + room2 = self.helper.create_room_as(alice, tok=alice_token) + print("room1=", room1) + print("room2=", room2) + self.helper.join(room1, bob, tok=bob_token) + + # The user sharing tables should have been updated. + public1 = self.get_success(self.user_dir_helper.get_users_in_public_rooms()) + self.assertEqual(set(public1), {(alice, room1), (alice, room2), (bob, room1)}) + + # Alice leaves room1. The user sharing tables should be updated. + self.helper.leave(room1, alice, tok=alice_token) + public2 = self.get_success(self.user_dir_helper.get_users_in_public_rooms()) + self.assertEqual(set(public2), {(alice, room2), (bob, room1)}) + + # Pause the processing of new events. + dir_handler = self.hs.get_user_directory_handler() + dir_handler.update_user_directory = False + + # Bob leaves one room and joins the other. + self.helper.leave(room1, bob, tok=bob_token) + self.helper.join(room2, bob, tok=bob_token) + + # Process the leave and join in one go. + dir_handler.update_user_directory = True + dir_handler.notify_new_event() + self.wait_for_background_updates() + + # The user sharing tables should have been updated. + public3 = self.get_success(self.user_dir_helper.get_users_in_public_rooms()) + self.assertEqual(set(public3), {(alice, room2), (bob, room2)}) + def test_private_room(self) -> None: """ A user can be searched for only by people that are either in a public From c11803591981f322cbcba5139904cc92b5a24061 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 4 Oct 2021 18:54:31 +0100 Subject: [PATCH 3/5] Changelog --- changelog.d/10982.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/10982.bugfix diff --git a/changelog.d/10982.bugfix b/changelog.d/10982.bugfix new file mode 100644 index 000000000000..5c9e15eeaa42 --- /dev/null +++ b/changelog.d/10982.bugfix @@ -0,0 +1 @@ +Fix a long-standing bug where the remainder of a batch of user directory changes would be silently dropped if the server left a room early in the batch. \ No newline at end of file From fa09db2e222895b9523eccee08f90866643a2fec Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 4 Oct 2021 19:39:37 +0100 Subject: [PATCH 4/5] Make `wait_for_background_updates` docstring comprehensible Thanks Patrick! Co-authored-by: Patrick Cloke --- tests/unittest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unittest.py b/tests/unittest.py index 506fa8364644..8e3fcb18755a 100644 --- a/tests/unittest.py +++ b/tests/unittest.py @@ -318,7 +318,7 @@ def wait_on_thread(self, deferred, timeout=10): time.sleep(0.01) def wait_for_background_updates(self) -> None: - """Block until we've written completed all background updates.""" + """Block until all background updates have completed.""" while not self.get_success( self.store.db_pool.updates.has_completed_background_updates() ): From 857aa46b9f46f69f28ea2437fb52df9f1479d3be Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 6 Oct 2021 13:30:18 +0100 Subject: [PATCH 5/5] Update tests/unittest.py Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- tests/unittest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unittest.py b/tests/unittest.py index 8e3fcb18755a..81c1a9e9d2d2 100644 --- a/tests/unittest.py +++ b/tests/unittest.py @@ -318,7 +318,7 @@ def wait_on_thread(self, deferred, timeout=10): time.sleep(0.01) def wait_for_background_updates(self) -> None: - """Block until all background updates have completed.""" + """Block until all background database updates have completed.""" while not self.get_success( self.store.db_pool.updates.has_completed_background_updates() ):