Skip to content
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.run_app: cleanup handlers not called if an error occurs before run_forever #1959

Closed
cecton opened this issue Jun 6, 2017 · 4 comments
Closed
Labels

Comments

@cecton
Copy link
Contributor

@cecton cecton commented Jun 6, 2017

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
Copy link
Contributor Author

@cecton cecton commented Jun 10, 2017

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
Copy link
Member

@asvetlov asvetlov commented Jun 10, 2017

Would you create a PR?

@asvetlov
Copy link
Member

@asvetlov asvetlov commented Jun 16, 2017

Fixed by #1982

@asvetlov asvetlov closed this Jun 16, 2017
@lock
Copy link

@lock lock bot commented Oct 28, 2019

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.

@lock lock bot added the outdated label Oct 28, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.