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

Add ability to cancel disconnected requests to SynapseRequest #12588

Merged
merged 4 commits into from
May 10, 2022
Merged

Add ability to cancel disconnected requests to SynapseRequest #12588

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0a150e9
Add ability to cancel disconnected requests to `SynapseRequest`
Apr 28, 2022
406e40b
Add newsfile
Apr 28, 2022
647256c
Don't trash the logging context when cancelling request processing
May 6, 2022
46f96b9
Update newsfile, since all the related PRs won't land in the same rel…
May 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/12588.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add `@cancellable` decorator, for use on endpoint methods that can be cancelled when clients disconnect.
23 changes: 22 additions & 1 deletion synapse/http/site.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import attr
from zope.interface import implementer

from twisted.internet.defer import Deferred
from twisted.internet.interfaces import IAddress, IReactorTime
from twisted.python.failure import Failure
from twisted.web.http import HTTPChannel
Expand Down Expand Up @@ -91,6 +92,13 @@ def __init__(
# we can't yet create the logcontext, as we don't know the method.
self.logcontext: Optional[LoggingContext] = None

# The `Deferred` to cancel if the client disconnects early. Expected to be set
# by `Resource.render`.
self.render_deferred: Optional["Deferred[None]"] = None
# A boolean indicating whether `_render_deferred` should be cancelled if the
# client disconnects early. Expected to be set during `Resource.render`.
self.is_render_cancellable = False

global _next_request_seq
self.request_seq = _next_request_seq
_next_request_seq += 1
Expand Down Expand Up @@ -357,7 +365,20 @@ def connectionLost(self, reason: Union[Failure, Exception]) -> None:
{"event": "client connection lost", "reason": str(reason.value)}
)

if not self._is_processing:
if self._is_processing:
if self.is_render_cancellable:
if self.render_deferred is not None:
# Throw a cancellation into the request processing, in the hope
# that it will finish up sooner than it normally would.
# The `self.processing()` context manager will call
# `_finished_processing()` when done.
self.render_deferred.cancel()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It turns out Deferred.cancel() is a lot like Deferred.callback()/errback() in that it will trash the logging context:
it can resume a coroutine, which will restore its own logging context, then run:

  • until it blocks, setting the sentinel context
  • or until it terminates, setting the context it was started with

So we need to wrap it in with PreserveLoggingContext():, like we do with .callback():

Suggested change
self.render_deferred.cancel()
with PreserveLoggingContext():
self.render_deferred.cancel()

Copy link
Contributor

Choose a reason for hiding this comment

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

This makes sense to me!

else:
logger.error(
"Connection from client lost, but have no Deferred to "
"cancel even though the request is marked as cancellable."
)
else:
self._finished_processing()

def _started_processing(self, servlet_name: str) -> None:
Expand Down