WIP Server class to start applications #2465

cecton · 2017-11-05T09:16:48Z

What do these changes do?

Attempt to answer to the issue raised on #2121 and #2375 by creating a Server class capable of registering multiple applications and providing a start() and a stop() asynchronous methods to allow the developers to open and close the server sockets to the application.

Are there changes in behavior for the user?

Added a Server class. web.run_app is preserved.

Related issue number

#2121 #2375

Checklist

I think the code is well written
Unit tests for the changes exist
Documentation reflects the changes
If you provide code modification, please add yourself to CONTRIBUTORS.txt
- The format is <Name> <Surname>.
- Please keep alphabetical order, the file is sorted by names.
Add a new news fragment into the CHANGES folder
- name it <issue_id>.<type> for example (588.bug)
- if you don't have an issue_id change it to the pr id after creating the pr
- ensure type is one of the following:
  - .feature: Signifying a new feature.
  - .bugfix: Signifying a bug fix.
  - .doc: Signifying a documentation improvement.
  - .removal: Signifying a deprecation or removal of public API.
  - .misc: A ticket has been closed, but it is not of interest to users.
- Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

cecton · 2017-11-05T09:38:54Z

@asvetlov is it more or less the idea you had in mind?

kxepal · 2017-11-05T09:56:21Z

aiohttp/web.py

@@ -27,7 +27,7 @@
 from .web_protocol import *  # noqa
 from .web_request import *  # noqa
 from .web_response import *  # noqa
-from .web_server import Server
+from .web_server import Server as WebServer


Maybe rename web_server.Server into web_server.WebServer to avoid collisions?

This will break retro compatibility but we can maybe

Actually I just saw this in a test traceback:

> srv = web.Server(handler) E TypeError: __init__() takes 1 positional argument but 2 were given

Which means the name Server is exported in aiohttp.web so I need to find another name than Server to avoid breaking compatibility.

Yea, better to avoid this name collision somehow.

kxepal · 2017-11-05T09:56:49Z

aiohttp/web.py

+        self.backlog = backlog
+        self.access_log_format = access_log_format
+        self.access_log = access_log
+        self.base_url = URL('{}://localhost'.format(scheme)).with_port(port)


URL.build was pretty nice.

Oops sorry I missed this one 😀 I based my branch on an old version by mistake then I rebased too late when I realized my mistake.

kxepal

Nice idea! Would be nice to see few examples about it usage.

kxepal · 2017-11-05T09:58:08Z

aiohttp/web.py

+        return asyncio.gather(*server_creations, loop=self.loop)
+
+    @asyncio.coroutine
+    def start(self):


How could I start specific app in runtime after starting all the apps?

I'm not sure what you mean. This is not meant to start selectively the applications. This is meant to group all the target applications and run them together on one connection. I mostly inspired on hapi's Glue for the concept.

I mean if Server object manages multiple applications, then why not you can't use it to register/start/stop arbitrary application is runtime? Quite oblivious task for it.

I haven't make it with the idea of changing the applications during runtime at all but it sounds like an interesting use case.

kxepal · 2017-11-05T09:58:51Z

aiohttp/web.py

+        return self.uris
+
+    @asyncio.coroutine
+    def stop(self):


How could I stop specific app in runtime?

kxepal · 2017-11-05T09:59:47Z

aiohttp/web.py

+        self._apps = []
+        self._servers = None
+
+    def register(self, app, *, prefix="/"):


How could I unregister specific app in runtime?

You can't at the moment. It's not meant to be mutable. I can try to implement it but this is not my priority. The code to create a server and register apps is pretty small and I suppose it's best to just re-instantiate a new one if you need to change something.

Re-instantiate a new server means to stop all the applications and cause a downtime. Not a good behaviour for adding a new app.

(Same comment: it wasn't meant to avoid downtime, it was meant to allow running the apps without having to make aiohttp the center of the process)

cecton · 2017-11-05T14:17:58Z

I will give thorough examples and documentation if this is what we want for aiohttp to handle multiple applications (and running the applications without being the main process). Right now all I can say is that you could potentially do this:

Imagine you want to run side by side a grpc server and an application. Now you can by creating dynamically a Server object and registering your application. You just start it and it will work while your main program can do something else. You can stop anytime if you don't want to expose it all the time.

It also helps to gather Application objects and run them together. ~~But then maybe it would be a good idea to allow web.run_app to take multiple positional arguments for apps and register all the apps to the server.~~ [edit: done]

I suppose there are a lot of use cases for this but to be honest I have never encountered one yet professionally since I mostly did microservices.

Maybe @alexmnazarenko could give his input on this feature. It's just a sample at this point, we can add wait() or other things quite easily I suppose.

asvetlov · 2017-11-05T14:36:02Z

Looks as good concept.
I have a couple ideas for discussion.
We have too many server classes :)
We need another name.
What is the best name for Application instance serving specific TCP or UNIX port? Site maybe? I'm thinking about documentation.

Server from ./aiottp.web_server is a protocol factory for making a HTTP protocol for incoming connections. It could be used alone for making proxies etc.

Application is the most used kind of Server. It has a state, route table, nested apps etc.
The application creates a Server internally with handler hook to process routes etc.

Running Application could serve several sites on different ports or unix paths.

P.S.
From my taste web._make_server_creators function is a mess: it tries to solve too many use cases.
What's about making a Site base class with TCPSite, UNIXSite and SocketSite derivatives for processing host/port, path and sock parameters of run_app correspondingly? These classes shares almost everything except socket creation procedure.

cecton · 2017-11-05T15:04:07Z

Looks as good concept.

It tried to read in your mind 😀

We have too many server classes :)

👍

We need another name.
What's about making a Site base class with TCPSite, UNIXSite and SocketSite derivatives for processing host/port, path and sock parameters of run_app correspondingly? These classes shares almost everything except socket creation procedure.

That sounds like a great idea!

Running Application could serve several sites on different ports or unix paths.

Wait I'm lost. So you would rename Application to Site and this new Server to Application? But then it's TCPApplication, UnixApplication and SocketApplication isn't it? Also you can do a major release if you rename everything like that.

asvetlov · 2017-11-05T15:19:52Z

Wait I'm lost. So you would rename Application to Site and this new Server to Application? But then it's TCPApplication, UnixApplication and SocketApplication isn't it? Also you can do a major release if you rename everything like that.

No. Application should not be touched.
Site use Application for serving the app on specific host/port.
Another site could do the same for other port (think about HTTP/HTTPS).

asvetlov · 2017-11-22T11:54:07Z

Superseded by #2530

asvetlov · 2017-11-22T12:19:47Z

@cecton I believe #2530 solves the problem better.
Big thank you for working on prototypes -- it allowed me to look on the problem from different points and find the final solution.

cecton · 2017-11-22T19:43:14Z

You're welcome :) I'm glad it helped!

lock · 2019-10-28T12:04:57Z

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

cecton self-assigned this Nov 5, 2017

cecton requested a review from asvetlov November 5, 2017 09:16

WIP Server class to start applications

05148c3

cecton force-pushed the server-of-applications branch from 641b49f to 05148c3 Compare November 5, 2017 09:33

Added missing _set_loop

1cae271

kxepal reviewed Nov 5, 2017

View reviewed changes

Readded lost change to build URL

816a38a

Allow running multiple apps with web.run_app

0aafa02

asvetlov mentioned this pull request Nov 22, 2017

Site and AppRunner #2530

Merged

asvetlov closed this Nov 22, 2017

lock bot added the outdated label Oct 28, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP Server class to start applications #2465

WIP Server class to start applications #2465

cecton commented Nov 5, 2017

cecton commented Nov 5, 2017

kxepal Nov 5, 2017

cecton Nov 5, 2017

cecton Nov 5, 2017

kxepal Nov 5, 2017

kxepal Nov 5, 2017

cecton Nov 5, 2017

kxepal left a comment

kxepal Nov 5, 2017

cecton Nov 5, 2017

kxepal Nov 5, 2017

cecton Nov 5, 2017

kxepal Nov 5, 2017

kxepal Nov 5, 2017

cecton Nov 5, 2017

kxepal Nov 5, 2017

cecton Nov 5, 2017

cecton commented Nov 5, 2017 •

edited

Loading

asvetlov commented Nov 5, 2017

cecton commented Nov 5, 2017

asvetlov commented Nov 5, 2017

asvetlov commented Nov 22, 2017

asvetlov commented Nov 22, 2017

cecton commented Nov 22, 2017

lock bot commented Oct 28, 2019

WIP Server class to start applications #2465

WIP Server class to start applications #2465

Conversation

cecton commented Nov 5, 2017

What do these changes do?

Are there changes in behavior for the user?

Related issue number

Checklist

cecton commented Nov 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kxepal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cecton commented Nov 5, 2017 • edited Loading

asvetlov commented Nov 5, 2017

cecton commented Nov 5, 2017

asvetlov commented Nov 5, 2017

asvetlov commented Nov 22, 2017

asvetlov commented Nov 22, 2017

cecton commented Nov 22, 2017

lock bot commented Oct 28, 2019

cecton commented Nov 5, 2017 •

edited

Loading