Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

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

Merged
merged 3 commits into from
May 11, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b94d7ee
Complain if a federation endpoint has the `@cancellable` flag
Apr 28, 2022
db83bc6
Disable tests for the `@cancellable` flag on `BaseFederationServlet` …
Apr 28, 2022
4d0c233
Add newsfile
May 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/12705.misc
View file
Open in desktop
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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the point just that we haven't audited this to check that it's cancellation-safe?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty much!

13 changes: 12 additions & 1 deletion synapse/federation/transport/server/_base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On one of the other PRs, I wondered (but didn't say) if we could use a more specific exception type here. I don't think there's an obvious candidate though, and if this gets thrown it'll happen loudly at startup, so it's probably fine as-is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure which exception type would be more appropriate. Nothing really springs to mind.

f"{self.__class__.__name__}.on_{method} has been marked as "
"cancellable, but federation servlets do not support cancellation "
"yet."
)

server.register_paths(
method,
(pattern,),
Expand Down
2 changes: 2 additions & 0 deletions tests/federation/transport/server/test__base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down