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

Web-handler cancellation #2098

Closed
asvetlov opened this Issue Jul 15, 2017 · 11 comments

Comments

Projects
None yet
7 participants
@asvetlov
Member

asvetlov commented Jul 15, 2017

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

This comment has been minimized.

Show comment
Hide comment
@argaen

argaen Jul 15, 2017

Member

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?

Member

argaen commented Jul 15, 2017

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

This comment has been minimized.

Show comment
Hide comment
@hellysmile

hellysmile Jul 15, 2017

Member

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

Member

hellysmile commented Jul 15, 2017

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

@asvetlov

This comment has been minimized.

Show comment
Hide comment
@asvetlov

asvetlov Jul 15, 2017

Member

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.

Member

asvetlov commented Jul 15, 2017

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

This comment has been minimized.

Show comment
Hide comment
@jettify

jettify Jul 15, 2017

Member

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
Member

jettify commented Jul 15, 2017

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

This comment has been minimized.

Show comment
Hide comment
@argaen

argaen Jul 15, 2017

Member

'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
Member

argaen commented Jul 15, 2017

'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

This comment has been minimized.

Show comment
Hide comment
@hellysmile

hellysmile Jul 15, 2017

Member

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

https://github.com/wikibusiness/async_armor

Member

hellysmile commented Jul 15, 2017

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

https://github.com/wikibusiness/async_armor

@fafhrd91

This comment has been minimized.

Show comment
Hide comment
@fafhrd91

fafhrd91 Jul 17, 2017

Member

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

Member

fafhrd91 commented Jul 17, 2017

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

@roganov

This comment has been minimized.

Show comment
Hide comment
@roganov

roganov Jul 20, 2017

Contributor

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

Contributor

roganov commented Jul 20, 2017

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

This comment has been minimized.

Show comment
Hide comment
@fafhrd91

fafhrd91 Jul 20, 2017

Member
Member

fafhrd91 commented Jul 20, 2017

@larry-dev

This comment has been minimized.

Show comment
Hide comment
@larry-dev

larry-dev Sep 4, 2017

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

larry-dev commented Sep 4, 2017

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

@asvetlov

This comment has been minimized.

Show comment
Hide comment
@asvetlov

asvetlov Sep 11, 2017

Member

Fixed by #2257

Member

asvetlov commented Sep 11, 2017

Fixed by #2257

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment