Run homeserver shutdown handlers at the end of each test (#20509)

This fixes the test `ThreadedMemoryReactorClock` to support
`removeSystemEventTrigger`, which in turn allows the shutdown triggers
to run when `HomeServer.shutdown()` is called in the tests. Previously,
`ThreadedMemoryReactorClock.removeSystemEventTrigger` raised a
`NotImplementedError`,

`cleanup_test_reactor_system_event_triggers` cleared the triggers by
hand so that a homeserver could be garbage collected. `shutdown()` now
removes them itself, so the helper goes.

We can also remove all the `tearDown` implementations that tried to
manually trigger the shutdown as its not longer necessary to do so
manually. They also didn't work, as they indexed `reactor.triggers` as
`[eventType][phase]` when the keys are `[phase][eventType]`

changelog.d/20509.misc

@@ -0,0 +1 @@
+Fix running homeserver shutdown handlers at the end of each test.

tests/app/test_homeserver_shutdown.py

@@ -29,7 +29,6 @@ from synapse.logging.context import LoggingContext
 from synapse.storage.background_updates import UpdaterStatus
 
 from tests.server import (
-    cleanup_test_reactor_system_event_triggers,
     get_clock,
     setup_test_homeserver,
 )
@@ -69,9 +68,6 @@ class HomeserverCleanShutdownTestCase(HomeserverTestCase):
 
         # Run the reactor so any `callWhenRunning` functions can be cleared out.
         self.reactor.run()
-        # This would normally happen as part of `HomeServer.shutdown` but the `MemoryReactor`
-        # we use in tests doesn't handle this properly (see doc comment)
-        cleanup_test_reactor_system_event_triggers(self.reactor)
 
         async def shutdown() -> None:
             # Use a logcontext just to double-check that we don't mangle the logcontext
@@ -127,7 +123,6 @@ class HomeserverCleanShutdownTestCase(HomeserverTestCase):
 
         # The same shutdown sequence as `test_clean_homeserver_shutdown`.
         self.reactor.run()
-        cleanup_test_reactor_system_event_triggers(self.reactor)
         self.get_success(self.hs.shutdown())
         self.reactor.advance(0)
         del self.hs
@@ -162,9 +157,6 @@ class HomeserverCleanShutdownTestCase(HomeserverTestCase):
 
         # Run the reactor so any `callWhenRunning` functions can be cleared out.
         self.reactor.run()
-        # This would normally happen as part of `HomeServer.shutdown` but the `MemoryReactor`
-        # we use in tests doesn't handle this properly (see doc comment)
-        cleanup_test_reactor_system_event_triggers(self.reactor)
 
         # Ensure the background updates are not complete.
         self.assertNotEqual(store.db_pool.updates.get_status(), UpdaterStatus.COMPLETE)
@@ -226,9 +218,6 @@ class HomeserverCleanShutdownTestCase(HomeserverTestCase):
 
         # Run the reactor so any `callWhenRunning` functions can be cleared out.
         self.reactor.run()
-        # This would normally happen as part of `HomeServer.shutdown` but the `MemoryReactor`
-        # we use in tests doesn't handle this properly (see doc comment)
-        cleanup_test_reactor_system_event_triggers(self.reactor)
 
         async def shutdown() -> None:
             # Use a logcontext just to double-check that we don't mangle the logcontext

tests/handlers/test_oauth_delegation.py

@@ -231,14 +231,6 @@ class MasAuthDelegation(HomeserverTestCase):
 
     def tearDown(self) -> None:
         self.server.shutdown()
-        # MemoryReactor doesn't trigger the shutdown phases, and we want the
-        # Tokio thread pool to be stopped
-        # XXX: This logic should probably get moved somewhere else
-        shutdown_triggers = self.reactor.triggers.get("shutdown", {})
-        for phase in ["before", "during", "after"]:
-            triggers = shutdown_triggers.get(phase, [])
-            for callbable, args, kwargs in triggers:
-                callbable(*args, **kwargs)
 
     def test_simple_introspection(self) -> None:
         self.server.introspection_response = {

tests/push/test_http.py

@@ -1255,16 +1255,6 @@ class MSC3881VersionsTestCase(HomeserverTestCase):
         versions.register_servlets,
     ]
 
-    def tearDown(self) -> None:
-        # MemoryReactor doesn't trigger the shutdown phases, and we want the
-        # Tokio thread pool to be stopped
-        # XXX: This logic should probably get moved somewhere else
-        shutdown_triggers = self.reactor.triggers.get("shutdown", {})
-        for phase in ["before", "during", "after"]:
-            triggers = shutdown_triggers.get(phase, [])
-            for callbable, args, kwargs in triggers:
-                callbable(*args, **kwargs)
-
     def test_msc3881_client_versions_flag(self) -> None:
         """Tests that MSC3881 only appears in /versions if user has it enabled."""
 

tests/rest/client/test_delayed_events.py

@@ -63,16 +63,6 @@ _EVENT_TYPE = "com.example.test"
 class DelayedEventsUnstableSupportTestCase(HomeserverTestCase):
     servlets = [versions.register_servlets]
 
-    def tearDown(self) -> None:
-        # MemoryReactor doesn't trigger the shutdown phases, and we want the
-        # Tokio thread pool to be stopped
-        # XXX: This logic should probably get moved somewhere else
-        shutdown_triggers = self.reactor.triggers.get("shutdown", {})
-        for phase in ["before", "during", "after"]:
-            triggers = shutdown_triggers.get(phase, [])
-            for callbable, args, kwargs in triggers:
-                callbable(*args, **kwargs)
-
     def test_false_by_default(self) -> None:
         channel = self.make_request("GET", "/_matrix/client/versions")
         self.assertEqual(channel.code, 200, channel.result)

tests/rest/client/test_login_token_request.py

@@ -52,16 +52,6 @@ class LoginTokenRequestServletTestCase(unittest.HomeserverTestCase):
 
         return self.hs
 
-    def tearDown(self) -> None:
-        # MemoryReactor doesn't trigger the shutdown phases, and we want the
-        # Tokio thread pool to be stopped
-        # XXX: This logic should probably get moved somewhere else
-        shutdown_triggers = self.reactor.triggers.get("shutdown", {})
-        for phase in ["before", "during", "after"]:
-            triggers = shutdown_triggers.get(phase, [])
-            for callbable, args, kwargs in triggers:
-                callbable(*args, **kwargs)
-
     def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None:
         self.user = "user123"
         self.password = "password"

tests/rest/client/test_matrixrtc.py

@@ -250,16 +250,6 @@ class MatrixRtcVersionsTestCase(HomeserverTestCase):
 
     servlets = [versions.register_servlets]
 
-    def tearDown(self) -> None:
-        # MemoryReactor doesn't trigger the shutdown phases, and we want the
-        # Tokio thread pool to be stopped
-        # XXX: This logic should probably get moved somewhere else
-        shutdown_triggers = self.reactor.triggers.get("shutdown", {})
-        for phase in ["before", "during", "after"]:
-            triggers = shutdown_triggers.get(phase, [])
-            for callbable, args, kwargs in triggers:
-                callbable(*args, **kwargs)
-
     def test_msc4143_false_by_default(self) -> None:
         channel = self.make_request("GET", "/_matrix/client/versions")
         self.assertEqual(channel.code, 200, channel.result)

tests/rest/client/test_versions.py

@@ -36,16 +36,6 @@ class VersionsTestCase(unittest.HomeserverTestCase):
         versions.register_servlets,
     ]
 
