Complain if a federation endpoint has the @cancellable flag (#12705)

`BaseFederationServlet` wraps its endpoints in a bunch of async code that has not been vetted for compatibility with cancellation. Fail CI if a `@cancellable` flag is applied to a federation endpoint. Signed-off-by: Sean Quah <[email protected]>
matrix-org · May 11, 2022 · 6ee61b9 · 6ee61b9
1 parent d38d242
commit 6ee61b9
Show file tree

Hide file tree

Showing 3 changed files with 15 additions and 1 deletion.
diff --git a/changelog.d/12705.misc b/changelog.d/12705.misc
@@ -0,0 +1 @@
+Complain if a federation endpoint has the `@cancellable` flag, since some of the wrapper code may not handle cancellation correctly yet.
diff --git a/synapse/federation/transport/server/_base.py b/synapse/federation/transport/server/_base.py
@@ -21,7 +21,7 @@
 
 from synapse.api.errors import Codes, FederationDeniedError, SynapseError
 from synapse.api.urls import FEDERATION_V1_PREFIX
-from synapse.http.server import HttpServer, ServletCallback
+from synapse.http.server import HttpServer, ServletCallback, is_method_cancellable
 from synapse.http.servlet import parse_json_object_from_request
 from synapse.http.site import SynapseRequest
 from synapse.logging.context import run_in_background
@@ -373,6 +373,17 @@ def register(self, server: HttpServer) -> None:
             if code is None:
                 continue
 
+            if is_method_cancellable(code):
+                # The wrapper added by `self._wrap` will inherit the cancellable flag,
+                # but the wrapper itself does not support cancellation yet.
+                # Once resolved, the cancellation tests in
+                # `tests/federation/transport/server/test__base.py` can be re-enabled.
+                raise Exception(
+                    f"{self.__class__.__name__}.on_{method} has been marked as "
+                    "cancellable, but federation servlets do not support cancellation "
+                    "yet."
+                )
+
             server.register_paths(
                 method,
                 (pattern,),

diff --git a/tests/federation/transport/server/test__base.py b/tests/federation/transport/server/test__base.py
@@ -59,6 +59,8 @@ class BaseFederationServletCancellationTests(
 ):
     """Tests for `BaseFederationServlet` cancellation."""
 
+    skip = "`BaseFederationServlet` does not support cancellation yet."
+
     path = f"{CancellableFederationServlet.PREFIX}{CancellableFederationServlet.PATH}"
 
     def create_test_resource(self):
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		Complain if a federation endpoint has the `@cancellable` flag, since some of the wrapper code may not handle cancellation correctly yet.