From 496415149583f5fdfaa642826394477e19d79f9f Mon Sep 17 00:00:00 2001 From: Sean Quah Date: Thu, 28 Apr 2022 15:50:37 +0100 Subject: [PATCH 1/4] Respect the `@cancellable` flag for `DirectServe{Html,Json}Resource`s `DirectServeHtmlResource` and `DirectServeJsonResource` both inherit from `_AsyncResource`. These classes expect to be subclassed with `_async_render_*` methods. This commit has no effect on `JsonResource`, despite inheriting from `_AsyncResource`. `JsonResource` has its own `_async_render` override which will need to be updated separately. Signed-off-by: Sean Quah --- synapse/http/server.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/synapse/http/server.py b/synapse/http/server.py index 4b4debc5cd2b..f6d4d8db86fa 100644 --- a/synapse/http/server.py +++ b/synapse/http/server.py @@ -382,6 +382,8 @@ async def _async_render(self, request: SynapseRequest) -> Optional[Tuple[int, An method_handler = getattr(self, "_async_render_%s" % (request_method,), None) if method_handler: + request.is_render_cancellable = is_method_cancellable(method_handler) + raw_callback_return = method_handler(request) # Is it synchronous? We'll allow this for now. From 8fd36ecbca84ffdbcb6ad96c3ee98991c1ac7cab Mon Sep 17 00:00:00 2001 From: Sean Quah Date: Thu, 28 Apr 2022 16:20:03 +0100 Subject: [PATCH 2/4] Test the `@cancellable` flag on `DirectServe{Html,Json}Resource` methods Signed-off-by: Sean Quah --- tests/test_server.py | 111 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 109 insertions(+), 2 deletions(-) diff --git a/tests/test_server.py b/tests/test_server.py index f2ffbc895b88..493b673f6d90 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -13,18 +13,28 @@ # limitations under the License. import re +from http import HTTPStatus +from typing import Tuple from twisted.internet.defer import Deferred from twisted.web.resource import Resource from synapse.api.errors import Codes, RedirectException, SynapseError from synapse.config.server import parse_listener_def -from synapse.http.server import DirectServeHtmlResource, JsonResource, OptionsResource -from synapse.http.site import SynapseSite +from synapse.http.server import ( + DirectServeHtmlResource, + DirectServeJsonResource, + JsonResource, + OptionsResource, + cancellable, +) +from synapse.http.site import SynapseRequest, SynapseSite from synapse.logging.context import make_deferred_yieldable +from synapse.types import JsonDict from synapse.util import Clock from tests import unittest +from tests.http.server._base import EndpointCancellationTestCase from tests.server import ( FakeSite, ThreadedMemoryReactorClock, @@ -363,3 +373,100 @@ async def callback(request): self.assertEqual(channel.result["code"], b"200") self.assertNotIn("body", channel.result) + + +class CancellableDirectServeJsonResource(DirectServeJsonResource): + def __init__(self, clock: Clock): + super().__init__() + self.clock = clock + + @cancellable + async def _async_render_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: + await self.clock.sleep(1.0) + return HTTPStatus.OK, {"result": True} + + async def _async_render_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: + await self.clock.sleep(1.0) + return HTTPStatus.OK, {"result": True} + + +class CancellableDirectServeHtmlResource(DirectServeHtmlResource): + ERROR_TEMPLATE = "{code} {msg}" + + def __init__(self, clock: Clock): + super().__init__() + self.clock = clock + + @cancellable + async def _async_render_GET(self, request: SynapseRequest) -> Tuple[int, bytes]: + await self.clock.sleep(1.0) + return HTTPStatus.OK, b"ok" + + async def _async_render_POST(self, request: SynapseRequest) -> Tuple[int, bytes]: + await self.clock.sleep(1.0) + return HTTPStatus.OK, b"ok" + + +class DirectServeJsonResourceCancellationTests(EndpointCancellationTestCase): + """Tests for `DirectServeJsonResource` cancellation.""" + + def setUp(self): + self.reactor = ThreadedMemoryReactorClock() + self.clock = Clock(self.reactor) + self.resource = CancellableDirectServeJsonResource(self.clock) + self.site = FakeSite(self.resource, self.reactor) + + def test_cancellable_disconnect(self) -> None: + """Test that handlers with the `@cancellable` flag can be cancelled.""" + channel = make_request( + self.reactor, self.site, "GET", "/sleep", await_result=False + ) + self._test_disconnect( + self.reactor, + channel, + expect_cancellation=True, + expected_body={"error": "Request cancelled", "errcode": Codes.UNKNOWN}, + ) + + def test_uncancellable_disconnect(self) -> None: + """Test that handlers without the `@cancellable` flag cannot be cancelled.""" + channel = make_request( + self.reactor, self.site, "POST", "/sleep", await_result=False + ) + self._test_disconnect( + self.reactor, + channel, + expect_cancellation=False, + expected_body={"result": True}, + ) + + +class DirectServeHtmlResourceCancellationTests(EndpointCancellationTestCase): + """Tests for `DirectServeHtmlResource` cancellation.""" + + def setUp(self): + self.reactor = ThreadedMemoryReactorClock() + self.clock = Clock(self.reactor) + self.resource = CancellableDirectServeHtmlResource(self.clock) + self.site = FakeSite(self.resource, self.reactor) + + def test_cancellable_disconnect(self) -> None: + """Test that handlers with the `@cancellable` flag can be cancelled.""" + channel = make_request( + self.reactor, self.site, "GET", "/sleep", await_result=False + ) + self._test_disconnect( + self.reactor, + channel, + expect_cancellation=True, + expected_body=b"499 Request cancelled", + ) + + def test_uncancellable_disconnect(self) -> None: + """Test that handlers without the `@cancellable` flag cannot be cancelled.""" + channel = make_request( + self.reactor, self.site, "POST", "/sleep", await_result=False + ) + self._test_disconnect( + self.reactor, channel, expect_cancellation=False, expected_body=b"ok" + ) From 86245bfdcf9b2c42972079567dbbf315e12de334 Mon Sep 17 00:00:00 2001 From: Sean Quah Date: Tue, 10 May 2022 21:00:54 +0100 Subject: [PATCH 3/4] Add newsfile --- changelog.d/12698.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/12698.misc diff --git a/changelog.d/12698.misc b/changelog.d/12698.misc new file mode 100644 index 000000000000..5d626352f9c2 --- /dev/null +++ b/changelog.d/12698.misc @@ -0,0 +1 @@ +Respect the `@cancellable` flag for `DirectServe{Html,Json}Resource`s. From adde6a3059ea68805dddf6c52d3cf78f3f218f8f Mon Sep 17 00:00:00 2001 From: Sean Quah Date: Tue, 10 May 2022 21:14:26 +0100 Subject: [PATCH 4/4] Fix CI --- tests/test_server.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/test_server.py b/tests/test_server.py index 493b673f6d90..0f1eb43cbced 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -34,7 +34,7 @@ from synapse.util import Clock from tests import unittest -from tests.http.server._base import EndpointCancellationTestCase +from tests.http.server._base import EndpointCancellationTestHelperMixin from tests.server import ( FakeSite, ThreadedMemoryReactorClock, @@ -407,7 +407,7 @@ async def _async_render_POST(self, request: SynapseRequest) -> Tuple[int, bytes] return HTTPStatus.OK, b"ok" -class DirectServeJsonResourceCancellationTests(EndpointCancellationTestCase): +class DirectServeJsonResourceCancellationTests(EndpointCancellationTestHelperMixin): """Tests for `DirectServeJsonResource` cancellation.""" def setUp(self): @@ -441,7 +441,7 @@ def test_uncancellable_disconnect(self) -> None: ) -class DirectServeHtmlResourceCancellationTests(EndpointCancellationTestCase): +class DirectServeHtmlResourceCancellationTests(EndpointCancellationTestHelperMixin): """Tests for `DirectServeHtmlResource` cancellation.""" def setUp(self):