From 3bbbbf5d04b46ce714ff4ce31f0c1b647c5745a3 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 13 Nov 2026 10:09:16 +0000 Subject: [PATCH] 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 | 1 + tests/app/test_homeserver_shutdown.py | 11 ----- tests/handlers/test_oauth_delegation.py | 8 ---- tests/push/test_http.py | 10 ----- tests/rest/client/test_delayed_events.py | 10 ----- tests/rest/client/test_login_token_request.py | 10 ----- tests/rest/client/test_matrixrtc.py | 10 ----- tests/rest/client/test_versions.py | 10 ----- tests/server.py | 44 +++++++++++++------ tests/synapse_rust/test_http_client.py | 10 ----- tests/synapse_rust/test_logcontext.py | 9 ---- 11 files changed, 32 insertions(+), 101 deletions(-) create mode 100644 changelog.d/20509.misc diff --git a/changelog.d/20509.misc b/changelog.d/20509.misc new file mode 100644 index 0000000000..005b1f1b62 --- /dev/null +++ b/changelog.d/20509.misc @@ -0,0 +1 @@ +Fix running homeserver shutdown handlers at the end of each test. diff --git a/tests/app/test_homeserver_shutdown.py b/tests/app/test_homeserver_shutdown.py index 938604ad28..0bf22fc548 100644 --- a/tests/app/test_homeserver_shutdown.py +++ b/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 diff --git a/tests/handlers/test_oauth_delegation.py b/tests/handlers/test_oauth_delegation.py index d37a58949e..4c10ec9bbf 100644 --- a/tests/handlers/test_oauth_delegation.py +++ b/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 = { diff --git a/tests/push/test_http.py b/tests/push/test_http.py index bf6d2ce084..8e3a801445 100644 --- a/tests/push/test_http.py +++ b/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.""" diff --git a/tests/rest/client/test_delayed_events.py b/tests/rest/client/test_delayed_events.py index d86c085978..75c859b2a0 100644 --- a/tests/rest/client/test_delayed_events.py +++ b/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) diff --git a/tests/rest/client/test_login_token_request.py b/tests/rest/client/test_login_token_request.py index 28a0fca9ce..fd39463a24 100644 --- a/tests/rest/client/test_login_token_request.py +++ b/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" diff --git a/tests/rest/client/test_matrixrtc.py b/tests/rest/client/test_matrixrtc.py index 30eec240c3..5a8a399a00 100644 --- a/tests/rest/client/test_matrixrtc.py +++ b/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) diff --git a/tests/rest/client/test_versions.py b/tests/rest/client/test_versions.py index 6ecff9de6f..46c8f71aec 100644 --- a/tests/rest/client/test_versions.py +++ b/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") diff --git a/tests/server.py b/tests/server.py index fae274072f..04c04a1194 100644 --- a/tests/server.py +++ b/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. diff --git a/tests/synapse_rust/test_http_client.py b/tests/synapse_rust/test_http_client.py index 330fbeeba6..72b85a5f85 100644 --- a/tests/synapse_rust/test_http_client.py +++ b/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), ( diff --git a/tests/synapse_rust/test_logcontext.py b/tests/synapse_rust/test_logcontext.py index 5e1cefaff9..3788e78119 100644 --- a/tests/synapse_rust/test_logcontext.py +++ b/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: