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

Split async server running functions #2121

Closed
txomon opened this issue Jul 21, 2017 · 29 comments · Fixed by #2530
Closed

Split async server running functions #2121

txomon opened this issue Jul 21, 2017 · 29 comments · Fixed by #2530
Labels
Milestone

Comments

@txomon
Copy link

txomon commented Jul 21, 2017

Long story short

The requirement to have run_app as only execution of the server is not enough for some applications. It would be nice if this run_app functionality could be divided in two functions, start() and cleanup(), as many other libraries do. After all aiohttp is not the core of my app.

The idea is I want to be in charge of the main loop and control how and when it runs.

Expected behaviour

Not to modify existing applications when including aiohttp.

Actual behaviour

aiohttp get's to be the centre of the application by running run_forever() inside the only provided function to run the web server part.

Steps to reproduce

  1. Write asyncio application
  2. Try to include web interface using aiohttp
  3. Modify application to be run with aiohttp as the centre of it, synchronously.
@txomon txomon changed the title Create async server running function Split async server running functions Jul 21, 2017
@arthurdarcet
Copy link
Contributor

aiohttp does not need to be the center of your app. Have a look at what the run_app helper is doing: https://github.com/aio-libs/aiohttp/blob/master/aiohttp/web.py#L412

You can copy some of that logic in your code, and trigger your loop.run_forever whenever you want. But of course, you should also handle some "process-related" issues on your own, such as handling the SIGTERM/INT signals to allow aiohttp to shutdown gracefully.

@asvetlov
Copy link
Member

Anyway keeping separate startup and shutdown functions in public api makes sense.
Honestly I cannot remember all needed steps without looking into source code

@alexmnazarenko
Copy link

alexmnazarenko commented Jul 26, 2017

In my project, I've implemented AsyncServer class for the very same reasons.

Something like this:

class AsyncServer:
    def __init__(...run_app params..)
    async def start()
    async def shutdown()
    async def wait()
    async def __aenter__()
    async def __aexit__()

Works fine )

Something like this will be nice to have right in the library.

@gjcarneiro
Copy link
Contributor

I have to add my +1 to fixing this. IMHO, the best thing would be to have a run() coroutine method on aiohttp.web.Application(), so that my main app looks like (simplified):

async def my_suff():
   ...

def main():
    loop = asyncio.get_event_loop()
    app = web.Application()
    ....
    loop.run_until_complete(
        asyncio.gather(
            app.run("0.0.0.0", 80),
            run_my_stuff(),
        )
    )

Now wouldn't that be lovely? ;-)

@asvetlov
Copy link
Member

No, it should be a separate class.
Think about running the same app on different hosts/ports and stopping the app.

@gjcarneiro
Copy link
Contributor

In that case, I think something like @alexmnazarenko's AsyncServer would be great to have. @alexmnazarenko, is your code available somewhere?

@alexmnazarenko
Copy link

@gjcarneiro Yes, it is: https://github.com/datadvance/pRpc/blob/master/prpc/platform/ws_aiohttp.py

But it's not feature complete for inclusion in aiohttp itself. I've dropped support for UDP sockets as I don't need it for know. Still, it works well for me allowing to run multiple servers on different ports in a single asyncio app.

@cecton
Copy link
Contributor

cecton commented Oct 24, 2017

After all aiohttp is not the core of my app.

I don't get that part. During the lifetime of the main process you want your web service to be started and stopped? Can you explain a use case for that?

@txomon @alexmnazarenko can you explain the use case? I just checked the pRpc library which does provide the lib but doesn't really use it except for tests.

@txomon
Copy link
Author

txomon commented Oct 24, 2017

In my case, this was part of a bot, which had many other things running at the same time. If i wanted to expose an API, but it wasn't the main objective of the server, I would still need to set this as the main process.

The idea is that aiohttp shouldn't be expected to be the centre of the app.

@cecton
Copy link
Contributor

cecton commented Oct 24, 2017

Does the API you're exposing get not exposed at some point? Or is the API always running until the end of the process?

but it wasn't the main objective of the server

In the current state of the project, is it now possible to start your bot with and without API? Or did it evolve from a loop to a web service?

The idea is that aiohttp shouldn't be expected to be the centre of the app.

That I get 😁 but it seems to me that if aiohttp is not the center of the application, then the application exposes an API for some time and then remove it. This particular use case doesn't make much sense to me.

@txomon
Copy link
Author

txomon commented Oct 24, 2017

This specific bot has several configuration options, working in some cases using websockets, other times push http notifications and others both. Coupling the main process to the webserver process would make it need to have the webserver up regardless on the configuration.

Also, it has hot plugin (re)loading, and the webserver may be terminated and restarted to accommodate new endpoints.

@cecton
Copy link
Contributor

cecton commented Oct 24, 2017

Thx! Makes a lot more sense now. What do you need exactly in the web.run_app for your application? I suppose you will handle the signals yourself.

