From 8c47577df7a177bcb9740954d2e85a2a54244583 Mon Sep 17 00:00:00 2001
From: Erik Johnston <erik@matrix.org>
Date: Mon, 22 Jun 2020 14:54:31 +0100
Subject: [PATCH 01/13] Pull out error handling from
 'wrap_json_request_handler'

---
 synapse/http/server.py | 83 +++++++++++++++++++-----------------------
 1 file changed, 38 insertions(+), 45 deletions(-)

diff --git a/synapse/http/server.py b/synapse/http/server.py
index 6aa1dc1f9227..caf039e2fb14 100644
--- a/synapse/http/server.py
+++ b/synapse/http/server.py
@@ -79,60 +79,53 @@ def wrap_json_request_handler(h):
     async def wrapped_request_handler(self, request):
         try:
             await h(self, request)
-        except SynapseError as e:
-            code = e.code
-            logger.info("%s SynapseError: %s - %s", request, code, e.msg)
-
-            # Only respond with an error response if we haven't already started
-            # writing, otherwise lets just kill the connection
-            if request.startedWriting:
-                if request.transport:
-                    try:
-                        request.transport.abortConnection()
-                    except Exception:
-                        # abortConnection throws if the connection is already closed
-                        pass
-            else:
-                respond_with_json(
-                    request,
-                    code,
-                    e.error_dict(),
-                    send_cors=True,
-                    pretty_print=_request_user_agent_is_curl(request),
-                )
-
         except Exception:
             # failure.Failure() fishes the original Failure out
             # of our stack, and thus gives us a sensible stack
             # trace.
             f = failure.Failure()
-            logger.error(
-                "Failed handle request via %r: %r",
-                request.request_metrics.name,
-                request,
-                exc_info=(f.type, f.value, f.getTracebackObject()),
-            )
-            # Only respond with an error response if we haven't already started
-            # writing, otherwise lets just kill the connection
-            if request.startedWriting:
-                if request.transport:
-                    try:
-                        request.transport.abortConnection()
-                    except Exception:
-                        # abortConnection throws if the connection is already closed
-                        pass
-            else:
-                respond_with_json(
-                    request,
-                    500,
-                    {"error": "Internal server error", "errcode": Codes.UNKNOWN},
-                    send_cors=True,
-                    pretty_print=_request_user_agent_is_curl(request),
-                )
+            return_json_error(f, request)
 
     return wrap_async_request_handler(wrapped_request_handler)
 
 
+def return_json_error(f: failure.Failure, request: Request) -> None:
+    """Sends a JSON error response to clients.
+    """
+
+    if f.check(SynapseError):
+        error_code = f.value.code
+        error_dict = f.value.error_dict()
+
+        logger.info("%s SynapseError: %s - %s", request, error_code, f.value.msg)
+    else:
+        error_code = 500
+        error_dict = {"error": "Internal server error", "errcode": Codes.UNKNOWN}
+
+        logger.error(
+            "Failed handle request via %r: %r",
+            request.request_metrics.name,
+            request,
+            exc_info=(f.type, f.value, f.getTracebackObject()),
+        )
+
+    if request.startedWriting:
+        if request.transport:
+            try:
+                request.transport.abortConnection()
+            except Exception:
+                # abortConnection throws if the connection is already closed
+                pass
+    else:
+        respond_with_json(
+            request,
+            error_code,
+            error_dict,
+            send_cors=True,
+            pretty_print=_request_user_agent_is_curl(request),
+        )
+
+
 TV = TypeVar("TV")
 
 

From 65a5e167a034dc898050d697d22b5564b5ea5c1b Mon Sep 17 00:00:00 2001
From: Erik Johnston <erik@matrix.org>
Date: Mon, 22 Jun 2020 15:14:53 +0100
Subject: [PATCH 02/13] Abstract out request handling from JsonResource

---
 synapse/http/server.py | 153 ++++++++++++++++++++++++++---------------
 1 file changed, 96 insertions(+), 57 deletions(-)

diff --git a/synapse/http/server.py b/synapse/http/server.py
index caf039e2fb14..cab592e5d21a 100644
--- a/synapse/http/server.py
+++ b/synapse/http/server.py
@@ -14,6 +14,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+import abc
 import collections
 import html
 import logging
@@ -21,7 +22,7 @@
 import urllib
 from http import HTTPStatus
 from io import BytesIO
-from typing import Awaitable, Callable, TypeVar, Union
+from typing import Any, Awaitable, Callable, Dict, Optional, Tuple, TypeVar, Union
 
 import jinja2
 from canonicaljson import encode_canonical_json, encode_pretty_printed_json, json
@@ -247,7 +248,87 @@ def register_paths(self, method, path_patterns, callback):
         pass
 
 
-class JsonResource(HttpServer, resource.Resource):
+class _AsyncResource(resource.Resource, metaclass=abc.ABCMeta):
+    """Base class for resources that have async handlers.
+    """
+
+    def render(self, request):
+        """ This gets called by twisted every time someone sends us a request.
+        """
+        defer.ensureDeferred(self._async_render(request))
+        return NOT_DONE_YET
+
+    @wrap_async_request_handler
+    async def _async_render(self, request):
+        """ This gets called from render() every time someone sends us a request.
+
+        Calls `self._get_handler_for_request` to get the callback to use.
+        """
+        try:
+            callback, servlet_classname, group_dict = self._get_handler_for_request(
+                request
+            )
+
+            # Make sure we have a name for this handler in prometheus.
+            request.request_metrics.name = servlet_classname
+
+            # Now trigger the callback. If it returns a response, we send it
+            # here. If it throws an exception, that is handled by the wrapper
+            # installed by @request_handler.
+            kwargs = intern_dict(
+                {
+                    name: urllib.parse.unquote(value) if value else value
+                    for name, value in group_dict.items()
+                }
+            )
+
+            callback_return = callback(request, **kwargs)
+
+            # Is it synchronous? We'll allow this for now.
+            if isinstance(callback_return, (defer.Deferred, types.CoroutineType)):
+                callback_return = await callback_return
+
+            if callback_return is not None:
+                code, response = callback_return
+                self._send_response(request, code, response)
+        except Exception:
+            f = failure.Failure()
+            self._send_error_response(f, request)
+
+    @abc.abstractmethod
+    def _get_handler_for_request(
+        self, request: SynapseRequest
+    ) -> Tuple[
+        Callable[..., Awaitable[Optional[Tuple[int, Any]]]], str, Dict[str, str],
+    ]:
+        """Finds a callback method to handle the given request.
+
+        Returns:
+            A tuple of callback method, the label to use for that method in
+            prometheus metrics, and a dict to pass as kwargs to the callback
+            (typically mapping keys to path components as specified in the
+            handler's path match regexp).
+
+            The returned callback should either return a tuple of response code
+            and response object that will be passed to `self._send_response` or
+            None if the callback writes the response itself.
+        """
+        raise NotImplementedError()
+
+    @abc.abstractmethod
+    def _send_response(
+        self, request: SynapseRequest, code: int, response_object: Any,
+    ) -> None:
+        raise NotImplementedError()
+
+    @abc.abstractmethod
+    def _send_error_response(
+        self, f: failure.Failure, request: SynapseRequest,
+    ) -> None:
+        raise NotImplementedError()
+
+
+class JsonResource(HttpServer, _AsyncResource):
     """ This implements the HttpServer interface and provides JSON support for
     Resources.
 
@@ -309,58 +390,8 @@ def register_paths(
                 self._PathEntry(path_pattern, callback, servlet_classname)
             )
 
-    def render(self, request):
-        """ This gets called by twisted every time someone sends us a request.
-        """
-        defer.ensureDeferred(self._async_render(request))
-        return NOT_DONE_YET
-
-    @wrap_json_request_handler
-    async def _async_render(self, request):
-        """ This gets called from render() every time someone sends us a request.
-            This checks if anyone has registered a callback for that method and
-            path.
-        """
-        callback, servlet_classname, group_dict = self._get_handler_for_request(request)
-
-        # Make sure we have a name for this handler in prometheus.
-        request.request_metrics.name = servlet_classname
-
-        # Now trigger the callback. If it returns a response, we send it
-        # here. If it throws an exception, that is handled by the wrapper
-        # installed by @request_handler.
-        kwargs = intern_dict(
-            {
-                name: urllib.parse.unquote(value) if value else value
-                for name, value in group_dict.items()
-            }
-        )
-
-        callback_return = callback(request, **kwargs)
-
-        # Is it synchronous? We'll allow this for now.
-        if isinstance(callback_return, (defer.Deferred, types.CoroutineType)):
-            callback_return = await callback_return
-
-        if callback_return is not None:
-            code, response = callback_return
-            self._send_response(request, code, response)
-
-    def _get_handler_for_request(self, request):
-        """Finds a callback method to handle the given request
-
-        Args:
-            request (twisted.web.http.Request):
-
-        Returns:
-            Tuple[Callable, str, dict[unicode, unicode]]: callback method, the
-                label to use for that method in prometheus metrics, and the
-                dict mapping keys to path components as specified in the
-                handler's path match regexp.
-
-                The callback will normally be a method registered via
-                register_paths, so will return (possibly via Deferred) either
-                None, or a tuple of (http code, response body).
+    def _get_handler_for_request(self, request: SynapseRequest):
+        """Implements _AsyncResource._get_handler_for_request
         """
         request_path = request.path.decode("ascii")
 
