This repository was 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
Add @cancellable decorator, for use on endpoint methods that can be cancelled when clients disconnect#12583
@cancellable decorator, for use on endpoint methods that can be cancelled when clients disconnect#12583Conversation
5fb9b3f to
7d3c2d3
Compare
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>
7d3c2d3 to
a89fc72
Compare
This was referenced Apr 28, 2022
squahtx
commented
May 5, 2022
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. | ||
|
|
Contributor
Author
There was a problem hiding this comment.
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.
synapse/synapse/rest/client/room.py
Lines 133 to 138 in 4bc8cb4
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(
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>
This was referenced May 10, 2022
Contributor
Author
|
All reviewed and merged now. Thank you to everyone who reviewed! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Best reviewed commit by commit.
I'm going to try to break this up into smaller PRs.
Add a
@cancellabledecorator that can be used on async HTTP endpoints.In Synapse, we have 5 ways of defining async HTTP endpoints:
RestServletsubclasses withon_$METHODmethodsBaseFederationServletsubclasses withon_$METHODmethodsReplicationEndpointsubclasses with a_handle_requestimplementation
DirectServe{Html,Json}Resourcesubclasses with_async_render_$METHODmethodsAll of these get invoked indirectly by
_AsyncResource.render, whichstarts the async processing.
DirectServeHtmlResourceandDirectServeJsonResourceinherit from_AsyncResourcedirectly, while the rest register themselves usingregister_pathson aJsonResource, which inherits from_AsyncResource.Going to split this up into smaller PRs like so: