Support application coroutine factory in web.run_app and gunicorn worker.

[asvetlov]

Summary

It should be something like:

async def make_app():
    app = web.Application()
    await setup_app(app)
    return app

web.run_app(make_app) for run_app and gunicorn script:make_app for Gunicorn.

Motivation

We encourage stop passing a loop instance to application.
But the app still needs a loop for it's functionality.
For this we have app._init_loop(loop) method which is called imperceptible by run_app() and Gunicorn worker. It at least very tricky. Other libraries like aiohttp-devtool should also call _init_loop internally and so on.

But creation application object inside a coroutine removes the need for extra loop initializer because the loop is already here implicitly.

To do it we need an asynchronous callback to create an application.

Implementation details

Both parameter for web.run_app() and Gunicorn entry point should be either web.Application entry or a async function which accepts no parameters.
In the last case aiohttp calls this function, gets coroutine back and runs the coroutine. Result is and web.Application instance.

Future improvements

Later we could support async context manager as factory:

@contextlib.asynccontextmanager
async def make_app():
    app = web.Application()
    await init()
    try:
        yield app
    finally:
        await cleanup()

While cleanup task could be solved by adding app.on_cleanup signal handler maybe somebody prefer more explicit approach.

[fafhrd91]

to me this looks like "coroutines" just for sake of "coroutines", extra complexity without clear reasoning.
putting application creation into coroutine would mix two different configuration steps into one.
actual application configuration, loading configs, registering routes, etc and preparing network connections (like db, redis, etc)
first step has nothing to do with asyncio.

code app._set_loop(loop) in run_app is a bug
applicaton has explicit method
https://github.com/aio-libs/aiohttp/blob/master/aiohttp/web.py#L238

[asvetlov]

The problem is initialization order.
I pretty sure app.startup() should be called before first app.make_handler() call.
Otherwise client may send HTTP request to not yet initialized application.

I understand your position. Maybe we could invite another approach?
Say, call app._set_loop(asyncio.get_event_loop()) in very first line of app.startup() if Python >= 3.5.3?
3.5.3 has a patch for always returning running loop.

[fafhrd91]

I am not sure how uninitialized application can receive request before make_handler returns? Also who cares about make_handler in general, we have signal.

[asvetlov]

No, I'm talking about another scenario:

1. app.make_handler()
2. Server is ready to receive HTTP requests
3. Request is come, web-handler called
4. app.startup() called

On stage 3) we have only partially initialized application

[fafhrd91]

Well, this is definitely bug in flow. There should be no configuration happen beyond make_handler call

[asvetlov]

That's why I swapped around make_handler() and startup() calls inside run_app().
But it also required adding _init_loop() call before startup().

[fafhrd91]

why run_app() calls startup? breaking encapsulation is really bad. this also means that workers are actually broken.

[asvetlov]

It did it starting from very initial version.
Sorry, I don't understand why workers are broken and what is encapsulation breakage.

[fafhrd91]

worker does not call startup.

app._set_loop() is private method, why run_app calls it?

[asvetlov]

worker does not call startup.

Actually it does: https://github.com/aio-libs/aiohttp/blob/master/aiohttp/worker.py#L44-L45
And you are author of these lines :)

app._set_loop() is private method, why run_app calls it?

Because it was the only way to setup application's loop before app.startup() call.

And it is the partial reason for raising this issue.

[fafhrd91]

worker is still broken, loop is not set when startup get called.

just move startup call to make_handler, and we won't need any coroutine crap.

[asvetlov]

Aah, I see.

app.startup() is a coroutine, we cannot call it from regular method app.make_handler()

[fafhrd91]

it doesn't matter, we use loop.run_until_complete() in both worker and run_app cases

we need one method that would finalize app configuration and prepare handler. everything before call to this method should be sync, everything after should be async.

[asvetlov]

Not so easy :(
app.make_handler() could be called several times in the middle of program execution to dynamically start serving new ports or Unix sockets. Most likely these calls a made from coroutine. Making run_until_complete() call from coroutine is forbidden.

It could be done by brand new method though which asserts that current loop is None or not running.

[fafhrd91]

how is this possible? only worker can call it or run_app method. if someone wants to shoot itself you can't protect them

[fafhrd91]

2.0 release is several months old, no body complained about startup method, so high likely nobody needs loop in startup and probably there is no application init problem

[samuelcolvin]

I've definitely had problems with missing loop in startup, can't remember how I worked around.

…

On 29 Jun 2017 6:32 pm, "Nikolay Kim" ***@***.***> wrote: 2.0 release is several months old, no body complained about startup method, so high likely nobody needs loop in startup and probably there is no application init problem — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#2033 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AD2jGS-JoadxFNdFxti0coNkJLKF1BAxks5sI9-ZgaJpZM4OH02G> .

[asvetlov]

I personally seen very many code that uses app.on_startup() signals.

Nobody complained because worker setups default event loop here: https://github.com/aio-libs/aiohttp/blob/master/aiohttp/worker.py#L34-L41

The only broken thing is at this stage app.loop is None.
Perhaps people don't use this attribute especially after fixing asyncio.get_event_loop() in Python 3.6 and Python 3.5.3

[asvetlov]

how is this possible? only worker can call it or run_app method. if someone wants to shoot itself you can't protect them

We din't explicitly forbid it but described make_handler() in documentation.
Before run_app() introduction people used to call .make_handler() explicitly.
I don't know is there all usages are replaced.

Also we do call .make_handler() from coroutine in our test suite.

[fafhrd91]

want to complicate configuration, fine. then i am not sure what we are discussing now.

[asvetlov]

Home Assistant calls make_handler() from coroutine: https://github.com/home-assistant/home-assistant/blob/0863d502102c0f66511097f6f237e4f3de7b1f4c/homeassistant/components/http/__init__.py#L329

[asvetlov]

I want to find a fix without breaking changes.
Let's forget about application factories (while I still not convinced that it is a bad idea).
We could call _init_loop() in worker before startup(), invite other policy for loop instantiation or do something else -- I don't know what.

Let me take a pause until tomorrow. I need think over alternatives.

[fafhrd91]

ok, they set custom attributes on loop. do you think we should encourage that too?

[fafhrd91]

I am bored with this issue. want to use coroutines everywhere fine. what introduce new api for each use case fine. just stop talking about clean design.

[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