Code review - Issue 8307045: listen on multiple addresses (and families)https://codereview.appspot.com/2013-04-23T16:59:20+00:00rietveld
Message from unknown
2013-04-03T12:45:09+00:00Giampaolo Rodolaurn:md5:7b5af0f6a7f6a8be52f963f11a86044e
Message from andrew.svetlov@gmail.com
2013-04-03T18:49:51+00:00Andrew Svetlovurn:md5:ac49974b8c1079fd1cdea5ba6e1564c5
Some comments
https://codereview.appspot.com/8307045/diff/1/tulip/base_events.py
File tulip/base_events.py (right):
https://codereview.appspot.com/8307045/diff/1/tulip/base_events.py#newcode455
tulip/base_events.py:455: reuse_addr = os.name == 'posix' and sys.platform != 'cygwin'
Perhaps reuse_addr should be added to function signature with None default value.
If None — use heuristic like this line.
If True — force SO_REUSEADDR
If False — never set SO_REUSERADDR
Thoughs?
https://codereview.appspot.com/8307045/diff/1/tulip/base_events.py#newcode485
tulip/base_events.py:485: finally:
If I've understood correctly you close already created sockets if some family has been already bound.
That's fine.
Should we raise more informative exception like:
cannot start serving {host}:{port} for {interface} since it's already allocated?
Message from g.rodola@gmail.com
2013-04-03T20:31:05+00:00Giampaolo Rodolaurn:md5:56e343dbae9d3cba007fb787d6a88259
https://codereview.appspot.com/8307045/diff/1/tulip/base_events.py
File tulip/base_events.py (right):
https://codereview.appspot.com/8307045/diff/1/tulip/base_events.py#newcode455
tulip/base_events.py:455: reuse_addr = os.name == 'posix' and sys.platform != 'cygwin'
Agreed.
https://codereview.appspot.com/8307045/diff/1/tulip/base_events.py#newcode485
tulip/base_events.py:485: finally:
Agreed.
Message from g.rodola@gmail.com
2013-04-03T20:34:36+00:00Giampaolo Rodolaurn:md5:5ec781e1f997d3d45ae3df4ed8049ed7
I need to post an updated patch.
Can I somehow update this ticket or shall I create a brand new one?
Message from fafhrd91@gmail.com
2013-04-03T20:41:31+00:00Nikolay Kimurn:md5:3f54d78ec8fa89f2bd095c488d712d5b
On 2013/04/03 20:34:36, g.rodola wrote:
> I need to post an updated patch.
> Can I somehow update this ticket or shall I create a brand new one?
upload.py -i 8307045
Message from unknown
2013-04-03T20:44:24+00:00Giampaolo Rodolaurn:md5:a7ee8e3173e187c94b5cda89fa921439
Message from g.rodola@gmail.com
2013-04-03T20:45:10+00:00Giampaolo Rodolaurn:md5:3499e6662aa8389c5e11cf27357545ee
Thanks, done.
2013/4/3 <fafhrd91@gmail.com>:
> On 2013/04/03 20:34:36, g.rodola wrote:
>>
>> I need to post an updated patch.
>> Can I somehow update this ticket or shall I create a brand new one?
>
>
> upload.py -i 8307045
>
> https://codereview.appspot.com/8307045/
Message from fafhrd91@gmail.com
2013-04-03T20:55:44+00:00Nikolay Kimurn:md5:4d542f621e0e3b16fc4c70864121b8e0
https://codereview.appspot.com/8307045/diff/2002/tests/events_test.py
File tests/events_test.py (right):
https://codereview.appspot.com/8307045/diff/2002/tests/events_test.py#newcode796
tests/events_test.py:796: # This is quite mysterious, but necessary.
this is probably "accept" and then "data_received"
i'd add test if server actually receiving b'xxx'
https://codereview.appspot.com/8307045/diff/2002/tulip/base_events.py
File tulip/base_events.py (right):
https://codereview.appspot.com/8307045/diff/2002/tulip/base_events.py#newcode469
tulip/base_events.py:469: type=socket.SOCK_STREAM, proto=0, flags=flags)
what if i want specific protocol?
Message from guido@python.org
2013-04-03T21:44:29+00:00gvrpythonurn:md5:94883481e06eb443c3a75cce93e3262b
Using the upload.py script, it's simple:
upload.py -i 8307045
On Wed, Apr 3, 2013 at 1:34 PM, <g.rodola@gmail.com> wrote:
> I need to post an updated patch.
> Can I somehow update this ticket or shall I create a brand new one?
>
> https://codereview.appspot.**com/8307045/<https://codereview.appspot.com/8307045/>
>
--
--Guido van Rossum (python.org/~guido)
Message from g.rodola@gmail.com
2013-04-03T22:55:17+00:00Giampaolo Rodolaurn:md5:d4e46eed3c1c8eadf11980918fddfe67
https://codereview.appspot.com/8307045/diff/2002/tulip/base_events.py
File tulip/base_events.py (right):
https://codereview.appspot.com/8307045/diff/2002/tulip/base_events.py#newcode469
tulip/base_events.py:469: type=socket.SOCK_STREAM, proto=0, flags=flags)
On 2013/04/03 20:55:44, Nikolay Kim wrote:
> what if i want specific protocol?
Are there cases where you actually want to do that?
Message from fafhrd91@gmail.com
2013-04-03T23:02:38+00:00Nikolay Kimurn:md5:2347348b45701098a5f6450b19249f1b
On 2013/04/03 22:55:17, g.rodola wrote:
> https://codereview.appspot.com/8307045/diff/2002/tulip/base_events.py
> File tulip/base_events.py (right):
>
> https://codereview.appspot.com/8307045/diff/2002/tulip/base_events.py#newcode469
> tulip/base_events.py:469: type=socket.SOCK_STREAM, proto=0, flags=flags)
> On 2013/04/03 20:55:44, Nikolay Kim wrote:
> > what if i want specific protocol?
>
> Are there cases where you actually want to do that?
ah. i didnt notice you removed proto from start_serving.
i think you should use "*" after port=None for start_serving.
Message from unknown
2013-04-03T23:31:21+00:00Giampaolo Rodolaurn:md5:91cf46593e53566dc133d69455dff1cd
Message from g.rodola@gmail.com
2013-04-03T23:34:54+00:00Giampaolo Rodolaurn:md5:0ba76d1940b15a40505b809e29a505a6
On 2013/04/03 23:02:38, Nikolay Kim wrote:
> On 2013/04/03 22:55:17, g.rodola wrote:
> > https://codereview.appspot.com/8307045/diff/2002/tulip/base_events.py
> > File tulip/base_events.py (right):
> >
> >
> https://codereview.appspot.com/8307045/diff/2002/tulip/base_events.py#newcode469
> > tulip/base_events.py:469: type=socket.SOCK_STREAM, proto=0, flags=flags)
> > On 2013/04/03 20:55:44, Nikolay Kim wrote:
> > > what if i want specific protocol?
> >
> > Are there cases where you actually want to do that?
>
> ah. i didnt notice you removed proto from start_serving.
>
> i think you should use "*" after port=None for start_serving.
done
Message from fafhrd91@gmail.com
2013-04-03T23:53:10+00:00Nikolay Kimurn:md5:455d2dcdc7980c380bf629343b96e3ae
You should change AbstractEventLoop.start_serving signature
to reflect new api. I'd move doc string as well.
otherwise LGTM.
Message from unknown
2013-04-04T08:49:50+00:00Giampaolo Rodolaurn:md5:9f5cfdad17522ed0b8c6b9af70384773
Message from gvanrossum@gmail.com
2013-04-23T03:29:55+00:00GvRurn:md5:2c0c6f1e713cbcef2d83b55214c454e7
There's a start_serving() call in srv.py that will break with this change. Possibly other calls in other examples recently added too. Please fix those. Also one comment about docstring markup.
https://codereview.appspot.com/8307045/diff/22001/tulip/events.py
File tulip/events.py (right):
https://codereview.appspot.com/8307045/diff/22001/tulip/events.py#newcode188
tulip/events.py:188: """Creates a TCP server bound to *host* and *port* and return
Please don't use markup like *host* to refer to the arguments. It sounds a bit like *shouting* to my ears.
Message from unknown
2013-04-23T16:59:20+00:00Giampaolo Rodolaurn:md5:ff2d8995d36c26d62903717ac7a06779