From 75e1c0770d6e80f0d056cb0c290a89da8dad3074 Mon Sep 17 00:00:00 2001 From: Sean Quah Date: Thu, 28 Apr 2022 16:13:18 +0100 Subject: [PATCH 1/8] Expose the `SynapseRequest` from `FakeChannel` for testing disconnection In order to simulate a client disconnection in tests, we would like to call `Request.connectionLost`. Make the `Request` accessible from the `FakeChannel` returned by `make_request`. Signed-off-by: Sean Quah --- tests/server.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/server.py b/tests/server.py index 16559d2588c2..4ae28590fe13 100644 --- a/tests/server.py +++ b/tests/server.py @@ -109,6 +109,17 @@ class FakeChannel: _ip: str = "127.0.0.1" _producer: Optional[Union[IPullProducer, IPushProducer]] = None resource_usage: Optional[ContextResourceUsage] = None + _request: Optional[Request] = None + + @property + def request(self) -> Request: + assert self._request is not None + return self._request + + @request.setter + def request(self, request: Request) -> None: + assert self._request is None + self._request = request @property def json_body(self): @@ -322,6 +333,8 @@ def make_request( channel = FakeChannel(site, reactor, ip=client_ip) req = request(channel, site) + channel.request = req + req.content = BytesIO(content) # Twisted expects to be at the end of the content when parsing the request. req.content.seek(0, SEEK_END) From f0b57d5e1cdca91393b3406142c2082b211873ca Mon Sep 17 00:00:00 2001 From: Sean Quah Date: Thu, 28 Apr 2022 16:17:32 +0100 Subject: [PATCH 2/8] Add helper class for testing request cancellation Signed-off-by: Sean Quah --- tests/http/server/__init__.py | 13 +++++ tests/http/server/_base.py | 96 +++++++++++++++++++++++++++++++++++ 2 files changed, 109 insertions(+) create mode 100644 tests/http/server/__init__.py create mode 100644 tests/http/server/_base.py diff --git a/tests/http/server/__init__.py b/tests/http/server/__init__.py new file mode 100644 index 000000000000..3a5f22c02235 --- /dev/null +++ b/tests/http/server/__init__.py @@ -0,0 +1,13 @@ +# Copyright 2022 The Matrix.org Foundation C.I.C. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. diff --git a/tests/http/server/_base.py b/tests/http/server/_base.py new file mode 100644 index 000000000000..b0c333b00f9d --- /dev/null +++ b/tests/http/server/_base.py @@ -0,0 +1,96 @@ +# Copyright 2022 The Matrix.org Foundation C.I.C. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unles4s required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +from http import HTTPStatus +from typing import Any, Callable, Optional, Union +from unittest import mock + +from twisted.internet.error import ConnectionDone + +from synapse.http.server import ( + HTTP_STATUS_REQUEST_CANCELLED, + respond_with_html_bytes, + respond_with_json, +) +from synapse.types import JsonDict + +from tests import unittest +from tests.server import FakeChannel, ThreadedMemoryReactorClock + + +class EndpointCancellationTestCase(unittest.TestCase): + """Provides helper methods for testing cancellation of endpoints.""" + + def _test_disconnect( + self, + reactor: ThreadedMemoryReactorClock, + channel: FakeChannel, + expect_cancellation: bool, + expected_body: Union[bytes, JsonDict], + expected_code: Optional[int] = None, + ) -> None: + """Disconnects an in-flight request and checks the response. + + Args: + reactor: The twisted reactor running the request handler. + channel: The `FakeChannel` for the request. + expect_cancellation: `True` if request processing is expected to be + cancelled, `False` if the request should run to completion. + expected_body: The expected response for the request. + """ + # Determine the expected status code. + if expected_code is None: + if expect_cancellation: + expected_code = HTTP_STATUS_REQUEST_CANCELLED + else: + expected_code = HTTPStatus.OK + + request = channel.request + self.assertFalse( + channel.is_finished(), "Request finished before we could disconnect" + ) + + # We're about to disconnect the request. This also disconnects the channel, so + # we have to rely on mocks to extract the response. + respond_method: Callable[..., Any] + if isinstance(expected_body, bytes): + respond_method = respond_with_html_bytes + else: + respond_method = respond_with_json + + with mock.patch( + f"synapse.http.server.{respond_method.__name__}", wraps=respond_method + ) as respond_mock: + # Disconnect the request. + request.connectionLost(reason=ConnectionDone()) + + if expect_cancellation: + # An immediate cancellation is expected. + respond_mock.assert_called_once() + args, _kwargs = respond_mock.call_args + code, body = args[1], args[2] + self.assertEqual(code, expected_code) + self.assertEqual(request.code, expected_code) + self.assertEqual(body, expected_body) + else: + respond_mock.assert_not_called() + + # The handler is expected to run to completion. + reactor.pump([1.0]) + respond_mock.assert_called_once() + args, _kwargs = respond_mock.call_args + code, body = args[1], args[2] + self.assertEqual(code, expected_code) + self.assertEqual(request.code, expected_code) + self.assertEqual(body, expected_body) From 957474d6f063641cf9fdf0451afebae2d811d2a9 Mon Sep 17 00:00:00 2001 From: Sean Quah Date: Thu, 28 Apr 2022 18:24:58 +0100 Subject: [PATCH 3/8] Add newsfile Signed-off-by: Sean Quah --- changelog.d/12630.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/12630.misc diff --git a/changelog.d/12630.misc b/changelog.d/12630.misc new file mode 100644 index 000000000000..d26e332305ce --- /dev/null +++ b/changelog.d/12630.misc @@ -0,0 +1 @@ +Add `@cancellable` decorator, for use on endpoint methods that can be cancelled when clients disconnect. From ccd0ab7d08ec136fb40666941dfec8ce75934ddc Mon Sep 17 00:00:00 2001 From: Sean Quah Date: Fri, 6 May 2022 20:27:52 +0100 Subject: [PATCH 4/8] Rename to `EndpointCancellationTestCase` to `EndpointCancellationTestHelperMixin` Signed-off-by: Sean Quah --- tests/http/server/_base.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/http/server/_base.py b/tests/http/server/_base.py index b0c333b00f9d..dc8208558108 100644 --- a/tests/http/server/_base.py +++ b/tests/http/server/_base.py @@ -29,7 +29,7 @@ from tests.server import FakeChannel, ThreadedMemoryReactorClock -class EndpointCancellationTestCase(unittest.TestCase): +class EndpointCancellationTestHelperMixin(unittest.TestCase): """Provides helper methods for testing cancellation of endpoints.""" def _test_disconnect( From 390117aed26f06b55113e7d051f8598b7176554a Mon Sep 17 00:00:00 2001 From: Sean Quah Date: Fri, 6 May 2022 20:29:09 +0100 Subject: [PATCH 5/8] Add missing docstring for `expected_body` parameter Signed-off-by: Sean Quah --- tests/http/server/_base.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/http/server/_base.py b/tests/http/server/_base.py index dc8208558108..02b20b2cc803 100644 --- a/tests/http/server/_base.py +++ b/tests/http/server/_base.py @@ -48,6 +48,8 @@ def _test_disconnect( expect_cancellation: `True` if request processing is expected to be cancelled, `False` if the request should run to completion. expected_body: The expected response for the request. + expected_code: The expected status code for the request. Defaults to `200` + or `499` depending on `expect_cancellation`. """ # Determine the expected status code. if expected_code is None: From 8c9b9f60bf833820ed0e6f52ad3c63cdf6d71341 Mon Sep 17 00:00:00 2001 From: Sean Quah Date: Fri, 6 May 2022 20:29:23 +0100 Subject: [PATCH 6/8] Improve assertion message when `await_result=False` is forgotten Signed-off-by: Sean Quah --- tests/http/server/_base.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/http/server/_base.py b/tests/http/server/_base.py index 02b20b2cc803..b9f1a381aa2b 100644 --- a/tests/http/server/_base.py +++ b/tests/http/server/_base.py @@ -60,7 +60,9 @@ def _test_disconnect( request = channel.request self.assertFalse( - channel.is_finished(), "Request finished before we could disconnect" + channel.is_finished(), + "Request finished before we could disconnect - " + "was `await_result=False` passed to `make_request`?", ) # We're about to disconnect the request. This also disconnects the channel, so From ee45caf20f4f99855ab5b9f551f3644cf719298a Mon Sep 17 00:00:00 2001 From: Sean Quah Date: Fri, 6 May 2022 21:50:12 +0100 Subject: [PATCH 7/8] Update the changelog, since not all related PRs will land in the same release cycle --- changelog.d/12630.misc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/12630.misc b/changelog.d/12630.misc index d26e332305ce..898f906cec0a 100644 --- a/changelog.d/12630.misc +++ b/changelog.d/12630.misc @@ -1 +1 @@ -Add `@cancellable` decorator, for use on endpoint methods that can be cancelled when clients disconnect. +Add a helper class to test cancellation of requests. From 01ad80bf8879e464b3f2e238088503a0e5fe131e Mon Sep 17 00:00:00 2001 From: Sean Quah Date: Fri, 6 May 2022 21:52:59 +0100 Subject: [PATCH 8/8] Update the changelog, since not all related PRs will land in the same release cycle --- changelog.d/12630.misc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/12630.misc b/changelog.d/12630.misc index 898f906cec0a..43e12603e2d8 100644 --- a/changelog.d/12630.misc +++ b/changelog.d/12630.misc @@ -1 +1 @@ -Add a helper class to test cancellation of requests. +Add a helper class for testing request cancellation.