classification
Title: support multiple hosts in create_server/start_server
Type: enhancement Stage:
Components: asyncio Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: gvanrossum, python-dev, sebastien.bourdeauducq, vstinner, yselivanov, ysionneau
Priority: normal Keywords: patch

Created on 2015-03-10 18:18 by sebastien.bourdeauducq, last changed 2015-09-22 22:06 by ysionneau. This issue is now closed.

Files
File name Uploaded Description Edit
asyncio_multibind.diff sebastien.bourdeauducq, 2015-03-10 19:20
asyncio_multibind_test.diff ysionneau, 2015-03-11 13:07 New test for asyncio multibind feature
multibind.patch vstinner, 2015-03-11 13:18 review
multibind2.patch ysionneau, 2015-08-26 21:02 new patch, with changes according to review review
multibind3.patch ysionneau, 2015-08-27 16:56 new patch, with changes according to review from Yury review
multibind4.patch ysionneau, 2015-08-28 14:06 new patch: more PEP8 friendly + capitalization of first word of sentences review
multibind6.patch ysionneau, 2015-09-03 15:16 new patch with no dependency on netifaces review
multibind7.patch ysionneau, 2015-09-04 14:19 new patch according to review http://bugs.python.org/review/23630/#ps15472 review
multibind8.patch ysionneau, 2015-09-04 22:49 review
Messages (43)
msg237791 - (view) Author: Sebastien Bourdeauducq (sebastien.bourdeauducq) * Date: 2015-03-10 18:18
I would like to be able to bind asyncio TCP servers to an arbitrary list of hosts, not just either one host or all interfaces.

I propose that the host parameter of create_server and start_server can be a list of strings that describes each host. Sockets are created for the set of all addresses of all specified hosts. The list may also contain None, or the empty string, in which case all interfaces are assumed.

If a string or None is passed directly, the behavior is unchanged - so this should not break compatibility.

I can submit a patch if this feature is approved.
msg237792 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2015-03-10 18:27
This makes some sense, but it's easy to work around -- just call create_server() multiple times, once for each host. Why does that not work for you?
msg237794 - (view) Author: Sebastien Bourdeauducq (sebastien.bourdeauducq) * Date: 2015-03-10 18:45
That could work, but I would have to manually resolve and filter the lists of hostnames so that the program does not attempt to bind the same address twice - something that is better implemented in create_server, which already does the hostname resolution. Additionally, the asyncio Server internally supports listening on multiple sockets, and that would be the right feature to use for implementing this instead of creating and managing multiple servers.
msg237795 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2015-03-10 18:46
So it sounds like you already have a patch in mind... Can you work on it and upload it? (It's open source -- you get to make it yourself. :-)
msg237800 - (view) Author: Sebastien Bourdeauducq (sebastien.bourdeauducq) * Date: 2015-03-10 19:20
See attached.
msg237803 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2015-03-10 19:28
Could use a test. Hopefully Victor can do a thorough review.

On Tue, Mar 10, 2015 at 12:20 PM, Sebastien Bourdeauducq <
report@bugs.python.org> wrote:

>
> Sebastien Bourdeauducq added the comment:
>
> See attached.
>
> ----------
> keywords: +patch
> Added file: http://bugs.python.org/file38428/asyncio_multibind.diff
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue23630>
> _______________________________________
>
msg237862 - (view) Author: Yann Sionneau (ysionneau) * Date: 2015-03-11 13:07
Here is a test related to Sebastien's patch.
msg237863 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-11 13:18
Combine patch written for CPython tree, so we get the "review" button.
msg238641 - (view) Author: Yann Sionneau (ysionneau) * Date: 2015-03-20 10:46
Please let me know if you need anything for this patch to be integrated.
msg241707 - (view) Author: Yann Sionneau (ysionneau) * Date: 2015-04-21 08:38
ping ?
msg246025 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-07-01 06:35
I reviewed multibind.patch.
msg249215 - (view) Author: Yann Sionneau (ysionneau) * Date: 2015-08-26 20:41
Here is a new patch, with modifications according to your review Victor.
Thanks!
msg249216 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-08-26 20:47
> Here is a new patch, with modifications according to your review Victor.

Could you please generate the patch with 'hg diff', so that code review works with it.
msg249218 - (view) Author: Yann Sionneau (ysionneau) * Date: 2015-08-26 21:02
Ah yes, sorry about that.
Here is a new patch.
However, I worked on these sources: https://github.com/python/asyncio which now I understand was not the correct repository, but I added a tests_require="netifaces" in the setup.py, where can I add this dependency in the CPython repository architecture?
Thanks!
msg249220 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-08-26 22:35
Left some feedback in the code review.

I have a question about the api: so we allow to pass a set of hosts to create_server, but why do we have only a single port?
msg249236 - (view) Author: Yann Sionneau (ysionneau) * Date: 2015-08-27 16:43
All right, thanks.
I fixed everything but the thing about getaddrinfo being Mock'ed.
How am I supposed to handle the method being called with a Mock'ed getaddrinfo?
msg249238 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-08-27 16:48
> How am I supposed to handle the method being called with a Mock'ed getaddrinfo?

Why would you call it that way?  Are regular asyncio users going to mock it in their production/test code?

If the only case for using mocks there are asyncio unittests -- just don't do that.
msg249240 - (view) Author: Yann Sionneau (ysionneau) * Date: 2015-08-27 16:56
All right, here is the new patch then.
msg249243 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-08-27 17:24
Thanks!

The only remaining question is is it OK that we can specify more than one host but only one port?  Guido, Victor?
msg249244 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2015-08-27 18:01
Yeah, I think this is fine. I think the use case for multiple hosts is very different than that for multiple ports, so I see no reason to link the two features, and multiple ports hasn't been requested.

The patch has a long line and the new docstring looks a little odd (sentences not starting with capital letters) but otherwise I'm in favor of this going into 3.6.
msg249245 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-08-27 19:00
> Yeah, I think this is fine. I think the use case for multiple hosts is very different than that for multiple ports, so I see no reason to link the two features, and multiple ports hasn't been requested.

OK!

> The patch has a long line and the new docstring looks a little odd (sentences not starting with capital letters) 

Yeah, I've noticed that too, and made a mental note to check line lengths before committing.

> but otherwise I'm in favor of this going into 3.6.

Why only 3.6?  The change is fully backwards compatible, why can't it go in 3.5.1?

Also, what's the protocol for committing something that might take years to be available in github/asyncio?
msg249248 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2015-08-27 19:03
It can't go into 3.5.1 (a bugfix release) because it is a new feature (however small), and asyncio is not provisional in 3.5 (only in 3.4).
msg249249 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2015-08-27 19:04
Also, I guess you'll need a branch that tracks what's going into 3.6.
msg249250 - (view) Author: Yann Sionneau (ysionneau) * Date: 2015-08-27 19:05
About the missing capitalization, I omitted it because it's the argument's name. But I can resubmit with it being capitalized.
I forgot to run flake8, sorry!
msg249252 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2015-08-27 19:07
The traditional solution is to rewrite the sentence so the name is not the first word. :-)
msg249279 - (view) Author: Yann Sionneau (ysionneau) * Date: 2015-08-28 14:06
A new patch with capitalization of first words of sentences + splitting lines which are too long (more PEP8 friendly).
msg249544 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-09-02 14:07
> The only remaining question is is it OK that we can specify more than one host but only one port?  Guido, Victor?

I can be convenient to have a single Server object with *all* listening sockets. So I like the idea of supporting multiple ports. It's very cheap to support it.
msg249545 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-09-02 14:19
Since the consensus on python-dev looks more to keep the provisional API for asyncion in Python 3.5, I propose to even modify Python 3.4 to not add too many "if python >= 3.5..." checks in asyncio source code.

If you really prefer to keep Python 3.4 unchanged, we should at least add the feature in Python 3.5.1, having to wait for Python 3.6 is too long, no?
msg249546 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-09-02 14:27
> I can be convenient to have a single Server object with *all* listening sockets. So I like the idea of supporting multiple ports. It's very cheap to support it.

I like the idea too, but I'm not sure how to redesign the function's signature to accept multiple hosts & ports.  We have a separate param for 'port', and this patch allows to pass an array of hosts in 'host' argument.  Should we allow passing an array of (host, port) tuples in 'host' with 'None' in 'port'?  This would be one ugly method...

> Since the consensus on python-dev looks more to keep the provisional API for asyncion in Python 3.5, I propose to even modify Python 3.4 to not add too many "if python >= 3.5..." checks in asyncio source code.

asyncio is provisional in 3.4 (and now in 3.5 too), so we can just keep updating it in bugfix releases.

> If you really prefer to keep Python 3.4 unchanged, we should at least add the feature in Python 3.5.1, having to wait for Python 3.6 is too long, no?

This was before we reinstated asyncio as provisional in 3.5.

But before committing this change, let's agree if we want this API to accept multiple ports when you specify more than one host.

We also need to create a dev branch on github/asyncio, so that it's easier for us to sync PyPI package with CPython releases.
msg249547 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-09-02 14:28
"So I like the idea of supporting multiple ports. It's very cheap to support it."

Oh, I had an example in my head: listening on tcp/80 for HTTP and tcp/443 for HTTPS... but it doesn't work. The problem is that the proposed API doesn't allow to specify a protocol factory per port. For such use case, you have to handle multiple servers.

Anyway, I still like the idea of listening to multiple hosts and/or multiple ports. It's a common requirement to start a server.
msg249548 - (view) Author: Yann Sionneau (ysionneau) * Date: 2015-09-02 14:31
About the function's signature to accept multiple hosts & ports, we could 

- accept host and port arguments being sequences and then we bind to all host:port combinations. Like if len(host) == N and len(port) == M, we bind to N*M sockets.

or

- accept host and port arguments being sequences and allow repetitions and bind to host:port couples from zip(host, port).

I think the first is "clean", but the second allows more flexibility.
Like for instance binding to IP1:port1 IP2:port2 but not IP1:port2
msg249549 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2015-09-02 14:35
Let's not extend the API with support for multiple ports. Nobody has asked for it, and clearly the API design is not so simple.
msg249551 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-09-02 14:38
> - accept host and port arguments being sequences and then we bind to all host:port combinations. Like if len(host) == N and len(port) == M, we bind to N*M sockets.

I can't think of a real life example for this.  But if you need that, you can always implement that on top of function that accepts multiple (host,port) pairs.

> - accept host and port arguments being sequences and allow repetitions and bind to host:port couples from zip(host, port).

I'm not sure I like this either.  This breaks one value `(h,p)` into two values, and passes them in separate collections to separated arguments.  Too complex.

My proposal is to allow the following:

   create_server(protofac, [(h1,p1),(h2,p2)])
msg249552 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-09-02 14:41
> Let's not extend the API with support for multiple ports. Nobody has asked for it, and clearly the API design is not so simple.

Ok. Anyway, the user of the API call call create_server() multiple times, even with the current code. So it's fine.
msg249556 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-09-02 16:26
Yann, I'll commit this as soon as 3.5.0 and 3.4.4 are out.
msg249658 - (view) Author: Yann Sionneau (ysionneau) * Date: 2015-09-03 15:16
Here is a new patch without any dependency on netifaces.
Thanks Victor for the great deal of help for all the mock() stuff!
msg249764 - (view) Author: Yann Sionneau (ysionneau) * Date: 2015-09-04 14:19
A new (and hopefully last?) version of the patch, thanks again for the review Victor!
msg249772 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-09-04 15:19
Good. I have less comments, it's getting better at each iteration ;)
msg249852 - (view) Author: Yann Sionneau (ysionneau) * Date: 2015-09-04 22:49
Yet another version of the patch according to the last review.
msg251240 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-09-21 16:42
New changeset 682623e3e793 by Victor Stinner in branch '3.4':
Issue #23630, asyncio: host parameter of loop.create_server() can now be a
https://hg.python.org/cpython/rev/682623e3e793
msg251241 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-09-21 16:45
I pushed the change to Python 3.4, 3.5, 3.6 and asyncio at Github.

Thanks Yann for your nice enhancement of asyncio! It put your name in CPython "acks" and asyncio author list.

I modified the patch a little bit (3 lines), how the hosts variable was initialized. That's all :-)
msg251252 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-09-21 20:32
New changeset 42e7334e0e4c by Victor Stinner in branch '3.4':
Issue #23630: Fix test_asyncio on Windows
https://hg.python.org/cpython/rev/42e7334e0e4c

New changeset 840942a3f6aa by Victor Stinner in branch '3.5':
Merge 3.4 (Issue #23630, fix test_asyncio)
https://hg.python.org/cpython/rev/840942a3f6aa

New changeset 1f979a573f8d by Victor Stinner in branch 'default':
Merge 3.5 (Issue #23630, fix test_asyncio)
https://hg.python.org/cpython/rev/1f979a573f8d
msg251352 - (view) Author: Yann Sionneau (ysionneau) * Date: 2015-09-22 22:06
Thanks a lot Victor for your numerous reviews and your help on this!
Also thanks for all other who commented.
History
Date User Action Args
2015-09-22 22:06:25ysionneausetmessages: + msg251352
2015-09-21 20:32:28python-devsetmessages: + msg251252
2015-09-21 16:45:49vstinnersetstatus: open -> closed
resolution: fixed
messages: + msg251241
2015-09-21 16:42:42python-devsetnosy: + python-dev
messages: + msg251240
2015-09-04 22:49:18ysionneausetfiles: + multibind8.patch

messages: + msg249852
2015-09-04 15:19:38vstinnersetmessages: + msg249772
2015-09-04 14:19:20ysionneausetfiles: + multibind7.patch

messages: + msg249764
2015-09-03 15:16:45ysionneausetfiles: + multibind6.patch

messages: + msg249658
2015-09-02 16:26:26yselivanovsetmessages: + msg249556
2015-09-02 14:41:03vstinnersetmessages: + msg249552
2015-09-02 14:38:28yselivanovsetmessages: + msg249551
2015-09-02 14:35:27gvanrossumsetmessages: + msg249549
2015-09-02 14:31:58ysionneausetmessages: + msg249548
2015-09-02 14:28:05vstinnersetmessages: + msg249547
2015-09-02 14:27:42yselivanovsetmessages: + msg249546
2015-09-02 14:19:11vstinnersetmessages: + msg249545
2015-09-02 14:07:48vstinnersetmessages: + msg249544
2015-08-28 14:06:12ysionneausetfiles: + multibind4.patch

messages: + msg249279
2015-08-27 19:07:10gvanrossumsetmessages: + msg249252
2015-08-27 19:05:47ysionneausetmessages: + msg249250
2015-08-27 19:04:30gvanrossumsetmessages: + msg249249
2015-08-27 19:03:57gvanrossumsetmessages: + msg249248
2015-08-27 19:00:17yselivanovsetmessages: + msg249245
2015-08-27 18:01:21gvanrossumsetmessages: + msg249244
2015-08-27 17:24:06yselivanovsetmessages: + msg249243
2015-08-27 16:56:37ysionneausetfiles: + multibind3.patch

messages: + msg249240
2015-08-27 16:48:41yselivanovsetmessages: + msg249238
2015-08-27 16:43:40ysionneausetmessages: + msg249236
2015-08-26 22:35:46yselivanovsetmessages: + msg249220
2015-08-26 21:02:50ysionneausetfiles: + multibind2.patch

messages: + msg249218
2015-08-26 20:58:11ysionneausetfiles: - multibind2.patch
2015-08-26 20:47:38yselivanovsetmessages: + msg249216
2015-08-26 20:41:53ysionneausetfiles: + multibind2.patch

messages: + msg249215
2015-07-01 06:35:20vstinnersetmessages: + msg246025
2015-04-21 08:38:11ysionneausetmessages: + msg241707
2015-03-20 10:46:35ysionneausetmessages: + msg238641
2015-03-11 13:18:28vstinnersetfiles: + multibind.patch

messages: + msg237863
2015-03-11 13:07:15ysionneausetfiles: + asyncio_multibind_test.diff
nosy: + ysionneau
messages: + msg237862

2015-03-10 19:28:35gvanrossumsetmessages: + msg237803
2015-03-10 19:20:56sebastien.bourdeauducqsetfiles: + asyncio_multibind.diff
keywords: + patch
messages: + msg237800
2015-03-10 18:46:55gvanrossumsetmessages: + msg237795
2015-03-10 18:45:02sebastien.bourdeauducqsetmessages: + msg237794
2015-03-10 18:27:09gvanrossumsetmessages: + msg237792
2015-03-10 18:18:41sebastien.bourdeauducqcreate