web.run_app provides:

  • SIGTERM and SIGINT handling
  • servers creations and closure
  • startup, clean and shutdown hooks

@txomon
Copy link
Author

txomon commented Oct 24, 2017

The best for me would to actually have those features separated in different functions, that I can decide to use or not.

In my case, I don't make any difference between server creation/closure and the startup, clean and shutdown hooks.

I'm not familiar on the internals, but in my case, having functions that can be used like the following:

  1. Run/await this function to init the lib
  2. Await this function to start a server
  3. Await this function to stop the server
  4. Run/await this function to clean up the lib

Where steps 2 and 3 can be looped (to restart the service). Maybe you don't need to have steps 1/4, but just following on the design pattern of other libs, such as libcurl

@alexmnazarenko
Copy link

@cecton As of my usecases:

  • Multiple apps on different ports + some async init/teardown
  • Weird tests

Example of multiple apps usecase:
https://github.com/datadvance/pAgent/blob/master/pagent/__main__.py

Here I have an program that uses HTTP for everything - for RPC, for administration and more. RPC and admin apps must have different endpoints (admin is usually localhost only).

@cecton
Copy link
Contributor

cecton commented Oct 24, 2017

Cool! Thanks a lot for all your feedbacks.

I believe signal handling is out of the scope here so we should probably have a start() and shutdown() (or stop()?) functions that will trigger the hooks and create/remove the servers cleanly like @alexmnazarenko did on https://github.com/datadvance/pRpc/blob/master/prpc/platform/ws_aiohttp.py

I'm not too much into introducing a new object. @asvetlov do you think we can add the start and stop methods on the class Application directly?

Then I will reduce web.run_app to only handle the signals and start the appropriate methods.

Do you think that will cover it all? I skipped the async context and the wait method because I don't think they are any useful but let me know if I'm totally wrong.

@asvetlov cecton@3b8a782 here a sample

@alexmnazarenko
Copy link

That should help, 'wait' and other convenience stuff can be easily implemented externally. The main point for me is to avoid copying the whole run_app signal stuff (as it can change between versions).

@cecton
Copy link
Contributor

cecton commented Oct 25, 2017

Ok perfect then? I will make some tests and see if it works as expected.

What is the use case of a "wait" by the way? Because you should be able to achieve the same thing by using the cleanup hook. What also would be the use case for an async context like you implemented? In tests maybe?

When you say "signals" you mean the "signals"/hooks of the application right? Not the Unix signals?

@alexmnazarenko
Copy link

alexmnazarenko commented Oct 25, 2017

Yeap, "ok perfect" 😄

  1. Wait is not critical. It does replace cleanup hook for me, just with less local variable hassle. But there are multiple ways to handle that, so I think "adding less API to the framework" is a strong enough concern to drop it.
  2. Yes, I mean hooks. The internal hook implementation classes are actually called '*Signal', that's why I call them so )

PS in your code sample "await self.cleanup()" is called twice?

@cecton
Copy link
Contributor

cecton commented Oct 25, 2017

Yes it was just a sample 😫 I did it really fast to show to Andrew what I had in mind.

@asvetlov
Copy link
Member

asvetlov commented Oct 25, 2017

I not sure should we add new methods and state into Application or create a new class for running server.
For nested app these methods are useless and should not be called.

@cecton
Copy link
Contributor

cecton commented Oct 25, 2017

@asvetlov I haven't thought about that and that's a good point. From my point of view it seems that the logic of Application is well scoped in the class itself. It was weird to have it spread in a different method.

@alexmnazarenko
Copy link

New class is fine by me. As I wrote, I just want to remove hook/socket handling from my code as it's an 'unstable' copypaste from the framework.

Also, I forgot - my implementation had a nice bonus of returning 'resolved' bound endpoints. That's very helpful when you use port '0'.

@asvetlov
Copy link
Member

Looks like we need a Pull Request.
Any hero?

@cecton
Copy link
Contributor

cecton commented Oct 25, 2017

@asvetlov I will but not now 😀 after office hours

@cecton
Copy link
Contributor

cecton commented Oct 25, 2017

@asvetlov so I make a different class or I put it in application?

@asvetlov
Copy link
Member

I suggest a different class.
Application describes a web site and used on initialization stage, a new class runs the app on single host/port.
Hmm, as a consequence the runner should not call on_startup() and on_cleanup() signals, only on_shutdown.
Startup/cleanup should be called only once. Maybe we need separate methods for it.

@cecton
Copy link
Contributor

cecton commented Oct 25, 2017

@asvetlov maybe I can make a class that takes multiple Application classes and do the whole job? Everything will be called once.

@asvetlov
Copy link
Member

Maybe it's good idea, let's look on API.

@lock
Copy link

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].
[new issue]: https://github.com/aio-libs/aiohttp/issues/new

@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.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants