From 6ee61b905256f87dc2b75007ed711cd59065db9a Mon Sep 17 00:00:00 2001 From: Sean Quah <8349537+squahtx@users.noreply.github.com> Date: Wed, 11 May 2022 14:52:26 +0100 Subject: [PATCH] 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 --- changelog.d/12705.misc | 1 + synapse/federation/transport/server/_base.py | 13 ++++++++++++- tests/federation/transport/server/test__base.py | 2 ++ 3 files changed, 15 insertions(+), 1 deletion(-) create mode 100644 changelog.d/12705.misc diff --git a/changelog.d/12705.misc b/changelog.d/12705.misc new file mode 100644 index 000000000..a913d8bb8 --- /dev/null +++ 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 index d629a3ecb..103861644 100644 --- a/synapse/federation/transport/server/_base.py +++ b/synapse/federation/transport/server/_base.py @@ -21,7 +21,7 @@ from typing import TYPE_CHECKING, Any, Awaitable, Callable, Dict, Optional, Tupl 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 @@ class BaseFederationServlet: 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 index 98a951f03..ac3695a8c 100644 --- 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):