diff --git a/changelog.d/18886.misc b/changelog.d/18886.misc new file mode 100644 index 000000000..d0d32e59a --- /dev/null +++ b/changelog.d/18886.misc @@ -0,0 +1 @@ +Start background tasks after we fork the process (daemonize). diff --git a/synapse/_scripts/update_synapse_database.py b/synapse/_scripts/update_synapse_database.py index 70e559841..3624db354 100644 --- a/synapse/_scripts/update_synapse_database.py +++ b/synapse/_scripts/update_synapse_database.py @@ -120,6 +120,13 @@ def main() -> None: # DB. hs.setup() + # This will cause all of the relevant storage classes to be instantiated and call + # `register_background_update_handler(...)`, + # `register_background_index_update(...)`, + # `register_background_validate_constraint(...)`, etc so they are available to use + # if we are asked to run those background updates. + hs.get_storage_controllers() + if args.run_background_updates: run_background_updates(hs) diff --git a/synapse/app/_base.py b/synapse/app/_base.py index 48989540b..bce6f4d82 100644 --- a/synapse/app/_base.py +++ b/synapse/app/_base.py @@ -609,10 +609,28 @@ async def start(hs: "HomeServer") -> None: setup_sentry(hs) setup_sdnotify(hs) - # If background tasks are running on the main process or this is the worker in - # charge of them, start collecting the phone home stats and shared usage metrics. + # Register background tasks required by this server. This must be done + # somewhat manually due to the background tasks not being registered + # unless handlers are instantiated. + # + # While we could "start" these before the reactor runs, nothing will happen until + # the reactor is running, so we may as well do it here in `start`. + # + # Additionally, this means we also start them after we daemonize and fork the + # process which means we can avoid any potential problems with cputime metrics + # getting confused about the per-thread resource usage appearing to go backwards + # because we're comparing the resource usage (`rusage`) from the original process to + # the forked process. if hs.config.worker.run_background_tasks: + hs.start_background_tasks() + + # TODO: This should be moved to same pattern we use for other background tasks: + # Add to `REQUIRED_ON_BACKGROUND_TASK_STARTUP` and rely on + # `start_background_tasks` to start it. await hs.get_common_usage_metrics_manager().setup() + + # TODO: This feels like another pattern that should refactored as one of the + # `REQUIRED_ON_BACKGROUND_TASK_STARTUP` start_phone_stats_home(hs) # We now freeze all allocated objects in the hopes that (almost) diff --git a/synapse/server.py b/synapse/server.py index 3eac271c9..3fb29a781 100644 --- a/synapse/server.py +++ b/synapse/server.py @@ -366,12 +366,6 @@ class HomeServer(metaclass=abc.ABCMeta): self.datastores = Databases(self.DATASTORE_CLASS, self) logger.info("Finished setting up.") - # Register background tasks required by this server. This must be done - # somewhat manually due to the background tasks not being registered - # unless handlers are instantiated. - if self.config.worker.run_background_tasks: - self.setup_background_tasks() - def __del__(self) -> None: """ Called when an the homeserver is garbage collected. @@ -410,7 +404,7 @@ class HomeServer(metaclass=abc.ABCMeta): appropriate listeners. """ - def setup_background_tasks(self) -> None: + def start_background_tasks(self) -> None: """ Some handlers have side effects on instantiation (like registering background updates). This function causes them to be fetched, and diff --git a/tests/server.py b/tests/server.py index ebff8b04b..7432db1ac 100644 --- a/tests/server.py +++ b/tests/server.py @@ -1160,6 +1160,16 @@ def setup_test_homeserver( with patch("synapse.storage.database.make_pool", side_effect=make_fake_db_pool): hs.setup() + # Register background tasks required by this server. This must be done + # somewhat manually due to the background tasks not being registered + # unless handlers are instantiated. + # + # Since, we don't have to worry about `daemonize` (forking the process) in tests, we + # can just start the background tasks straight away after `hs.setup`. (compare this + # with where we call `hs.start_background_tasks()` outside of the test environment). + if hs.config.worker.run_background_tasks: + hs.start_background_tasks() + # Since we've changed the databases to run DB transactions on the same # thread, we need to stop the event fetcher hogging that one thread. hs.get_datastores().main.USE_DEDICATED_DB_THREADS_FOR_EVENT_FETCHING = False