Signals Startup & Cleanup still not good

[cecton]

Long story short

In my application:

• In attribute
  • Client sessions to connect to APIs
• On startup:
  • Background tasks
  • Foreground asynchronous code
• On cleanup:
  • Cancel background tasks
  • Close sessions

My goal is to properly clean up and exit the application if one of the startup hook did not succeeded. But I just can't figure out a way at the moment to properly cleanup my application on exit.

Expected behaviour

1. Failure during a startup hook should prevent the application to startup (cannot reproduce)
2. Cleanup should be sent even if the startup signal failed
3. We should have an helper for background task because it's way too complicated to handle properly
4. Any error during a startup hook should be displayed properly

Actual behaviour

1. The cleanup signal is sent only if all the startup signals succeeded. I believe this is problematic at least in my case because I need to close the client sessions in a kind of "finally" statement.

The following workaround will work for failures during startup signal, but this call to app.cleanup will be duplicated with the one in web.run_app if the application did not fail during startup but is cancelled by a manual KeyboardInterrupt:

loop = asyncio.get_event_loop()
try:
    web.run_app(app, loop=loop)
except (SystemExit, KeyboardInterrupt):
    loop.run_until_complete(app.cleanup())
finally:
    loop.close()

2. Now I remember: any code executed in a startup hook that raises an exception does not see its exception being retrieved (Task exception was never retrieved).

def safe_startup(func):
    async def wrapper(app):
        try:
            await func(app)
        except:
            logger.exception("Startup failed")
            raise
    return wrapper

3. An exception during a cleanup hook prevents the rest of the cleanup hooks to be executed. But that's probably fine.

async def stop_my_bg_task(app):
    if 'my_bg_task' in app:
        app['my_bg_task'].cancel()
        try:
            await app['my_by_task']
        except:  # NOTE: awaiting on a failed bg task retrieves also the exception
            pass  # NOTE: hide that exception

Steps to reproduce

(Examples given above)

Your environment

ArchLinux, Python 3.6, aiohttp 2.2.3

[asvetlov]

We don't support atomic execution for startup/cleanup pair for pairs like start_my_bg_task/stop_my_bg_task.
But we could run cleanup stage unconditionally and use trick like:

async def cleanup(app):
    bg_task = app.get('bg_task')
    if not bg_task:
        return
    try:
        bg_task.cancel()
        await bg_task
    except:
        pass

[cecton]

Yes actually that's what I had in mind. In my PR I make cleanup being called unconditionally and I make a simple mechanism strongly tested for bg processes. Basically it does the await properly for you (when it can).

[socketpair]

We should have an helper for background task because it's way too complicated to handle properly

https://aiojobs.readthedocs.io ?

[cecton]

@socketpair thx! I know already, the rest of the discussion is on #2096

[asvetlov]

@cecton I think we need to invite completely new solution.
Checking for existence of key in app is backward incompatible change.
Why not add brand new approach with keeping existing startup/cleanup but recommending in docs switching to new way?

My wild proposal is:

• Introduce Application.on_safe_startup property for registering async context managers. The name is a subject for discussion.
• The signal only works with objects with __aenter__/__aexit__ defined (https://docs.python.org/dev/library/contextlib.html#contextlib.asynccontextmanager in Python 3.7 could make user code much easier)
• On startup phase process all successive __aenter__ calls and do call __aexit__ on them if something goes wrong.

Unfortunately it doesn't work very well until Python 3.7 release but the implementation already exists in http://pythonhosted.org/asyncio_extras/#module-asyncio_extras.contextmanager

[cecton]

@asvetlov actually that is awesome! I would love using context in place of the signals. This will naturally cleanup only the hooks that have actually started. I will try to implement something in that direction unless you want to do it yourself.

[asvetlov]

Please do.
I've overwhelmed by current issues.

[asvetlov]

Fixed by #2747

[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