@@ -376,19 +407,27 @@ def _get_handler_for_request(self, request):
         return _unrecognised_request_handler, "unrecognised_request_handler", {}
 
     def _send_response(
-        self, request, code, response_json_object, response_code_message=None
+        self, request, code, response_object,
     ):
+        """Implements _AsyncResource._send_response
+        """
         # TODO: Only enable CORS for the requests that need it.
         respond_with_json(
             request,
             code,
-            response_json_object,
+            response_object,
             send_cors=True,
-            response_code_message=response_code_message,
             pretty_print=_request_user_agent_is_curl(request),
             canonical_json=self.canonical_json,
         )
 
+    def _send_error_response(
+        self, f: failure.Failure, request: SynapseRequest,
+    ) -> None:
+        """Implements _AsyncResource._send_error_response
+        """
+        return_json_error(f, request)
+
 
 class DirectServeResource(resource.Resource):
     def render(self, request):

From 99be35e2351bb53ec69b81a413b2a8727e712017 Mon Sep 17 00:00:00 2001
From: Erik Johnston <erik@matrix.org>
Date: Mon, 22 Jun 2020 16:12:53 +0100
Subject: [PATCH 03/13] Create and use DirectServer*Resource classes

---
 synapse/http/server.py                        | 113 ++++++++++++------
 synapse/rest/consent/consent_resource.py      |  10 +-
 synapse/rest/key/v2/remote_key_resource.py    |  12 +-
 synapse/rest/media/v1/config_resource.py      |  14 +--
 synapse/rest/media/v1/download_resource.py    |  12 +-
 synapse/rest/media/v1/preview_url_resource.py |  10 +-
 synapse/rest/media/v1/thumbnail_resource.py   |   9 +-
 synapse/rest/media/v1/upload_resource.py      |  14 +--
 synapse/rest/oidc/callback_resource.py        |   7 +-
 tests/test_server.py                          |  12 +-
 10 files changed, 104 insertions(+), 109 deletions(-)

diff --git a/synapse/http/server.py b/synapse/http/server.py
index cab592e5d21a..86ae7081c51c 100644
--- a/synapse/http/server.py
+++ b/synapse/http/server.py
@@ -130,27 +130,6 @@ def return_json_error(f: failure.Failure, request: Request) -> None:
 TV = TypeVar("TV")
 
 
-def wrap_html_request_handler(
-    h: Callable[[TV, SynapseRequest], Awaitable]
-) -> Callable[[TV, SynapseRequest], Awaitable[None]]:
-    """Wraps a request handler method with exception handling.
-
-    Also does the wrapping with request.processing as per wrap_async_request_handler.
-
-    The handler method must have a signature of "handle_foo(self, request)",
-    where "request" must be a SynapseRequest.
-    """
-
-    async def wrapped_request_handler(self, request):
-        try:
-            await h(self, request)
-        except Exception:
-            f = failure.Failure()
-            return_html_error(f, request, HTML_ERROR_TEMPLATE)
-
-    return wrap_async_request_handler(wrapped_request_handler)
-
-
 def return_html_error(
     f: failure.Failure, request: Request, error_template: Union[str, jinja2.Template],
 ) -> None:
@@ -429,27 +408,91 @@ def _send_error_response(
         return_json_error(f, request)
 
 
-class DirectServeResource(resource.Resource):
-    def render(self, request):
+class _DirectServeResource(_AsyncResource):
+    """Base class for resources that will just call `self._async_on_<METHOD>`
+    on new requests, formatting responses and errors as JSON.
+    """
+
+    def __init__(self):
+        super().__init__()
+
+        self._callbacks = {}
+        self._name = self.__class__.__name__
+
+        for method in ("GET", "PUT", "POST", "OPTIONS", "DELETE"):
+            method_handler = getattr(self, "_async_render_%s" % (method,), None)
+            if method_handler:
+                self._callbacks[method.encode("ascii")] = trace_servlet(self._name)(
+                    method_handler
+                )
+
+    def _get_handler_for_request(self, request: SynapseRequest):
+        """Implements _AsyncResource._get_handler_for_request
         """
-        Render the request, using an asynchronous render handler if it exists.
+        callback = self._callbacks.get(request.method)
+        if not callback:
+            return _unrecognised_request_handler, "unrecognised_request_handler", {}
+
+        return callback, self._name, {}
+
+
+class DirectServeJsonResource(_DirectServeResource):
+    """A resource that will call `self._async_on_<METHOD>` on new requests,
+    formatting responses and errors as JSON.
+    """
+
+    def _send_response(
+        self, request, code, response_object,
+    ):
+        """Implements _AsyncResource._send_response
+        """
+        # TODO: Only enable CORS for the requests that need it.
+        respond_with_json(
+            request,
+            code,
+            response_object,
+            send_cors=True,
+            pretty_print=_request_user_agent_is_curl(request),
+            canonical_json=self.canonical_json,
+        )
+
+    def _send_error_response(
+        self, f: failure.Failure, request: SynapseRequest,
+    ) -> None:
+        """Implements _AsyncResource._send_error_response
         """
-        async_render_callback_name = "_async_render_" + request.method.decode("ascii")
+        return_json_error(f, request)
 
-        # Try and get the async renderer
-        callback = getattr(self, async_render_callback_name, None)
 
-        # No async renderer for this request method.
-        if not callback:
-            return super().render(request)
+class DirectServeHtmlResource(_DirectServeResource):
+    """A resource that will call `self._async_on_<METHOD>` on new requests,
+    formatting responses and errors as HTML.
+    """
 
-        resp = trace_servlet(self.__class__.__name__)(callback)(request)
+    # The error template to use for this resource
+    ERROR_TEMPLATE = HTML_ERROR_TEMPLATE
+
+    def _send_response(
+        self, request: SynapseRequest, code: int, response_object: Any,
+    ):
+        """Implements _AsyncResource._send_response
+        """
+        # We expect to get bytes for us to write
+        assert isinstance(response_object, bytes)
+        html_bytes = response_object
 
-        # If it's a coroutine, turn it into a Deferred
-        if isinstance(resp, types.CoroutineType):
-            defer.ensureDeferred(resp)
+        request.setResponseCode(200)
+        request.setHeader(b"Content-Type", b"text/html; charset=utf-8")
+        request.setHeader(b"Content-Length", b"%d" % (len(html_bytes),))
+        request.write(html_bytes)
+        finish_request(request)
 
-        return NOT_DONE_YET
+    def _send_error_response(
+        self, f: failure.Failure, request: SynapseRequest,
+    ) -> None:
+        """Implements _AsyncResource._send_error_response
+        """
+        return_html_error(f, request, self.ERROR_TEMPLATE)
 
 
 def _options_handler(request):
diff --git a/synapse/rest/consent/consent_resource.py b/synapse/rest/consent/consent_resource.py
index 049c16b2363d..07bccebd2775 100644
--- a/synapse/rest/consent/consent_resource.py
+++ b/synapse/rest/consent/consent_resource.py
@@ -26,11 +26,7 @@
 
 from synapse.api.errors import NotFoundError, StoreError, SynapseError
 from synapse.config import ConfigError
-from synapse.http.server import (
-    DirectServeResource,
-    finish_request,
-    wrap_html_request_handler,
-)
+from synapse.http.server import DirectServeHtmlResource, finish_request
 from synapse.http.servlet import parse_string
 from synapse.types import UserID
 
@@ -48,7 +44,7 @@ def compare_digest(a, b):
         return a == b
 
 
-class ConsentResource(DirectServeResource):
+class ConsentResource(DirectServeHtmlResource):
     """A twisted Resource to display a privacy policy and gather consent to it
 
     When accessed via GET, returns the privacy policy via a template.
@@ -119,7 +115,6 @@ def __init__(self, hs):
 
         self._hmac_secret = hs.config.form_secret.encode("utf-8")
 
-    @wrap_html_request_handler
     async def _async_render_GET(self, request):
         """
         Args:
@@ -160,7 +155,6 @@ async def _async_render_GET(self, request):
         except TemplateNotFound:
             raise NotFoundError("Unknown policy version")
 
-    @wrap_html_request_handler
     async def _async_render_POST(self, request):
         """
         Args:
diff --git a/synapse/rest/key/v2/remote_key_resource.py b/synapse/rest/key/v2/remote_key_resource.py
index ab671f733470..e149ac173334 100644
--- a/synapse/rest/key/v2/remote_key_resource.py
+++ b/synapse/rest/key/v2/remote_key_resource.py
@@ -20,17 +20,13 @@
 
 from synapse.api.errors import Codes, SynapseError
 from synapse.crypto.keyring import ServerKeyFetcher
-from synapse.http.server import (
-    DirectServeResource,
-    respond_with_json_bytes,
-    wrap_json_request_handler,
-)
+from synapse.http.server import DirectServeJsonResource, respond_with_json_bytes
 from synapse.http.servlet import parse_integer, parse_json_object_from_request
 
 logger = logging.getLogger(__name__)
 
 
-class RemoteKey(DirectServeResource):
+class RemoteKey(DirectServeJsonResource):
     """HTTP resource for retreiving the TLS certificate and NACL signature
     verification keys for a collection of servers. Checks that the reported
     X.509 TLS certificate matches the one used in the HTTPS connection. Checks
@@ -92,13 +88,14 @@ class RemoteKey(DirectServeResource):
     isLeaf = True
 
     def __init__(self, hs):
+        super().__init__()
+
         self.fetcher = ServerKeyFetcher(hs)
         self.store = hs.get_datastore()
         self.clock = hs.get_clock()
         self.federation_domain_whitelist = hs.config.federation_domain_whitelist
         self.config = hs.config
 
-    @wrap_json_request_handler
     async def _async_render_GET(self, request):
         if len(request.postpath) == 1:
             (server,) = request.postpath
@@ -115,7 +112,6 @@ async def _async_render_GET(self, request):
 
         await self.query_keys(request, query, query_remote_on_cache_miss=True)
 
-    @wrap_json_request_handler
     async def _async_render_POST(self, request):
         content = parse_json_object_from_request(request)
 
diff --git a/synapse/rest/media/v1/config_resource.py b/synapse/rest/media/v1/config_resource.py
index 9f747de26398..68dd2a1c8ab4 100644
--- a/synapse/rest/media/v1/config_resource.py
+++ b/synapse/rest/media/v1/config_resource.py
@@ -14,16 +14,10 @@
 # limitations under the License.
 #
 
-from twisted.web.server import NOT_DONE_YET
+from synapse.http.server import DirectServeJsonResource, respond_with_json
 
-from synapse.http.server import (
-    DirectServeResource,
-    respond_with_json,
-    wrap_json_request_handler,
-)
 
-
-class MediaConfigResource(DirectServeResource):
+class MediaConfigResource(DirectServeJsonResource):
     isLeaf = True
 
     def __init__(self, hs):
@@ -33,11 +27,9 @@ def __init__(self, hs):
         self.auth = hs.get_auth()
         self.limits_dict = {"m.upload.size": config.max_upload_size}
 
-    @wrap_json_request_handler
     async def _async_render_GET(self, request):
         await self.auth.get_user_by_req(request)
         respond_with_json(request, 200, self.limits_dict, send_cors=True)
 
-    def render_OPTIONS(self, request):
+    async def _async_render_OPTIONS(self, request):
         respond_with_json(request, 200, {}, send_cors=True)
-        return NOT_DONE_YET
diff --git a/synapse/rest/media/v1/download_resource.py b/synapse/rest/media/v1/download_resource.py
index 24d3ae5bbca2..d3d84573037d 100644
--- a/synapse/rest/media/v1/download_resource.py
+++ b/synapse/rest/media/v1/download_resource.py
@@ -15,18 +15,14 @@
 import logging
 
 import synapse.http.servlet
-from synapse.http.server import (
-    DirectServeResource,
-    set_cors_headers,
-    wrap_json_request_handler,
-)
+from synapse.http.server import DirectServeJsonResource, set_cors_headers
 
 from ._base import parse_media_id, respond_404
 
 logger = logging.getLogger(__name__)
 
 
-class DownloadResource(DirectServeResource):
+class DownloadResource(DirectServeJsonResource):
     isLeaf = True
 
     def __init__(self, hs, media_repo):
@@ -34,10 +30,6 @@ def __init__(self, hs, media_repo):
         self.media_repo = media_repo
         self.server_name = hs.hostname
 
-        # this is expected by @wrap_json_request_handler
-        self.clock = hs.get_clock()
-
-    @wrap_json_request_handler
     async def _async_render_GET(self, request):
         set_cors_headers(request)
         request.setHeader(
diff --git a/synapse/rest/media/v1/preview_url_resource.py b/synapse/rest/media/v1/preview_url_resource.py
index b4645cd608aa..e52c86c798f1 100644
--- a/synapse/rest/media/v1/preview_url_resource.py
+++ b/synapse/rest/media/v1/preview_url_resource.py
@@ -34,10 +34,9 @@
 from synapse.api.errors import Codes, SynapseError
 from synapse.http.client import SimpleHttpClient
 from synapse.http.server import (
-    DirectServeResource,
+    DirectServeJsonResource,
     respond_with_json,
     respond_with_json_bytes,
-    wrap_json_request_handler,
 )
 from synapse.http.servlet import parse_integer, parse_string
 from synapse.logging.context import make_deferred_yieldable, run_in_background
@@ -58,7 +57,7 @@
 OG_TAG_VALUE_MAXLEN = 1000
 
 
-class PreviewUrlResource(DirectServeResource):
+class PreviewUrlResource(DirectServeJsonResource):
     isLeaf = True
 
     def __init__(self, hs, media_repo, media_storage):
@@ -108,11 +107,10 @@ def __init__(self, hs, media_repo, media_storage):
                 self._start_expire_url_cache_data, 10 * 1000
             )
 
-    def render_OPTIONS(self, request):
+    async def _async_render_OPTIONS(self, request):
         request.setHeader(b"Allow", b"OPTIONS, GET")
-        return respond_with_json(request, 200, {}, send_cors=True)
+        respond_with_json(request, 200, {}, send_cors=True)
 
-    @wrap_json_request_handler
     async def _async_render_GET(self, request):
 
         # XXX: if get_user_by_req fails, what should we do in an async render?
diff --git a/synapse/rest/media/v1/thumbnail_resource.py b/synapse/rest/media/v1/thumbnail_resource.py
index 0b8722023420..23811d0b9673 100644
--- a/synapse/rest/media/v1/thumbnail_resource.py
+++ b/synapse/rest/media/v1/thumbnail_resource.py
@@ -16,11 +16,7 @@
 
 import logging
 
-from synapse.http.server import (
-    DirectServeResource,
-    set_cors_headers,
-    wrap_json_request_handler,
-)
+from synapse.http.server import DirectServeJsonResource, set_cors_headers
 from synapse.http.servlet import parse_integer, parse_string
 
 from ._base import (
@@ -34,7 +30,7 @@
 logger = logging.getLogger(__name__)
 
 
-class ThumbnailResource(DirectServeResource):
+class ThumbnailResource(DirectServeJsonResource):
     isLeaf = True
 
     def __init__(self, hs, media_repo, media_storage):
@@ -47,7 +43,6 @@ def __init__(self, hs, media_repo, media_storage):
         self.server_name = hs.hostname
         self.clock = hs.get_clock()
 
-    @wrap_json_request_handler
     async def _async_render_GET(self, request):
         set_cors_headers(request)
         server_name, media_id, _ = parse_media_id(request)
diff --git a/synapse/rest/media/v1/upload_resource.py b/synapse/rest/media/v1/upload_resource.py
index 83d005812de8..3ebf7a68e673 100644
--- a/synapse/rest/media/v1/upload_resource.py
+++ b/synapse/rest/media/v1/upload_resource.py
@@ -15,20 +15,14 @@
 
 import logging
 
-from twisted.web.server import NOT_DONE_YET
-
 from synapse.api.errors import Codes, SynapseError
-from synapse.http.server import (
-    DirectServeResource,
-    respond_with_json,
-    wrap_json_request_handler,
-)
+from synapse.http.server import DirectServeJsonResource, respond_with_json
 from synapse.http.servlet import parse_string
 
 logger = logging.getLogger(__name__)
 
 
-class UploadResource(DirectServeResource):
+class UploadResource(DirectServeJsonResource):
     isLeaf = True
 
     def __init__(self, hs, media_repo):
@@ -43,11 +37,9 @@ def __init__(self, hs, media_repo):
         self.max_upload_size = hs.config.max_upload_size
         self.clock = hs.get_clock()
 
-    def render_OPTIONS(self, request):
+    async def _async_render_OPTIONS(self, request):
         respond_with_json(request, 200, {}, send_cors=True)
-        return NOT_DONE_YET
 
-    @wrap_json_request_handler
     async def _async_render_POST(self, request):
         requester = await self.auth.get_user_by_req(request)
         # TODO: The checks here are a bit late. The content will have
diff --git a/synapse/rest/oidc/callback_resource.py b/synapse/rest/oidc/callback_resource.py
index c03194f00178..f7a0bc4bdbab 100644
--- a/synapse/rest/oidc/callback_resource.py
+++ b/synapse/rest/oidc/callback_resource.py
@@ -14,18 +14,17 @@
 # limitations under the License.
 import logging
 
-from synapse.http.server import DirectServeResource, wrap_html_request_handler
+from synapse.http.server import DirectServeHtmlResource
 
 logger = logging.getLogger(__name__)
 
 
-class OIDCCallbackResource(DirectServeResource):
+class OIDCCallbackResource(DirectServeHtmlResource):
     isLeaf = 1
 
     def __init__(self, hs):
         super().__init__()
         self._oidc_handler = hs.get_oidc_handler()
 
-    @wrap_html_request_handler
     async def _async_render_GET(self, request):
-        return await self._oidc_handler.handle_oidc_callback(request)
+        await self._oidc_handler.handle_oidc_callback(request)
diff --git a/tests/test_server.py b/tests/test_server.py
index 3f6f468e5b2c..030f58cbdc14 100644
--- a/tests/test_server.py
+++ b/tests/test_server.py
@@ -24,12 +24,7 @@
 
 from synapse.api.errors import Codes, RedirectException, SynapseError
 from synapse.config.server import parse_listener_def
-from synapse.http.server import (
-    DirectServeResource,
-    JsonResource,
-    OptionsResource,
-    wrap_html_request_handler,
-)
+from synapse.http.server import DirectServeHtmlResource, JsonResource, OptionsResource
 from synapse.http.site import SynapseSite, logger
 from synapse.logging.context import make_deferred_yieldable
 from synapse.util import Clock
@@ -256,12 +251,11 @@ def test_known_request(self):
 
 
 class WrapHtmlRequestHandlerTests(unittest.TestCase):
-    class TestResource(DirectServeResource):
+    class TestResource(DirectServeHtmlResource):
         callback = None
 
-        @wrap_html_request_handler
         async def _async_render_GET(self, request):
-            return await self.callback(request)
+            await self.callback(request)
 
     def setUp(self):
         self.reactor = ThreadedMemoryReactorClock()

From f5b79680e00a210b281f38c9d609268d42951ff7 Mon Sep 17 00:00:00 2001
From: Erik Johnston <erik@matrix.org>
Date: Mon, 22 Jun 2020 16:23:14 +0100
Subject: [PATCH 04/13] Newsfile

---
 changelog.d/7732.bugfix | 1 +
 1 file changed, 1 insertion(+)
 create mode 100644 changelog.d/7732.bugfix

diff --git a/changelog.d/7732.bugfix b/changelog.d/7732.bugfix
new file mode 100644
index 000000000000..d5e352e141b5
--- /dev/null
+++ b/changelog.d/7732.bugfix
@@ -0,0 +1 @@
+Fix "Tried to close a non-active scope!" error messages when opentracing is enabled.

From 328a4a6fc8d6f305581a7b62a01d49eb277884a8 Mon Sep 17 00:00:00 2001
From: Erik Johnston <erik@matrix.org>
Date: Mon, 22 Jun 2020 16:33:43 +0100
Subject: [PATCH 05/13] Fix mypy

---
 synapse/http/server.py | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/synapse/http/server.py b/synapse/http/server.py
index 86ae7081c51c..b2bdca41c59d 100644
--- a/synapse/http/server.py
+++ b/synapse/http/server.py
@@ -261,11 +261,13 @@ async def _async_render(self, request):
                 }
             )
 
-            callback_return = callback(request, **kwargs)
+            raw_callback_return = callback(request, **kwargs)
 
             # Is it synchronous? We'll allow this for now.
-            if isinstance(callback_return, (defer.Deferred, types.CoroutineType)):
-                callback_return = await callback_return
+            if isinstance(raw_callback_return, (defer.Deferred, types.CoroutineType)):
+                callback_return = await raw_callback_return
+            else:
+                callback_return = raw_callback_return
 
             if callback_return is not None:
                 code, response = callback_return
@@ -416,7 +418,9 @@ class _DirectServeResource(_AsyncResource):
     def __init__(self):
         super().__init__()
 
-        self._callbacks = {}
+        self._callbacks = (
+            {}
+        )  # type: Dict[bytes, Callable[..., Awaitable[Optional[Tuple[int, Any]]]]]
         self._name = self.__class__.__name__
 
         for method in ("GET", "PUT", "POST", "OPTIONS", "DELETE"):

From c31f29874847cf4a8210d493c9eb9f4a29f5c119 Mon Sep 17 00:00:00 2001
From: Erik Johnston <erik@matrix.org>
Date: Mon, 22 Jun 2020 16:36:32 +0100
Subject: [PATCH 06/13] Fix missed type

---
 synapse/rest/saml2/response_resource.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/synapse/rest/saml2/response_resource.py b/synapse/rest/saml2/response_resource.py
index 75e58043b420..c10188a5d72d 100644
--- a/synapse/rest/saml2/response_resource.py
+++ b/synapse/rest/saml2/response_resource.py
@@ -16,10 +16,10 @@
 from twisted.python import failure
 
 from synapse.api.errors import SynapseError
-from synapse.http.server import DirectServeResource, return_html_error
+from synapse.http.server import DirectServeHtmlResource, return_html_error
 
 
-class SAML2ResponseResource(DirectServeResource):
+class SAML2ResponseResource(DirectServeHtmlResource):
     """A Twisted web resource which handles the SAML response"""
 
     isLeaf = 1

From d57547785751e4adf90a555eb1f24725c6a7b4c4 Mon Sep 17 00:00:00 2001
From: Erik Johnston <erik@matrix.org>
Date: Tue, 23 Jun 2020 14:03:30 +0100
Subject: [PATCH 07/13] Fix up _get_handler_for_request type

---
 synapse/http/server.py | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/synapse/http/server.py b/synapse/http/server.py
index b2bdca41c59d..4a4941c15b86 100644
--- a/synapse/http/server.py
+++ b/synapse/http/server.py
@@ -62,6 +62,15 @@
 </html>
 """
 
+T = TypeVar("T")
+
+# A tuple of HTTP response code and response body. For JSON requests the body is
+# anything JSON serializable, while for HTML requests they are bytes.
+ResponseTuple = Tuple[int, Any]
+
+# Used for functions that may or may not return an awaitable object.
+MaybeAwaitable = Union[Awaitable[T], T]
+
 
 def wrap_json_request_handler(h):
     """Wraps a request handler method with exception handling.
@@ -280,7 +289,7 @@ async def _async_render(self, request):
     def _get_handler_for_request(
         self, request: SynapseRequest
     ) -> Tuple[
-        Callable[..., Awaitable[Optional[Tuple[int, Any]]]], str, Dict[str, str],
+        Callable[..., MaybeAwaitable[Optional[ResponseTuple]]], str, Dict[str, str],
     ]:
         """Finds a callback method to handle the given request.
 

From ffbddb648400a85c99a27bdcf74080ff84b6136f Mon Sep 17 00:00:00 2001
From: Erik Johnston <erik@matrix.org>
Date: Tue, 23 Jun 2020 14:23:21 +0100
Subject: [PATCH 08/13] Add support for '_async_render' fallback and use it

---
 synapse/http/additional_resource.py | 19 +++++--------------
 synapse/http/server.py              | 21 +++++++++++++++++----
 2 files changed, 22 insertions(+), 18 deletions(-)

diff --git a/synapse/http/additional_resource.py b/synapse/http/additional_resource.py
index 096619a8c21b..479746c9c56c 100644
--- a/synapse/http/additional_resource.py
+++ b/synapse/http/additional_resource.py
@@ -13,13 +13,10 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-from twisted.web.resource import Resource
-from twisted.web.server import NOT_DONE_YET
+from synapse.http.server import DirectServeJsonResource
 
-from synapse.http.server import wrap_json_request_handler
 
-
-class AdditionalResource(Resource):
+class AdditionalResource(DirectServeJsonResource):
     """Resource wrapper for additional_resources
 
     If the user has configured additional_resources, we need to wrap the
@@ -41,16 +38,10 @@ def __init__(self, hs, handler):
             handler ((twisted.web.server.Request) -> twisted.internet.defer.Deferred):
                 function to be called to handle the request.
         """
-        Resource.__init__(self)
+        super().__init__()
         self._handler = handler
 
-        # required by the request_handler wrapper
-        self.clock = hs.get_clock()
-
-    def render(self, request):
-        self._async_render(request)
-        return NOT_DONE_YET
-
-    @wrap_json_request_handler
     def _async_render(self, request):
+        # Cheekily pass the result straight through, so we don't need to worry
+        # if its an awaitable or not.
         return self._handler(request)
diff --git a/synapse/http/server.py b/synapse/http/server.py
index 4a4941c15b86..1e2d75f74a96 100644
--- a/synapse/http/server.py
+++ b/synapse/http/server.py
@@ -420,8 +420,11 @@ def _send_error_response(
 
 
 class _DirectServeResource(_AsyncResource):
-    """Base class for resources that will just call `self._async_on_<METHOD>`
+    """Base class for resources that will just call `self._async_render_<METHOD>`
     on new requests, formatting responses and errors as JSON.
+
+    If an `_async_render` function exists then that will be called if no method
+    specific function exists, otherwise returns a 400.
     """
 
     def __init__(self):
@@ -439,14 +442,24 @@ def __init__(self):
                     method_handler
                 )
 
+        fallback_handler = getattr(self, "_async_render", None)
+        if fallback_handler:
+            self._fallback_handler = trace_servlet(self._name)(fallback_handler)
+
+        else:
+            self._fallback_handler = None
+
     def _get_handler_for_request(self, request: SynapseRequest):
         """Implements _AsyncResource._get_handler_for_request
         """
         callback = self._callbacks.get(request.method)
-        if not callback:
-            return _unrecognised_request_handler, "unrecognised_request_handler", {}
+        if callback:
+            return callback, self._name, {}
+
+        if self._fallback_handler:
+            return self._fallback_handler, self._name, {}
 
-        return callback, self._name, {}
+        return _unrecognised_request_handler, "unrecognised_request_handler", {}
 
 
 class DirectServeJsonResource(_DirectServeResource):

From 3bb4db7c35f6e54994ad2ac7522ed7d7916e6616 Mon Sep 17 00:00:00 2001
From: Erik Johnston <erik@matrix.org>
Date: Tue, 23 Jun 2020 14:24:05 +0100
Subject: [PATCH 09/13] Remove unused function and attribute

---
 synapse/http/server.py                      | 27 ---------------------
 synapse/rest/media/v1/thumbnail_resource.py |  1 -
 2 files changed, 28 deletions(-)

diff --git a/synapse/http/server.py b/synapse/http/server.py
index 1e2d75f74a96..d2e9f5edfe76 100644
--- a/synapse/http/server.py
+++ b/synapse/http/server.py
@@ -72,33 +72,6 @@
 MaybeAwaitable = Union[Awaitable[T], T]
 
 
-def wrap_json_request_handler(h):
-    """Wraps a request handler method with exception handling.
-
-    Also does the wrapping with request.processing as per wrap_async_request_handler.
-
-    The handler method must have a signature of "handle_foo(self, request)",
-    where "request" must be a SynapseRequest.
-
-    The handler must return a deferred or a coroutine. If the deferred succeeds
-    we assume that a response has been sent. If the deferred fails with a SynapseError we use
-    it to send a JSON response with the appropriate HTTP reponse code. If the
-    deferred fails with any other type of error we send a 500 reponse.
-    """
-
-    async def wrapped_request_handler(self, request):
-        try:
-            await h(self, request)
-        except Exception:
-            # failure.Failure() fishes the original Failure out
-            # of our stack, and thus gives us a sensible stack
-            # trace.
-            f = failure.Failure()
-            return_json_error(f, request)
-
-    return wrap_async_request_handler(wrapped_request_handler)
-
-
 def return_json_error(f: failure.Failure, request: Request) -> None:
     """Sends a JSON error response to clients.
     """
diff --git a/synapse/rest/media/v1/thumbnail_resource.py b/synapse/rest/media/v1/thumbnail_resource.py
index 23811d0b9673..a83535b97b5e 100644
--- a/synapse/rest/media/v1/thumbnail_resource.py
+++ b/synapse/rest/media/v1/thumbnail_resource.py
@@ -41,7 +41,6 @@ def __init__(self, hs, media_repo, media_storage):
         self.media_storage = media_storage
         self.dynamic_thumbnails = hs.config.dynamic_thumbnails
         self.server_name = hs.hostname
-        self.clock = hs.get_clock()
 
     async def _async_render_GET(self, request):
         set_cors_headers(request)

From 7fee36220ff7a5a901e94cdeac6ec9dff9bc0a72 Mon Sep 17 00:00:00 2001
From: Erik Johnston <erik@matrix.org>
Date: Tue, 23 Jun 2020 15:06:04 +0100
Subject: [PATCH 10/13] Add very basic additonal resource tests

---
 tests/http/test_additional_resource.py | 62 ++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)
 create mode 100644 tests/http/test_additional_resource.py

diff --git a/tests/http/test_additional_resource.py b/tests/http/test_additional_resource.py
new file mode 100644
index 000000000000..0fbf5056e3df
--- /dev/null
+++ b/tests/http/test_additional_resource.py
@@ -0,0 +1,62 @@
+# -*- coding: utf-8 -*-
+# Copyright 2018 New Vector Ltd
+#
+# 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.
+
+
+from synapse.http.additional_resource import AdditionalResource
+from synapse.http.server import respond_with_json
+
+from tests.unittest import HomeserverTestCase
+
+
+class _AsyncTestCustomEndpoint:
+    def __init__(self, config, module_api):
+        pass
+
+    async def handle_request(self, request):
+        respond_with_json(request, 200, {"some_key": "some_value_async"})
+
+
+class _SyncTestCustomEndpoint:
+    def __init__(self, config, module_api):
+        pass
+
+    async def handle_request(self, request):
+        respond_with_json(request, 200, {"some_key": "some_value_sync"})
+
+
+class AdditionalResourceTests(HomeserverTestCase):
+    """Very basic tests that `AdditionalResource` works correctly with sync
+    and async handler.s
+    """
+
+    def test_async(self):
+        handler = _AsyncTestCustomEndpoint({}, None).handle_request
+        self.resource = AdditionalResource(self.hs, handler)
+
+        request, channel = self.make_request("GET", "/")
+        self.render(request)
+
+        self.assertEqual(request.code, 200)
+        self.assertEqual(channel.json_body, {"some_key": "some_value_async"})
+
+    def test_sync(self):
+        handler = _SyncTestCustomEndpoint({}, None).handle_request
+        self.resource = AdditionalResource(self.hs, handler)
+
+        request, channel = self.make_request("GET", "/")
+        self.render(request)
+
+        self.assertEqual(request.code, 200)
+        self.assertEqual(channel.json_body, {"some_key": "some_value_sync"})

From 40b84a0f7fadf66185aa71494a97a35ee17bf6fa Mon Sep 17 00:00:00 2001
From: Erik Johnston <erik@matrix.org>
Date: Thu, 25 Jun 2020 15:37:47 +0100
Subject: [PATCH 11/13] Rejig everything

---
 synapse/federation/transport/server.py |   6 +-
 synapse/http/server.py                 | 231 +++++++++----------------
 synapse/logging/opentracing.py         |  67 ++++---
 synapse/replication/http/__init__.py   |   4 +-
 synapse/replication/http/_base.py      |  11 +-
 5 files changed, 119 insertions(+), 200 deletions(-)

diff --git a/synapse/federation/transport/server.py b/synapse/federation/transport/server.py
index af4595498c0b..bfb7831a02db 100644
--- a/synapse/federation/transport/server.py
+++ b/synapse/federation/transport/server.py
@@ -361,11 +361,7 @@ def register(self, server):
                 continue
 
             server.register_paths(
-                method,
-                (pattern,),
-                self._wrap(code),
-                self.__class__.__name__,
-                trace=False,
+                method, (pattern,), self._wrap(code), self.__class__.__name__,
             )
 
 
diff --git a/synapse/http/server.py b/synapse/http/server.py
index d2e9f5edfe76..785663680e6b 100644
--- a/synapse/http/server.py
+++ b/synapse/http/server.py
@@ -22,7 +22,7 @@
 import urllib
 from http import HTTPStatus
 from io import BytesIO
-from typing import Any, Awaitable, Callable, Dict, Optional, Tuple, TypeVar, Union
+from typing import Any, Awaitable, Tuple, TypeVar, Union
 
 import jinja2
 from canonicaljson import encode_canonical_json, encode_pretty_printed_json, json
@@ -211,39 +211,46 @@ def register_paths(self, method, path_patterns, callback):
 
 class _AsyncResource(resource.Resource, metaclass=abc.ABCMeta):
     """Base class for resources that have async handlers.
+
+    Args:
+        extract_context: Whether to attempt to extract the opentracing
+            context from the request the servlet is handling.
     """
 
+    def __init__(self, extract_context=False):
+        super().__init__()
+
+        self._extract_context = extract_context
+
     def render(self, request):
         """ This gets called by twisted every time someone sends us a request.
         """
-        defer.ensureDeferred(self._async_render(request))
+        defer.ensureDeferred(self._async_render_wrapper(request))
         return NOT_DONE_YET
 
     @wrap_async_request_handler
-    async def _async_render(self, request):
-        """ This gets called from render() every time someone sends us a request.
-
-        Calls `self._get_handler_for_request` to get the callback to use.
+    async def _async_render_wrapper(self, request):
+        """This is a wrapper that delegates to `_async_render`,
         """
         try:
-            callback, servlet_classname, group_dict = self._get_handler_for_request(
-                request
-            )
+            request.request_metrics.name = self.__class__.__name__
 
-            # Make sure we have a name for this handler in prometheus.
-            request.request_metrics.name = servlet_classname
-
-            # Now trigger the callback. If it returns a response, we send it
-            # here. If it throws an exception, that is handled by the wrapper
-            # installed by @request_handler.
-            kwargs = intern_dict(
-                {
-                    name: urllib.parse.unquote(value) if value else value
-                    for name, value in group_dict.items()
-                }
-            )
+            with trace_servlet(request, self._extract_context):
+                callback_return = await self._async_render(request)
 
-            raw_callback_return = callback(request, **kwargs)
+                if callback_return is not None:
+                    code, response = callback_return
+                    self._send_response(request, code, response)
+        except Exception:
+            f = failure.Failure()
+            self._send_error_response(f, request)
+
+    async def _async_render(self, request):
+        method_handler = getattr(
+            self, "_async_render_%s" % (request.method.decode("ascii"),), None
+        )
+        if method_handler:
+            raw_callback_return = method_handler(request)
 
             # Is it synchronous? We'll allow this for now.
             if isinstance(raw_callback_return, (defer.Deferred, types.CoroutineType)):
@@ -251,32 +258,9 @@ async def _async_render(self, request):
             else:
                 callback_return = raw_callback_return
 
-            if callback_return is not None:
-                code, response = callback_return
-                self._send_response(request, code, response)
-        except Exception:
-            f = failure.Failure()
-            self._send_error_response(f, request)
+            return callback_return
 
-    @abc.abstractmethod
-    def _get_handler_for_request(
-        self, request: SynapseRequest
-    ) -> Tuple[
-        Callable[..., MaybeAwaitable[Optional[ResponseTuple]]], str, Dict[str, str],
-    ]:
-        """Finds a callback method to handle the given request.
-
-        Returns:
-            A tuple of callback method, the label to use for that method in
-            prometheus metrics, and a dict to pass as kwargs to the callback
-            (typically mapping keys to path components as specified in the
-            handler's path match regexp).
-
-            The returned callback should either return a tuple of response code
-            and response object that will be passed to `self._send_response` or
-            None if the callback writes the response itself.
-        """
-        raise NotImplementedError()
+        _unrecognised_request_handler(request)
 
     @abc.abstractmethod
     def _send_response(
@@ -291,7 +275,35 @@ def _send_error_response(
         raise NotImplementedError()
 
 
-class JsonResource(HttpServer, _AsyncResource):
+class DirectServeJsonResource(_AsyncResource):
+    """A resource that will call `self._async_on_<METHOD>` on new requests,
+    formatting responses and errors as JSON.
+    """
+
+    def _send_response(
+        self, request, code, response_object,
+    ):
+        """Implements _AsyncResource._send_response
+        """
+        # TODO: Only enable CORS for the requests that need it.
+        respond_with_json(
+            request,
+            code,
+            response_object,
+            send_cors=True,
+            pretty_print=_request_user_agent_is_curl(request),
+            canonical_json=self.canonical_json,
+        )
+
+    def _send_error_response(
+        self, f: failure.Failure, request: SynapseRequest,
+    ) -> None:
+        """Implements _AsyncResource._send_error_response
+        """
+        return_json_error(f, request)
+
+
+class JsonResource(DirectServeJsonResource):
     """ This implements the HttpServer interface and provides JSON support for
     Resources.
 
@@ -311,17 +323,15 @@ class JsonResource(HttpServer, _AsyncResource):
         "_PathEntry", ["pattern", "callback", "servlet_classname"]
     )
 
-    def __init__(self, hs, canonical_json=True):
-        resource.Resource.__init__(self)
+    def __init__(self, hs, canonical_json=True, extract_context=False):
+        super().__init__(extract_context)
 
         self.canonical_json = canonical_json
         self.clock = hs.get_clock()
         self.path_regexs = {}
         self.hs = hs
 
-    def register_paths(
-        self, method, path_patterns, callback, servlet_classname, trace=True
-    ):
+    def register_paths(self, method, path_patterns, callback, servlet_classname):
         """
         Registers a request handler against a regular expression. Later request URLs are
         checked against these regular expressions in order to identify an appropriate
@@ -337,16 +347,9 @@ def register_paths(
 
             servlet_classname (str): The name of the handler to be used in prometheus
                 and opentracing logs.
-
-            trace (bool): Whether we should start a span to trace the servlet.
         """
         method = method.encode("utf-8")  # method is bytes on py3
 
-        if trace:
-            # We don't extract the context from the servlet because we can't
-            # trust the sender
-            callback = trace_servlet(servlet_classname)(callback)
-
         for path_pattern in path_patterns:
             logger.debug("Registering for %s %s", method, path_pattern.pattern)
             self.path_regexs.setdefault(method, []).append(
@@ -369,101 +372,33 @@ def _get_handler_for_request(self, request: SynapseRequest):
         # Huh. No one wanted to handle that? Fiiiiiine. Send 400.
         return _unrecognised_request_handler, "unrecognised_request_handler", {}
 
-    def _send_response(
-        self, request, code, response_object,
-    ):
-        """Implements _AsyncResource._send_response
-        """
-        # TODO: Only enable CORS for the requests that need it.
-        respond_with_json(
-            request,
-            code,
-            response_object,
-            send_cors=True,
-            pretty_print=_request_user_agent_is_curl(request),
-            canonical_json=self.canonical_json,
+    async def _async_render(self, request):
+        callback, servlet_classname, group_dict = self._get_handler_for_request(request)
+
+        request.request_metrics.name = servlet_classname
+
+        # Now trigger the callback. If it returns a response, we send it
+        # here. If it throws an exception, that is handled by the wrapper
+        # installed by @request_handler.
+        kwargs = intern_dict(
+            {
+                name: urllib.parse.unquote(value) if value else value
+                for name, value in group_dict.items()
+            }
         )
 
-    def _send_error_response(
-        self, f: failure.Failure, request: SynapseRequest,
-    ) -> None:
-        """Implements _AsyncResource._send_error_response
-        """
-        return_json_error(f, request)
-
-
-class _DirectServeResource(_AsyncResource):
-    """Base class for resources that will just call `self._async_render_<METHOD>`
-    on new requests, formatting responses and errors as JSON.
-
-    If an `_async_render` function exists then that will be called if no method
-    specific function exists, otherwise returns a 400.
-    """
-
-    def __init__(self):
-        super().__init__()
-
-        self._callbacks = (
-            {}
-        )  # type: Dict[bytes, Callable[..., Awaitable[Optional[Tuple[int, Any]]]]]
-        self._name = self.__class__.__name__
-
-        for method in ("GET", "PUT", "POST", "OPTIONS", "DELETE"):
-            method_handler = getattr(self, "_async_render_%s" % (method,), None)
-            if method_handler:
-                self._callbacks[method.encode("ascii")] = trace_servlet(self._name)(
-                    method_handler
-                )
-
-        fallback_handler = getattr(self, "_async_render", None)
-        if fallback_handler:
-            self._fallback_handler = trace_servlet(self._name)(fallback_handler)
+        raw_callback_return = callback(request, **kwargs)
 
+        # Is it synchronous? We'll allow this for now.
+        if isinstance(raw_callback_return, (defer.Deferred, types.CoroutineType)):
+            callback_return = await raw_callback_return
         else:
-            self._fallback_handler = None
-
-    def _get_handler_for_request(self, request: SynapseRequest):
-        """Implements _AsyncResource._get_handler_for_request
-        """
-        callback = self._callbacks.get(request.method)
-        if callback:
-            return callback, self._name, {}
-
-        if self._fallback_handler:
-            return self._fallback_handler, self._name, {}
+            callback_return = raw_callback_return
 
-        return _unrecognised_request_handler, "unrecognised_request_handler", {}
-
-
-class DirectServeJsonResource(_DirectServeResource):
-    """A resource that will call `self._async_on_<METHOD>` on new requests,
-    formatting responses and errors as JSON.
-    """
-
-    def _send_response(
-        self, request, code, response_object,
-    ):
-        """Implements _AsyncResource._send_response
-        """
-        # TODO: Only enable CORS for the requests that need it.
-        respond_with_json(
-            request,
-            code,
-            response_object,
-            send_cors=True,
-            pretty_print=_request_user_agent_is_curl(request),
-            canonical_json=self.canonical_json,
-        )
-
-    def _send_error_response(
-        self, f: failure.Failure, request: SynapseRequest,
-    ) -> None:
-        """Implements _AsyncResource._send_error_response
-        """
-        return_json_error(f, request)
+        return callback_return
 
 
-class DirectServeHtmlResource(_DirectServeResource):
+class DirectServeHtmlResource(_AsyncResource):
     """A resource that will call `self._async_on_<METHOD>` on new requests,
     formatting responses and errors as HTML.
     """
diff --git a/synapse/logging/opentracing.py b/synapse/logging/opentracing.py
index 73bef5e5ca87..cbcc41aaeea9 100644
--- a/synapse/logging/opentracing.py
+++ b/synapse/logging/opentracing.py
@@ -169,7 +169,6 @@ def set_fates(clotho, lachesis, atropos, father="Zues", mother="Themis"):
 import inspect
 import logging
 import re
-import types
 from functools import wraps
 from typing import TYPE_CHECKING, Dict, Optional, Type
 
@@ -182,6 +181,7 @@ def set_fates(clotho, lachesis, atropos, father="Zues", mother="Themis"):
 
 if TYPE_CHECKING:
     from synapse.server import HomeServer
+    from synapse.http.site import SynapseRequest
 
 # Helper class
 
@@ -793,48 +793,41 @@ def _tag_args_inner(*args, **kwargs):
     return _tag_args_inner
 
 
-def trace_servlet(servlet_name, extract_context=False):
-    """Decorator which traces a serlet. It starts a span with some servlet specific
-    tags such as the servlet_name and request information
+@contextlib.contextmanager
+def trace_servlet(request: "SynapseRequest", extract_context: bool = False):
+    """Context manager which traces a servlet. It starts a span with some
+    servlet specific tags such as the servlet_name and request information.
 
     Args:
-        servlet_name (str): The name to be used for the span's operation_name
-        extract_context (bool): Whether to attempt to extract the opentracing
+        request
+        extract_context: Whether to attempt to extract the opentracing
             context from the request the servlet is handling.
-
     """
 
-    def _trace_servlet_inner_1(func):
-        if not opentracing:
-            return func
-
-        @wraps(func)
-        async def _trace_servlet_inner(request, *args, **kwargs):
-            request_tags = {
-                "request_id": request.get_request_id(),
-                tags.SPAN_KIND: tags.SPAN_KIND_RPC_SERVER,
-                tags.HTTP_METHOD: request.get_method(),
-                tags.HTTP_URL: request.get_redacted_uri(),
-                tags.PEER_HOST_IPV6: request.getClientIP(),
-            }
-
-            if extract_context:
-                scope = start_active_span_from_request(
-                    request, servlet_name, tags=request_tags
-                )
-            else:
-                scope = start_active_span(servlet_name, tags=request_tags)
-
-            with scope:
-                result = func(request, *args, **kwargs)
+    if opentracing is None:
+        yield
+        return
 
-                if not isinstance(result, (types.CoroutineType, defer.Deferred)):
-                    # Some servlets aren't async and just return results
-                    # directly, so we handle that here.
-                    return result
+    request_tags = {
+        "request_id": request.get_request_id(),
+        tags.SPAN_KIND: tags.SPAN_KIND_RPC_SERVER,
+        tags.HTTP_METHOD: request.get_method(),
+        tags.HTTP_URL: request.get_redacted_uri(),
+        tags.PEER_HOST_IPV6: request.getClientIP(),
+    }
 
-                return await result
+    request_name = request.request_metrics.name
+    if extract_context:
+        scope = start_active_span_from_request(request, request_name, tags=request_tags)
+    else:
+        scope = start_active_span(request_name, tags=request_tags)
 
-        return _trace_servlet_inner
+    with scope:
+        try:
+            yield
+        finally:
+            # We set the operation name again in case its changed (which happens
+            # with JsonResource).
+            scope.span.set_operation_name(request.request_metrics.name)
 
-    return _trace_servlet_inner_1
+            scope.span.set_tag("request_tag", request.request_metrics.start_context.tag)
diff --git a/synapse/replication/http/__init__.py b/synapse/replication/http/__init__.py
index 19b69e0e113b..1c7b0111c3f7 100644
--- a/synapse/replication/http/__init__.py
+++ b/synapse/replication/http/__init__.py
@@ -30,7 +30,9 @@
 
 class ReplicationRestResource(JsonResource):
     def __init__(self, hs):
-        JsonResource.__init__(self, hs, canonical_json=False)
+        # We enabling extracting jaeger contexts here as these are internal
+        # APIs.
+        super().__init__(hs, canonical_json=False, extract_context=True)
         self.register_servlets(hs)
 
     def register_servlets(self, hs):
diff --git a/synapse/replication/http/_base.py b/synapse/replication/http/_base.py
index 9caf1e80c1b6..0843d28d4b15 100644
--- a/synapse/replication/http/_base.py
+++ b/synapse/replication/http/_base.py
@@ -28,11 +28,7 @@
     RequestSendFailed,
     SynapseError,
 )
-from synapse.logging.opentracing import (
-    inject_active_span_byte_dict,
-    trace,
-    trace_servlet,
-)
+from synapse.logging.opentracing import inject_active_span_byte_dict, trace
 from synapse.util.caches.response_cache import ResponseCache
 from synapse.util.stringutils import random_string
 
@@ -240,11 +236,8 @@ def register(self, http_server):
         args = "/".join("(?P<%s>[^/]+)" % (arg,) for arg in url_args)
         pattern = re.compile("^/_synapse/replication/%s/%s$" % (self.NAME, args))
 
-        handler = trace_servlet(self.__class__.__name__, extract_context=True)(handler)
-        # We don't let register paths trace this servlet using the default tracing
-        # options because we wish to extract the context explicitly.
         http_server.register_paths(
-            method, [pattern], handler, self.__class__.__name__, trace=False
+            method, [pattern], handler, self.__class__.__name__,
         )
 
     def _cached_handler(self, request, txn_id, **kwargs):

From f0f2baf93d0e9d1d33a6453533e540574b0dbda0 Mon Sep 17 00:00:00 2001
From: Erik Johnston <erik@matrix.org>
Date: Tue, 30 Jun 2020 11:31:58 +0100
Subject: [PATCH 12/13] Fix up comments and unused vars

---
 synapse/http/server.py               | 39 ++++++++++++++++------------
 synapse/logging/opentracing.py       |  5 ++--
 synapse/replication/http/__init__.py |  3 +--
 3 files changed, 26 insertions(+), 21 deletions(-)

diff --git a/synapse/http/server.py b/synapse/http/server.py
index 785663680e6b..502a2049bf8b 100644
--- a/synapse/http/server.py
+++ b/synapse/http/server.py
@@ -22,7 +22,7 @@
 import urllib
 from http import HTTPStatus
 from io import BytesIO
-from typing import Any, Awaitable, Tuple, TypeVar, Union
+from typing import Any, Callable, Dict, Tuple, Union
 
 import jinja2
 from canonicaljson import encode_canonical_json, encode_pretty_printed_json, json
@@ -62,17 +62,8 @@
 </html>
 """
 
-T = TypeVar("T")
 
-# A tuple of HTTP response code and response body. For JSON requests the body is
-# anything JSON serializable, while for HTML requests they are bytes.
-ResponseTuple = Tuple[int, Any]
-
-# Used for functions that may or may not return an awaitable object.
-MaybeAwaitable = Union[Awaitable[T], T]
-
-
-def return_json_error(f: failure.Failure, request: Request) -> None:
+def return_json_error(f: failure.Failure, request: SynapseRequest) -> None:
     """Sends a JSON error response to clients.
     """
 
@@ -109,9 +100,6 @@ def return_json_error(f: failure.Failure, request: Request) -> None:
         )
 
 
-TV = TypeVar("TV")
-
-
 def return_html_error(
     f: failure.Failure, request: Request, error_template: Union[str, jinja2.Template],
 ) -> None:
@@ -212,6 +200,9 @@ def register_paths(self, method, path_patterns, callback):
 class _AsyncResource(resource.Resource, metaclass=abc.ABCMeta):
     """Base class for resources that have async handlers.
 
+    Sub classes can either implement `_async_render_<METHOD>` to handle
+    requests by method, or override `_async_render` to handle all requests.
+
     Args:
         extract_context: Whether to attempt to extract the opentracing
             context from the request the servlet is handling.
@@ -230,7 +221,8 @@ def render(self, request):
 
     @wrap_async_request_handler
     async def _async_render_wrapper(self, request):
-        """This is a wrapper that delegates to `_async_render`,
+        """This is a wrapper that delegates to `_async_render` and handles
+        exceptions, return values, metrics, etc.
         """
         try:
             request.request_metrics.name = self.__class__.__name__
@@ -246,6 +238,11 @@ async def _async_render_wrapper(self, request):
             self._send_error_response(f, request)
 
     async def _async_render(self, request):
+        """Delegates to `_async_render_<METHOD>` methods, or returns a 400 if
+        no appropriate method exists. Can be overriden in sub classes for
+        different routing.
+        """
+
         method_handler = getattr(
             self, "_async_render_%s" % (request.method.decode("ascii"),), None
         )
@@ -356,8 +353,14 @@ def register_paths(self, method, path_patterns, callback, servlet_classname):
                 self._PathEntry(path_pattern, callback, servlet_classname)
             )
 
-    def _get_handler_for_request(self, request: SynapseRequest):
-        """Implements _AsyncResource._get_handler_for_request
+    def _get_handler_for_request(
+        self, request: SynapseRequest
+    ) -> Tuple[Callable, str, Dict[str, str]]:
+        """Finds a callback method to handle the given request.
+
+        Returns:
+            A tuple of the callback to use, the name of the servlet, and the
+            key word arguments to pass to the callback
         """
         request_path = request.path.decode("ascii")
 
@@ -375,6 +378,8 @@ def _get_handler_for_request(self, request: SynapseRequest):
     async def _async_render(self, request):
         callback, servlet_classname, group_dict = self._get_handler_for_request(request)
 
+        # Make sure we have an appopriate name for this handler in prometheus
+        # (rather than the default of JsonResource).
         request.request_metrics.name = servlet_classname
 
         # Now trigger the callback. If it returns a response, we send it
diff --git a/synapse/logging/opentracing.py b/synapse/logging/opentracing.py
index cbcc41aaeea9..1676771ef0fb 100644
--- a/synapse/logging/opentracing.py
+++ b/synapse/logging/opentracing.py
@@ -795,8 +795,9 @@ def _tag_args_inner(*args, **kwargs):
 
 @contextlib.contextmanager
 def trace_servlet(request: "SynapseRequest", extract_context: bool = False):
-    """Context manager which traces a servlet. It starts a span with some
-    servlet specific tags such as the servlet_name and request information.
+    """Returns a context manager which traces a request. It starts a span
+    with some servlet specific tags such as the request metrics name and
+    request information.
 
     Args:
         request
diff --git a/synapse/replication/http/__init__.py b/synapse/replication/http/__init__.py
index 1c7b0111c3f7..5ef1c6c1dcce 100644
--- a/synapse/replication/http/__init__.py
+++ b/synapse/replication/http/__init__.py
@@ -30,8 +30,7 @@
 
 class ReplicationRestResource(JsonResource):
     def __init__(self, hs):
-        # We enabling extracting jaeger contexts here as these are internal
-        # APIs.
+        # We enable extracting jaeger contexts here as these are internal APIs.
         super().__init__(hs, canonical_json=False, extract_context=True)
         self.register_servlets(hs)
 

From e0f89861c52ef48820a4a9dee5e65c622f73ffa8 Mon Sep 17 00:00:00 2001
From: Erik Johnston <erik@matrix.org>
Date: Fri, 3 Jul 2020 14:23:46 +0100
Subject: [PATCH 13/13] Add back comments. Use respond_with_html_bytes

---
 synapse/http/server.py                 | 11 ++++++-----
 tests/http/test_additional_resource.py |  2 +-
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/synapse/http/server.py b/synapse/http/server.py
index a7c9daf2b390..2b35f8606662 100644
--- a/synapse/http/server.py
+++ b/synapse/http/server.py
@@ -83,6 +83,8 @@ def return_json_error(f: failure.Failure, request: SynapseRequest) -> None:
             exc_info=(f.type, f.value, f.getTracebackObject()),
         )
 
+    # Only respond with an error response if we haven't already started writing,
+    # otherwise lets just kill the connection
     if request.startedWriting:
         if request.transport:
             try:
@@ -229,6 +231,9 @@ async def _async_render_wrapper(self, request):
                     code, response = callback_return
                     self._send_response(request, code, response)
         except Exception:
+            # failure.Failure() fishes the original Failure out
+            # of our stack, and thus gives us a sensible stack
+            # trace.
             f = failure.Failure()
             self._send_error_response(f, request)
 
@@ -415,11 +420,7 @@ def _send_response(
         assert isinstance(response_object, bytes)
         html_bytes = response_object
 
-        request.setResponseCode(200)
-        request.setHeader(b"Content-Type", b"text/html; charset=utf-8")
-        request.setHeader(b"Content-Length", b"%d" % (len(html_bytes),))
-        request.write(html_bytes)
-        finish_request(request)
+        respond_with_html_bytes(request, 200, html_bytes)
 
     def _send_error_response(
         self, f: failure.Failure, request: SynapseRequest,
diff --git a/tests/http/test_additional_resource.py b/tests/http/test_additional_resource.py
index 0fbf5056e3df..62d36c29060c 100644
--- a/tests/http/test_additional_resource.py
+++ b/tests/http/test_additional_resource.py
@@ -38,7 +38,7 @@ async def handle_request(self, request):
 
 class AdditionalResourceTests(HomeserverTestCase):
     """Very basic tests that `AdditionalResource` works correctly with sync
-    and async handler.s
+    and async handlers.
     """
 
     def test_async(self):