Change web handlers cancellation policy

[asvetlov]

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]

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):

[gjcarneiro]

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]

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]

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]

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]

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]

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]

What should be Request.poll() behavior?

[gjcarneiro]

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]

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]

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]

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]

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]

We are discussing long enough to keep the issue open

[gjcarneiro]

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]

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]

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

[7ckingBest]

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]

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]

Done by aiohttp 3.7