From b94d7ee637cff519237feab10d2e792033123e12 Mon Sep 17 00:00:00 2001 From: Sean Quah Date: Thu, 28 Apr 2022 16:03:39 +0100 Subject: [PATCH 1/3] Complain if a federation endpoint has the `@cancellable` flag `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 --- synapse/federation/transport/server/_base.py | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/synapse/federation/transport/server/_base.py b/synapse/federation/transport/server/_base.py index d629a3ecb5dd..73467bccf7e8 100644 --- 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,15 @@ 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. + 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,), From db83bc6c3d91479264f24a7e45ccb449d53c14ec Mon Sep 17 00:00:00 2001 From: Sean Quah Date: Thu, 28 Apr 2022 16:29:58 +0100 Subject: [PATCH 2/3] Disable tests for the `@cancellable` flag on `BaseFederationServlet` methods Signed-off-by: Sean Quah --- synapse/federation/transport/server/_base.py | 2 ++ tests/federation/transport/server/test__base.py | 2 ++ 2 files changed, 4 insertions(+) diff --git a/synapse/federation/transport/server/_base.py b/synapse/federation/transport/server/_base.py index 73467bccf7e8..103861644a70 100644 --- a/synapse/federation/transport/server/_base.py +++ b/synapse/federation/transport/server/_base.py @@ -376,6 +376,8 @@ def register(self, server: HttpServer) -> None: 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 " diff --git a/tests/federation/transport/server/test__base.py b/tests/federation/transport/server/test__base.py index 98a951f03e07..ac3695a8ccab 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): From 4d0c233dc3c72972a5311a29fe2750dedeeb481f Mon Sep 17 00:00:00 2001 From: Sean Quah Date: Wed, 11 May 2022 12:30:21 +0100 Subject: [PATCH 3/3] Add newsfile --- changelog.d/12705.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/12705.misc diff --git a/changelog.d/12705.misc b/changelog.d/12705.misc new file mode 100644 index 000000000000..a913d8bb85eb --- /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.