web.run_app: cleanup handlers not called if an error occurs before run_forever

[cecton]

Long story short

The current implementation of web.run_app does not wrap the whole server creation in a try...except but only the run_forever part. This means that if an error occurs (ex: port already taken) between those two lines, the cleanup handlers will not be called despite the startup ones have been called.

Typically when you create a task in a startup handler, it won't be canceled and awaited by the cleanup handler because the cleanup handler is never called.

Expected behaviour

If an error occurs during the server creation, the cleanup handlers still need to be called.

Actual behaviour

If the process ends, it shows the warning:

Task was destroyed but it is pending!

Steps to reproduce

Run two instances of the following script:

import asyncio
from aiohttp import web


queue = asyncio.Queue()


async def background_coroutine(app):
    try:
        await queue.get()
    except asyncio.CancelledError:
        pass


async def start_background_coroutine(app):
    app['background_coroutine'] = app.loop.create_task(background_coroutine(app))


async def stop_background_coroutine(app):
    app['background_coroutine'].cancel()
    await app['background_coroutine']


app = web.Application()
app.on_startup.append(start_background_coroutine)
app.on_cleanup.append(stop_background_coroutine)

web.run_app(app, port=8080)

However... I could reproduce this issue only by using asyncio.Queue. For some reason, a simple asyncio.sleep or an asynchronous HTTP request does not show the error "Task was destroyed but it is pending!". It doesn't even show anything at all actually...

Your environment

Python 3.6, Arch Linux

[cecton]

I did some experimentation here: https://github.com/cecton/aiohttp/compare/sigterm-handling...cecton:runapp-better-context?expand=1

I made contexts, re-indented the code... nothing big despite the diff. I also tried to preserve the order in the original finally.

The test in that commit will fail if executed with the current code in master:

def test_startup_cleanup_signals_even_on_failure(loop, mocker):
    skip_if_no_dict(loop)

    setattr(loop, 'create_server', raise_expected_exception)
    loop.call_later(0.05, loop.stop)

    app = web.Application()
    mocker.spy(app, 'startup')
    mocker.spy(app, 'cleanup')

    with pytest.raises(ExpectedException):
        web.run_app(app, loop=loop)

    app.startup.assert_called_once_with()
    app.cleanup.assert_called_once_with()

https://github.com/cecton/aiohttp/blob/runapp-better-context/tests/test_run_app.py#L575

[asvetlov]

Would you create a PR?

[asvetlov]

Fixed by #1982

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