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

Add @cancellable decorator, for use on endpoint methods that can be cancelled when clients disconnect #12583

Closed
wants to merge 21 commits into from

Conversation

squahtx
Copy link
Contributor

@squahtx squahtx commented Apr 28, 2022

Best reviewed commit by commit.
I'm going to try to break this up into smaller PRs.


Add a @cancellable decorator that can be used on async HTTP endpoints.

In Synapse, we have 5 ways of defining async HTTP endpoints:

  • RestServlet subclasses with on_$METHOD methods
  • BaseFederationServlet subclasses with on_$METHOD methods
  • ReplicationEndpoint subclasses with a _handle_request
    implementation
  • DirectServe{Html,Json}Resource subclasses with
    _async_render_$METHOD methods

All of these get invoked indirectly by _AsyncResource.render, which
starts the async processing.
DirectServeHtmlResource and DirectServeJsonResource inherit from
_AsyncResource directly, while the rest register themselves using
register_paths on a JsonResource, which inherits from
_AsyncResource.


Going to split this up into smaller PRs like so:

#12586 (merged)
2780bedb5185d729d03faa4cc4739862609539c4 Add `@cancellable` decorator, for use on request handlers
|
|   #12587 (merged)
|   1ce7dbf42c24c139683e58cd79d0ad6f16502219 Improve logging for cancelled requests
|   |
|   |   #12588 (merged)
|   |   0dc4178587b65313c7d81b5f4880082f1c2e3d11 Add ability to cancel disconnected requests to `SynapseRequest`
|   |   |   |
|   |   |   v
|   |   |   #12694 (merged)
|   |   |   5a9991c0f93b71b5eac8d6c47b1ba6d5bb2004c7 Capture the `Deferred` for request cancellation in `_AsyncResource`
|   |   |   |
|   |   |   |   #12630 (merged)
|   +---------->3d89472339c9d506fa87ad21d57f502ff4b9c342 Expose the `SynapseRequest` from `FakeChannel` for testing disconnection
|   |   |   |   2bbad2930db9b373b149f295d71d5bb49748c1ed Add helper class for testing request cancellation
|   |   |   |   |
+<--+<--+<--+<--+
|
|   #12698 (merged)
+-->46cdb4bd0746df6aad0b2408beb35b38b5a94c18 Respect the `@cancellable` flag for `DirectServe{Html,Json}Resource`s
|   92b7b17c3df6d71dea82e64e8c97b81c6cd1a76b Test the `@cancellable` flag on `DirectServe{Html,Json}Resource` methods
|
|   #12699 (merged)
+-->2326bbf0999e99b57dafcebd966dd9bd942c52a2 Respect the `@cancellable` flag for `RestServlet`s and `BaseFederationServlet`s
|   6720b8780f57c4d361a82fb6a539781f4f0d63f6 Test the `@cancellable` flag on `RestServlet` methods
|   3544cfdaa192831cc59684abd5618d7748bd6f1f Fix `make_signed_federation_request` turning empty dicts into `b""`
|   89cb0f140e5d81b3f3f049029ffeb65fa36d2c61 Test the `@cancellable` flag on `BaseFederationServlet` methods
|   |
|   v
|   #12705 (merged)
|   c3eb1e3358be4453c36426ac5b117c9ae7d0fec3 Complain if a federation endpoint has the `@cancellable` flag
|   342a502a1e08968dd2643af46eaf4105b086edf9 Disable tests for the `@cancellable` flag on `BaseFederationServlet` methods
|
|   #12700 (merged)
+-->62d3b915a5eb110b36e75107d77dd161305bf7e9 Respect the `@cancellable` flag for `ReplicationEndpoint`s
    d3f75f3c9430059a4b9e70b56eb344d70b6c693f Test the `@cancellable` flag on `ReplicationEndpoint._handle_request`

@squahtx squahtx requested a review from a team as a code owner April 28, 2022 17:27
@squahtx squahtx marked this pull request as draft April 28, 2022 17:28
@squahtx squahtx force-pushed the squah/add_endpoint_cancellation_flag branch from 5fb9b3f to 7d3c2d3 Compare April 28, 2022 17:57
Sean Quah added 17 commits April 28, 2022 19:28
Signed-off-by: Sean Quah <seanq@element.io>
Don't log stack traces for cancelled requests and use a custom HTTP
status code of 499.

Signed-off-by: Sean Quah <seanq@element.io>
Signed-off-by: Sean Quah <seanq@element.io>
All async request processing goes through `_AsyncResource`, so this is
the only place where a `Deferred` needs to be captured for cancellation.

Unfortunately, the same isn't true for determining whether a request
can be cancelled. Each of `RestServlet`, `BaseFederationServlet`,
`DirectServe{Html,Json}Resource` and `ReplicationEndpoint` have
different wrappers around the method doing the request handling and they
all need to be handled separately.

Signed-off-by: Sean Quah <seanq@element.io>
`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 <seanq@element.io>
…nServlet`s

Both `RestServlet`s and `BaseFederationServlet`s register their handlers
with `HttpServer.register_paths` / `JsonResource.register_paths`. Update
`JsonResource` to respect the `@cancellable` flag on handlers registered
in this way.

Although `ReplicationEndpoint` also registers itself using
`register_paths`, it does not pass the handler method that would have the
`@cancellable` flag directly, and so needs separate handling.

Signed-off-by: Sean Quah <seanq@element.io>
`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 <seanq@element.io>
While `ReplicationEndpoint`s register themselves via `JsonResource`,
they pass a method that calls the handler, instead of the handler itself,
to `register_paths`. As a result, `JsonResource` will not correctly pick
up the `@cancellable` flag and we have to apply it ourselves.

Signed-off-by: Sean Quah <seanq@element.io>
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 <seanq@element.io>
Signed-off-by: Sean Quah <seanq@element.io>
Signed-off-by: Sean Quah <seanq@element.io>
Signed-off-by: Sean Quah <seanq@element.io>
Signed-off-by: Sean Quah <seanq@element.io>
Signed-off-by: Sean Quah <seanq@element.io>
Signed-off-by: Sean Quah <seanq@element.io>
…methods

Signed-off-by: Sean Quah <seanq@element.io>
Signed-off-by: Sean Quah <seanq@element.io>
Comment on lines +304 to +306
The callback may be marked with the `@cancellable` decorator, which will
cause request processing to be cancelled when clients disconnect early.

Copy link
Contributor Author

@squahtx squahtx May 5, 2022

Choose a reason for hiding this comment

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

I wonder if we should add an explicit cancellable parameter to register_paths instead.
There are a few places where we call register_paths manually, with callbacks that don't match the ^on_(GET|PUT|POST|DELETE)$ pattern, eg.

http_server.register_paths(
"GET",
client_patterns(no_state_key, v1=True),
self.on_GET_no_state_key,
self.__class__.__name__,
)

There we have the option of either relaxing the validation in the @cancellable decorator or adding an explicit cancellable parameter to register_paths.

The benefit of relaxing the decorator validation is that it's more obvious that on_GET_no_state_key has cancellation enabled:

    @cancellable
    def on_GET_no_state_key(
        self, request: SynapseRequest, room_id: str, event_type: str
    ) -> Awaitable[Tuple[int, JsonDict]]:
        return self.on_GET(request, room_id, event_type, "")

    @cancellable
    async def on_GET(

Sean Quah added 3 commits May 6, 2022 20:54
Signed-off-by: Sean Quah <seanq@element.io>
…HelperMixin`

Signed-off-by: Sean Quah <seanq@element.io>
Signed-off-by: Sean Quah <seanq@element.io>
Signed-off-by: Sean Quah <seanq@element.io>
@squahtx
Copy link
Contributor Author

squahtx commented May 11, 2022

All reviewed and merged now. Thank you to everyone who reviewed!

@squahtx squahtx closed this May 11, 2022
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant