Web-handler cancellation

[asvetlov]

Now web-handler task is cancelled on client disconnection (normal socket closing by peer or just connection dropping by unpluging a wire etc.)

This is pretty normal behavior but it confuses people who come from world of synchronous WSGI web frameworks like flask or django.

The problem is: if web-handler writes a data to DB the task might be cancelled without waiting for transmission completion. The following snippet might be stopped on every line:

async def handler(request):
    async with request.app['db'] as conn:
         await conn.execute('UPDATE ...')

To make execution atomic user should spawn a task:

async def writer(db):
    async with db as conn:
         await conn.execute('UPDATE ...')

async def handler(request):
    get_event_loop().create_task(writer(request.app['db']))

This approach is not reliable, new task is not awaited.

User need to use aiojobs:

async def handler(request):
    aiojobs.spawn(writer(request.app['db']))

Or maybe not existing yet:

@aiojobs.atomic
async def handler(request):
    async with request.app['db'] as conn:
         await conn.execute('UPDATE ...')

Sure, I could document it in http://aiohttp.readthedocs.io/en/stable/web.html but not sure is it very visible and obvious way to protect from cancellation. Ideas?

[argaen]

First of all. As you say the behavior is normal so IMO with a big disclaimer in aiohttp documentation should be more than enough (for example in http://aiohttp.readthedocs.io/en/stable/web.html). Anyway not all users read the documentation so this issue will still surprise them, as long as we have it documented and reachable, should be fine.

Now if we want to offer an alternative so handlers don't get cancelled sounds nice. If aiojobs is able to do that good to reference it in the documentation but two things:

• First we should mention what is considered normal behavior, what users should expect etc and then, offer the alternative: "In case you don't want the handler to be cancelled, do this and that".

• Also, wouldn't asyncio.shield be enough for that behavior we want to add?

[hellysmile]

'asyncio.shield' is not enough, cuz You'r app can be stopped at any moment and shield wouln't be finished

[asvetlov]

Well, shield might be enough in many cases. But there is very common pattern: send response quickly and do all slow operations in background.
I believe aiojobs solves it as well as other requirements.

[jettify]

asyncio cancel behaviour is very tricky business, and we had bunch of issues in other projects (in all our DB dirvers). I believe we should educate people and have separate entry in docs about cancel stuff as such question is most common one for new to aiohttp devs.

Things I see we can do:

1. separate doc entry about cancel behavior and why we want to have it in aiohttp (long running response use case vs multiple updates).
2. Add examples with aiojobs
3. Update aiohttp demo to use aiojobs

[argaen]

'asyncio.shield' is not enough, cuz You'r app can be stopped at any moment and shield wouln't be finished

You're right there but I believe those are edge cases, I think the problem proposed here is more focused on an api that is not stopped where this problem still exists when the client disconnects. asyncio.shield would be enough there.

Anyway aiojobs would suffer from the same problem if close is not called before the app exits.

Well, shield might be enough in many cases. But there is very common pattern: send response quickly and do all slow operations in background.
I believe aiojobs solves it as well as other requirements.

Yeah, I don't argue with the use case about responding quickly and do slow operations in background. It's a necessary and nice tool but in the case you propose I believe asyncio.shield would be enough and as a developer I usually aim for keeping my dependencies as low as possible. IMO in summary:

• For client disconnections/fighting against Task cancellation I would recommend asyncio.shield. That's why it exists
• In case user wants a way to control tasks in a more granular way, then I would recommend aiojobs
• Ofc if user wants to execute background tasks (inside the same loop) I would also recommend aiojobs

[hellysmile]

Sorry for possible off-topic, but there is how we have solved exactly this issue

https://github.com/wikibusiness/async_armor

[fafhrd91]

I think separate documentation section is enough. remember, we are not babysitters.

[roganov]

Is it feasible to have an option to disable CancelledError's completely and have an ability on re-enable on per-handler basis? I understand why CancelledError may be great for microservices, but IMO for user-facing services it creates only problems by having business logic completed only partially (i.e., failing to send an email after committing a transaction, etc).
Obviously I can wrap all my handlers in aiojobs, but that would create overhead (though I didn't measure if it's substantial).

[fafhrd91]

I do not think it is possible. CancelledError baked into asyncio so deep.

…

Sent from my iPhone

On Jul 20, 2017, at 9:25 AM, Yegor Roganov ***@***.***> wrote: Is it feasible to have an option to disable CancelledError's completely and have an ability on re-enable on per-handler basis? I understand why CancelledError may be great for microservices, but IMO for user-facing services it creates only problems by having business logic completed only partially (i.e., failing to send an email after committing a transaction, etc). Obviously I can wrap all my handlers in aiojobs, but that would create overhead (though I didn't measure if it's substantial). — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[larryclean]

i use aiojobs in python_socket.io,but it doesn't solve the problem.

[asvetlov]

Fixed by #2257

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a [new issue] for related bugs.
If you feel like there's important points made in this discussion, please include those exceprts into that [new issue].
[new issue]: https://github.com/aio-libs/aiohttp/issues/new