-    def tearDown(self) -> None:
-        # MemoryReactor doesn't trigger the shutdown phases, and we want the
-        # Tokio thread pool to be stopped
-        # XXX: This logic should probably get moved somewhere else
-        shutdown_triggers = self.reactor.triggers.get("shutdown", {})
-        for phase in ["before", "during", "after"]:
-            triggers = shutdown_triggers.get(phase, [])
-            for callbable, args, kwargs in triggers:
-                callbable(*args, **kwargs)
-
     def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None:
         self.admin_user = self.register_user("admin", "pass", admin=True)
         self.admin_user_tok = self.login("admin", "pass")

tests/server.py

@@ -639,6 +639,37 @@ class ThreadedMemoryReactorClock(MemoryReactorClock):
         # So manually clear the hooks here after they have been run.
         self.whenRunningHooks.clear()
 
+    @override
+    def addSystemEventTrigger(
+        self,
+        phase: str,
+        eventType: str,
+        callable: Callable[P, object],
+        *args: P.args,
+        **kw: P.kwargs,
+    ) -> Any:
+        """
+        Override the call from `MemoryReactor` to return an ID that
+        `removeSystemEventTrigger` accepts. `MemoryReactor` returns `None`.
+        """
+        super().addSystemEventTrigger(phase, eventType, callable, *args, **kw)
+        return (phase, eventType, self.triggers[phase][eventType][-1])
+
+    @override
+    def removeSystemEventTrigger(self, triggerID: Any) -> None:
+        """
+        Override the call from `MemoryReactor`, which raises
+        `NotImplementedError`.
+
+        `HomeServer.shutdown()` removes each shutdown handler's trigger before it
+        calls the handler, and skips the handler if the removal fails. Without
+        this, no shutdown handler would ever run in a test. One of them stops
+        the homeserver's Tokio runtime, which would otherwise carry on running
+        its tasks during later tests.
+        """
+        phase, eventType, trigger = triggerID
+        self.triggers[phase][eventType].remove(trigger)
+
     def installNameResolver(self, resolver: IHostnameResolver) -> IHostnameResolver:
         raise NotImplementedError()
 
@@ -792,19 +823,6 @@ class ThreadedMemoryReactorClock(MemoryReactorClock):
         return [readers[fileno] for fileno, _event in poller.poll(0)]
 
 
-def cleanup_test_reactor_system_event_triggers(
-    reactor: ThreadedMemoryReactorClock,
-) -> None:
-    """Cleanup any registered system event triggers.
-    The `twisted.internet.test.ThreadedMemoryReactor` does not implement
-    `removeSystemEventTrigger` so won't clean these triggers up on it's own properly.
-    When trying to override `removeSystemEventTrigger` in `ThreadedMemoryReactorClock`
-    in order to implement this functionality, twisted complains about the reactor being
-    unclean and fails some tests.
-    """
-    reactor.triggers.clear()
-
-
 def validate_connector(connector: tcp.Connector, expected_ip: str) -> None:
     """Try to validate the obtained connector as it would happen when
     synapse is running and the conection will be established.

tests/synapse_rust/test_http_client.py

@@ -95,16 +95,6 @@ class HttpClientTestCase(HomeserverTestCase):
 
         self.server = StubServer()
 
-    def tearDown(self) -> None:
-        # MemoryReactor doesn't trigger the shutdown phases, and we want the
-        # Tokio thread pool to be stopped
-        # XXX: This logic should probably get moved somewhere else
-        shutdown_triggers = self.reactor.triggers.get("shutdown", {})
-        for phase in ["before", "during", "after"]:
-            triggers = shutdown_triggers.get(phase, [])
-            for callbable, args, kwargs in triggers:
-                callbable(*args, **kwargs)
-
     def _check_current_logcontext(self, expected_logcontext_string: str) -> None:
         context = current_context()
         assert isinstance(context, LoggingContext) or isinstance(context, _Sentinel), (

tests/synapse_rust/test_logcontext.py

@@ -91,15 +91,6 @@ class RustLogContextTestCase(HomeserverTestCase):
 
         return hs
 
-    def tearDown(self) -> None:
-        # MemoryReactor doesn't trigger the shutdown phases, and we want the Tokio
-        # thread pool to be stopped.
-        shutdown_triggers = self.reactor.triggers.get("shutdown", {})
-        for phase in ["before", "during", "after"]:
-            triggers = shutdown_triggers.get(phase, [])
-            for callbable, args, kwargs in triggers:
-                callbable(*args, **kwargs)
-
     def prepare(
         self, reactor: MemoryReactor, clock: Clock, homeserver: HomeServer
     ) -> None: