Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change web handlers cancellation policy #2492

Closed
asvetlov opened this issue Nov 9, 2017 · 20 comments
Closed

Change web handlers cancellation policy #2492

asvetlov opened this issue Nov 9, 2017 · 20 comments
Labels
Milestone

Comments

@asvetlov
Copy link
Member

asvetlov commented Nov 9, 2017

Cancelling a web handler task on client disconnection is the most unexpected and annoying feature of aiohttp web server.
People from classic WSGI world don't expect it.
Moreover tornado doesn't support a cancellation (I hope it will in 5.0 release).

The proposal is:

  • web-handler cancellation should be opt-in, not opt-out (by aiojobs and async_armor usage).
  • Application should have allow_cancel constructor parameter, False by default.
  • Any record in application route table should accept allow_cancel to override the default.

Opinions?

@webknjaz
Copy link
Member

webknjaz commented Nov 9, 2017

Yes, this should be configurable at (1) global and (2) handler level.
We could provide some decorator (@cancel_safe?) explicitly enabling cancellation at handler level and it would be harder to do this path-dependent (probably via middlewares).
As for constructor, I'd go for subclassing, like: class CalcellableApp(Application):

@asvetlov asvetlov removed the web label Nov 12, 2017
@asvetlov asvetlov added this to the 3.0 milestone Dec 5, 2017
@asvetlov asvetlov modified the milestones: 3.0, 3.1 Feb 9, 2018
@asvetlov asvetlov modified the milestones: 3.1, 3.2 Mar 22, 2018
@asvetlov asvetlov modified the milestones: 3.2, 3.3 May 7, 2018
@asvetlov asvetlov modified the milestones: 3.3, 3.5 Oct 18, 2018
@gjcarneiro
Copy link
Contributor

Personally, I like the current policy. Typically, inside a view only a small portion of the view code needs to be protected from cancellation. For that, you can wrap it inside asyncio.shield().

@asvetlov
Copy link
Member Author

PR #4080 replaces task cancellation with OSError raised on trying to communicate with a closed peer (read request BODY or send data back).
By this, the behavior becomes more obvious; the server code is still notified about peer disconnection but in a more predictable way on better-specified points.

We can close the issue.

@gjcarneiro
Copy link
Contributor

How does this handle the case?

  1. We receive a request;
  2. Handler does a long and complicated DB query;
  3. Client loses patience and closes the connection.

With the old behaviour, the query would be automatically cancelled. With the new PR, it doesn't get cancelled, and so the database is doing a lot of useless work.

@gjcarneiro
Copy link
Contributor

I think the original proposal was reasonable, making cancellation opt-in, and per route, but #4080 just removes cancellation completely. I think #4080 removes an important functionality from aiohttp.

How can we even detect client disconnection? There's only two points where we can: (1) at the beginning, (2) at the end. If the client disconnects 5 seconds into a query that takes 20 seconds, we will not be notified. The handler only finds out at the end of the query, when it's about to write the response.

@asvetlov
Copy link
Member Author

The previous approach required protecting user code from cancellation.
This behavior was constantly ignored by users. Even people remember to add shield/@atomic guards to their code the code becomes overcomplicated without a reason.

In the case mentioned by you (20 sec DB request has client disconnection after 5 sec of running) the normal behavior takes 20 secs. You don't rely on early connection dropping but think that all requests are handled without user interruption normally.

It overloads the server, very long DB queries should be refactored most likely to make the system more responsible. Early cancellation doesn't help at all.

In the case of response streaming the server sends data by chunks. It gets an exception about client disconnection on the next send.

I deemed a lot about the cancellation. Now I'm pretty sure that the user should do nothing with it. aiohttp can implement support for early client disconnection notification but I very doubt if any people will use it; while we should care about the maintenance burden (code, documentation, tests).

@gjcarneiro
Copy link
Contributor

Fine, I kind of understand you reasoning. But I still wish aiohttp made it possible, even if not so simple, for application code to be notified of client disconnections in real time.

As it is now, only when you try to return the response does aiohttp itself get the notification. Application code only gets notification in case of streaming response which, in case of a long DB query, is going to be near the end of query completion.

Maybe we could at least have a simple async def web.Request.poll(): method. Then you could launch your query in the background while polling the client request for connection drop. If web.Request.poll() raises an exception, then you cancel the background task.

Looking through the sources, it seems that I could potentially get request.transport.is_closing() every 250ms or so, while running the background task. That will mostly get me back what I need. Unless of course you decide to remove the BaseRequest.transport property from the public API in the future. Can you at least promise not to do that?

@asvetlov
Copy link
Member Author

What should be Request.poll() behavior?

@gjcarneiro
Copy link
Contributor

Well, to get real time notification of disconnections, something like Request.poll() would do nothing, and simply hang until the client disconnects.

Maybe it needs a better name:

