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

Commit

Permalink
Write some more words in docstrings
Browse files Browse the repository at this point in the history
  • Loading branch information
Sean Quah committed May 4, 2022
1 parent 474f85b commit 773a9b6
Showing 1 changed file with 12 additions and 1 deletion.
13 changes: 12 additions & 1 deletion synapse/http/server.py
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,17 @@
def cancellable(method: F) -> F:
"""Marks a servlet method as cancellable.
Methods with this decorator will be cancelled if the client disconnects before we
finish processing the request.
During cancellation, `Deferred.cancel()` will be invoked on the `Deferred` wrapping
the method. The `cancel()` call will propagate down to the `Deferred` that is
currently being waited on. That `Deferred` will raise a `CancelledError`, which will
propagate up, as per normal exception handling.
Before applying this decorator to a new endpoint, it is wise to recursively check
that all `await`s are on coroutines or `Deferred`s that handle cancellation cleanly.

This comment has been minimized.

Copy link
@DMRobertson

DMRobertson May 9, 2022

Contributor

What could go wrong if this wasn't the case?

This comment has been minimized.

Copy link
@squahtx

squahtx May 9, 2022

Contributor

That's a very good question without a single answer.

All sorts of things can happen. So far we have seen:

  • cancellation being ignored (due to module callbacks)
  • logging contexts being finished while they're still in use (due to the @cached decorator)
  • cancellation leaking across requests (due to get_event)
  • all future requests of a certain type getting stuck indefinitely (due to Linearizers)
  • caches not being invalidated correctly (in runInteraction)

It really varies depending on the code. You could imagine database corruption happening if we have some code that splits a write into two transactions and a cancellation happens in the middle. Though that's not really cancellation's fault, since the same thing would happen if synapse died in the middle of the two writes. I'm very reluctant to make any POST endpoints cancellable for this reason.

This comment has been minimized.

Copy link
@DMRobertson

DMRobertson May 9, 2022

Contributor

Does this intentionally exclude other Awaitables like asyncio.Future?

I can see that we need to exclude certain Deferreds; but I'm not sure what's special about coroutines compared to a generic awaitable.

This comment has been minimized.

Copy link
@DMRobertson

DMRobertson May 9, 2022

Contributor

Thanks, that's insightful.

It'd be nice to summarise that and have a stronger warning if possible. Not sure how to word it though. Maybe:

Before applying this decorator to a new endpoint, you MUST recursively check that all awaits in this function are on coroutines or Deferreds that handle cancellation cleanly. Otherwise you can expect a variety of problems, including: premature logging context closure, stuck Linearizers, and incorrect cache invalidation.

Not worth blocking this change on finding the perfect phrasing; just a nice-to-have.

Usage:
class SomeServlet(RestServlet):
@cancellable
Expand All @@ -126,7 +137,7 @@ async def on_GET(self, request: SynapseRequest) -> ...:


def is_method_cancellable(method: Callable[..., Any]) -> bool:
"""Checks whether a servlet method is cancellable."""
"""Checks whether a servlet method has the `@cancellable` flag."""
return getattr(method, "cancellable", False)


Expand Down

0 comments on commit 773a9b6

Please sign in to comment.