Start background tasks after we fork the process (daemonize) (#18886)

Spawning from #18871

[This change](6ce2f3e)
was originally used to fix CPU time going backwards when we `daemonize`.

While, we don't seem to run into this problem on `develop`, I still
think this is a good change to make. We don't need background tasks
running on a process that will soon be forcefully exited and where the
reactor isn't even running yet. We now kick off the background tasks
(`run_as_background_process`) after we have forked the process and
started the reactor.

Also as simple note, we don't need background tasks running in both halves of a fork.

Author: MadLittleMods

changelog.d/18886.misc

@@ -0,0 +1 @@
+Start background tasks after we fork the process (daemonize).

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)
 

synapse/app/_base.py

@@ -609,10 +609,28 @@ def run_sighup(*args: Any, **kwargs: Any) -> 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)

synapse/server.py

@@ -366,12 +366,6 @@ def setup(self) -> None:
         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 @@ def start_listening(self) -> None:  # noqa: B027 (no-op by design)
         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

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