class Request:
    async def wait_for_disconnection(self): ..
        """Does nothing and returns when the socket that sent this request disconnects"""

Then you could have a handler like:

async def handler(request):
   wait = asyncio.create_task(request.wait_for_disconnection())
   bgtask = asyncio.create_task(my_bg_code())
   asyncio.wait([wait, bgtask], return_when=asyncio.FIRST_COMPLETED)
   if bgtask.done():
       wait.cancel()
       return web.Response(bgtask.result())
   bgtask.cancel()
   raise web.HTTPGatewayTimeout()

It's not pretty, but at least it's possible.

@gjcarneiro
Copy link
Contributor

There's probably a few awaits missing in the above, but I can always make a utility function in my app, that simplifies it:

async def cancel_on_disconnect(request, coro):
    ...

def handler(request):
    result = await cancel_on_disconnect(request, my_bg_task())

The cancel_on_disconnect() utility function doesn't need to be maintained by aiohttp. But, ideally, a request.wait_for_disconnection() method should be provided by aiohttp. Otherwise, I can emulate by calling request.transport.is_closing() periodically. 🤷‍♂️

@asvetlov
Copy link
Member Author

Ok, we need request.wait_for_disconnection() async function and probably request.disconnected boolean property.
asyncio prefers functions instead of properties (transp.is_closing() vs transp.closing); aiohttp has another policy.

Regarding request.transport. It is supported now, as well and request.protocol and request.task.
protocol is completely useless at the user level.
transport is just dangerous; the only safe call is request.transport.get_extra_info() which can be replaced with just request.get_extra_info().
task can be used for waiting and cancellation. Waiting can be replaced with wait_for_disconnection, for cancellation we can implement request.disconnect().

What do you think?

@gjcarneiro
Copy link
Contributor

Yes, I prefer request.wait_for_disconnection() async function as well.

I am not sure about request.disconnect(); not seeing the use case for that? What does it do? Disconnect the client http socket? Why would I want to do that? Or are you thinking of aiohttp client-side API?...

@asvetlov
Copy link
Member Author

Suppose you have a streaming or web-socket response.
Suppose you want to drop all such connections on server graceful shutdown.
To do this you need to close all active communications.
Better to have a generic API for this; sure that the universal approach is not as flexible as sending a user-defined message about web-socket communication finish but still it can be useful.

We can left request.disconnect() for the future though.

@asvetlov asvetlov reopened this Oct 16, 2019
@asvetlov
Copy link
Member Author

We are discussing long enough to keep the issue open

@gjcarneiro
Copy link
Contributor

WebSocketResponse already has a close() method.

For other types of responses you can simply send a response with empty or error message body, and then aiohttp can close the transport itself if it's in shutdown mode.

In any case, this does feel like a separate issue. I don't have strong opinion either way, but, if you want Request.disconnect() I suggest instead of generalise WebSocketResponse.close(), moving it to the base class so that it is available to all response types, for greater consistency.

@gjcarneiro
Copy link
Contributor

I'm being stupid, WebSocketResponse is a response, you said request.disconnect(). Anyway, an entirely different issue.

Would you like me to prepare a PR for request.wait_for_disconnection()?

@asvetlov
Copy link
Member Author

The idea for await request.disconnect() is stopping a connection outside of a task that is spawn on request handling.
Anyway, let's postpone it for a while.

Would you like me to prepare a PR for request.wait_for_disconnection()?

Please do

gjcarneiro pushed a commit to gjcarneiro/aiohttp that referenced this issue Oct 19, 2019
as means of allowing request handlers to be notified of premature client
disconnections.
@7ckingBest
Copy link

7ckingBest commented Feb 29, 2020

The proposal is:

  • web-handler cancellation should be opt-in, not opt-out (by aiojobs and async_armor usage).
  • Application should have allow_cancel constructor parameter, False by default.
  • Any record in application route table should accept allow_cancel to override the default.

I agree with the second paragraph. About first - there is a asyncio way to wrap handler into asyncio.shield:

from functools import wraps
def atomic(coro):
	@wraps(coro)
	async def wrapper(*args, **kwargs):
		return await asyncio.shield(coro(*args, **kwargs))
	return wrapper

@atomic
async def handler(request):
    . . .

@lilydjwg
Copy link

I've caught by this unexpected cancellation yesterday. I don't actually care about whether it is the default, but can you print out such cancellation at a higher logging level than debug (+ debug=True) ?

My coroutine just disappeared, without a trace. Even without an access log entry.

FYI, my use case is different than most common ones: the request is not requesting data or modifying state; it's a signal that some event has happened. It's the endpoint of a GitHub webhook, so it's expected to continue running even the requestor has gone. (I've solved this by spawning a free future after discovering aiohttp cancelled my coroutine.)

@asvetlov
Copy link
Member Author

Done by aiohttp 3.7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants