Cancelling Deferreds to abort processing looks very promising!

How Deferred.cancel() works

When Deferred.cancel() is called on a Deferred wrapping a coroutine, the call propagates down into the Deferred the coroutine is waiting on. That Deferred will either propagate the cancel() further or fail with a twisted.internet.defer.CancelledError, which gets raised inside the coroutine. This means that any await expression may raise a CancelledError, including trivial ones like sleep()!

The coroutine may then deal with the CancelledError using normal exception handling. The Deferred wrapping the coroutine will not automatically fail with a CancelledError unless it bubbles up through the coroutine.

asyncio has a similar feature, where an asyncio.CancelledError will be raised instead. It's interesting to note that twisted CancelledErrors are Exceptions, while asyncio CancelledErrors are BaseExceptions.

Complications

Unfortunately aborting processing of requests isn't as simple as calling Deferred.cancel() when clients disconnect:

In many places, we catch and swallow plain Exceptions without regard for CancelledErrors. The result is that cancellation is ignored, which isn't too bad, since that's almost what happens today.
More importantly, we have quite a few constructs (gatherResults, ObservableDeferred, ReadWriteLock, DeferredCache, run_in_background, defer_to_thread and more) which do fancy things with Deferreds and may not have been designed with cancellation in mind.
Some of them allow cancel() calls to propagate when they shouldn't, don't propagate cancel() calls when they should or don't clean up after CancelledErrors correctly when they come from unexpected places.
This can result in stuck requests piling up and all sorts of general weirdness. Ideally each construct needs to be checked, tested and fixed which is going to be very time-consuming.

Perhaps we could limit cancellation to a handful of interesting endpoints, like /sync? That way we wouldn't have to make all of synapse support cancellation right now. Though that might not be much help since all the interesting endpoints probably make use of many of the constructs that need checking.

Uh oh!

we shouldn't process requests where the requester has timed out/gone away #3528

Description

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions