msg103942 - (view) |
Author: Daniel Evers (Daniel.Evers) |
Date: 2010-04-22 10:02 |
I'm trying to rewrite a server application in python that accepts exactly 1 connection. I have a previous version written in C that can call listen() on a socket with a backlog of 0 connections, but this is not possible in python. In Modules/socketmodule.c (function socket_listen) the backlog is reset to at least "1".
|
msg103957 - (view) |
Author: Charles-François Natali (neologix) *  |
Date: 2010-04-22 12:00 |
That's a non-portable user of listen()'s backlog, see http://www.opengroup.org/onlinepubs/009695399/functions/listen.html:
A backlog argument of 0 may allow the socket to accept connections, in which case the length of the listen queue may be set to an implementation-defined minimum value.
Furthermore, python's socket documentation makes it clear:
Enable a server to accept connections. The backlog argument must be at least 1; it specifies the number of unaccepted connection that the system will allow before refusing new connections.
If you really want to only accept 1 connection (I'm talking of sending back a SYN-ACK, not an accept() call), you can just close the server socket as soon as you've done the accept, and re-open it when you're ready to process another request. Or you could accept new connections and close them immediately (but then you'd send back a SYN-ACK then a FIN).
So I'd say it's not a python bug.
|
msg103961 - (view) |
Author: Jean-Paul Calderone (exarkun) *  |
Date: 2010-04-22 13:12 |
> Furthermore, python's socket documentation makes it clear:
Why does CPython go out of its way to make it impossible to pass 0 to the platform listen() API? The part of the specification you quoted makes it very clear that 0 is a valid value for this argument. And 0 seems to have a very different meaning than 1, but for some reason CPython thinks that 1 is a better thing for 0 to mean.
Perhaps there is a real reason someone thought to add this check in sock_listen, but if not, I think it's worth considering removing this check.
Unfortunately the commit message for the revision which added this check (r3880) is not informative:
sock_listen: ensure backlog argument is at least1
> If you really want to only accept 1 connection (I'm talking of sending back a SYN-ACK, not an accept() call), you can just close the server socket
This makes perfect sense, though, and is good advice in general.
|
msg103978 - (view) |
Author: Daniel Evers (Daniel.Evers) |
Date: 2010-04-22 20:01 |
Thanks for your help. I'll close the listen socket.
Nevertheless I'd like to understand the check in sock_listen. The commit message is not very helpful, I agree, that's why I wrote this issue. 0 is a valid value, and I'd suggest two ways to handle this situation:
1. Always set backlog at least to 1 (like now), but make this explicit in the documentation (docs.python.org says "should be at least 1" which is not the same as "will be at least 1")
2. Allow backlog = 0 and make a note in the docs that the exact behaviour will be implementation/OS-specific.
In case (1) the hint to close the listen socket should also be added to the documentation.
|
msg103986 - (view) |
Author: Antoine Pitrou (pitrou) *  |
Date: 2010-04-22 23:16 |
The request to allow 0 looks reasonable to me. Now if someone wants to propose a patch (+ tests if there's any way of testing for this).
|
msg104072 - (view) |
Author: Daniel Evers (Daniel.Evers) |
Date: 2010-04-24 09:42 |
I attached a patch:
The backlog is set to at least 0 instead of 1. I also added a comment that a backlog < 0 can lead to problems and doesn't make sense anyway (so if there are systems that may crash with backlog < 0 this will be avoided).
|
msg104073 - (view) |
Author: Charles-François Natali (neologix) *  |
Date: 2010-04-24 09:57 |
It looks like you forgot to update the function's documentation ;-)
|
msg104075 - (view) |
Author: Daniel Evers (Daniel.Evers) |
Date: 2010-04-24 11:54 |
Ah right ;)
Sorry, attached the path incl. the doc string.
|
msg104076 - (view) |
Author: Daniel Evers (Daniel.Evers) |
Date: 2010-04-24 11:58 |
A second patch for the documentation of socket.listen().
|
msg104137 - (view) |
Author: Jean-Paul Calderone (exarkun) *  |
Date: 2010-04-25 01:16 |
It'd be nice to have a unit test that passes a small enough value to listen() to trigger the check. Since there's no way to reliably determine what the system backlog really is, there's probably no reason to actually try to determine that the "right" value was passed to listen. Just making sure the code actually goes through the success-case when this check is hit is probably enough.
|
msg134962 - (view) |
Author: Daniel Evers (Daniel.Evers) |
Date: 2011-05-02 12:12 |
To revive this issue, I tried to write a unit test to verify the behaviour. Onfurtunately, the test doesn't work and I don't understand why. I hope, someone here is more enlightend than me...
(files: server.py, client.py)
|
msg134963 - (view) |
Author: Daniel Evers (Daniel.Evers) |
Date: 2011-05-02 12:12 |
(client.py)
|
msg135070 - (view) |
Author: Charles-François Natali (neologix) *  |
Date: 2011-05-03 21:00 |
> To revive this issue, I tried to write a unit test to verify the behaviour.
> Onfurtunately, the test doesn't work and I don't understand why. I hope,
> someone here is more enlightend than me...
The semantic of listen's backlog argument has always been unclear, so
different implementations apply some "fudge factor" to the value
passed, see for example
http://books.google.com/books?id=ptSC4LpwGA0C&lpg=PA108&ots=Kq9FQogkTr&dq=berkeley%20listen%20backlog%20ack&pg=PA108#v=onepage&q=berkeley
listen backlog ack&f=false
Most of the time, passing <backlog> means that the kernel will queue
at least <backlog> requests.
Under the kernel I'm using (2.6.32), the maximum number of queued
requests is actually backlog + 1, and it seems to be the same on
FreeBSD (I guess one of the reason is to cope with backlog == 0).
Note that under Linux, there's another setting coming into play,
net.ipv4.tcp_abort_on_overflow:
"""
If listening service is too slow to accept new connections, reset
them. Not enabled by default. It means that if overflow occurred due
to a burst, connection will recover.
Enable this option only if you are really sure that listening daemon
cannot be tuned to accept connections faster. Enabling this option can
harm clients of your server.
"""
The goal is to avoid dropping connection requests because of a temporary burst.
So if you change your script to make as many connections as possible
with the default value, you'll probably be able to go well above
listen's backlog.
To deactivate it, you can use:
# sysctl net.ipv4.tcp_abort_on_overflow=1
(But it's not a good idea in a production setting).
As for the test, you should add it to Lib/test/test_socket.py (just
creating a socket, binding it and passing a backlog of 0 should be
more than enough).
|
msg135105 - (view) |
Author: Daniel Evers (Daniel.Evers) |
Date: 2011-05-04 09:40 |
Thanks for the tip. I added the unit test and uploaded my final patch (which includes all changes).
Is it ok to remove the files I uploaded previously?
|
msg135230 - (view) |
Author: Charles-François Natali (neologix) *  |
Date: 2011-05-05 18:53 |
> Thanks for the tip. I added the unit test and uploaded my final patch
> (which includes all changes).
A couple comments (note that I'm not entitled to accept or commit a patch, so feel free to ignore them if I'm just being a pain):
- "the maximum value is system-dependent (usually 5)"
I would remove this "usually 5" part: since the kernel silently adjusts the value, it shouldn't be of interest to the programmer. Furthermore, this was probably true a decade ago, but nowadays it doesn't hold anymore, Linux for example allows a much higher limit
- port = support.bind_port(srv)
Since you don't care about which port you get bound to, you could discard the port value, or even call srv.bind((HOST, 0)) directly
- I'm not sure that creating a new SocketListenBacklog0Test TestCase just for this check is necessary. Since it's so short, you could just add a new test to e.g. GeneralModuleTests
> Is it ok to remove the files I uploaded previously?
It's probably a good idea, so there won't be any confusion with "stale" patches and test scripts.
|
msg135594 - (view) |
Author: Daniel Evers (Daniel.Evers) |
Date: 2011-05-09 15:01 |
Thanks, I removed the old patches and changed the unit test according to your suggestion.
I kept the "usually 5" remark, because I'm not sure how reality really looks like. But feel free to suggest a patch ;)
|
msg135651 - (view) |
Author: Charles-François Natali (neologix) *  |
Date: 2011-05-09 21:10 |
The patch looks fine to me: you just need to find someone interested
in reviewing and committing it (didn't find anyone listed as expert
for the socket module).
|
msg135721 - (view) |
Author: Roundup Robot (python-dev)  |
Date: 2011-05-10 17:20 |
New changeset 48743ad2d2ef by Antoine Pitrou in branch '2.7':
Issue #8498: In socket.accept(), allow to specify 0 as a backlog value in
http://hg.python.org/cpython/rev/48743ad2d2ef
New changeset 713006ecee0c by Antoine Pitrou in branch '3.2':
Issue #8498: In socket.accept(), allow to specify 0 as a backlog value in
http://hg.python.org/cpython/rev/713006ecee0c
New changeset 5e1ed84883c5 by Antoine Pitrou in branch 'default':
Issue #8498: In socket.accept(), allow to specify 0 as a backlog value in
http://hg.python.org/cpython/rev/5e1ed84883c5
|
msg135722 - (view) |
Author: Antoine Pitrou (pitrou) *  |
Date: 2011-05-10 17:22 |
Committed, thanks for the patch and review!
|
|
Date |
User |
Action |
Args |
2022-04-11 14:57:00 | admin | set | github: 52744 |
2011-05-10 17:22:03 | pitrou | set | status: open -> closed versions:
+ Python 3.3, - Python 2.6, Python 3.1 messages:
+ msg135722
resolution: fixed stage: resolved |
2011-05-10 17:20:49 | python-dev | set | nosy:
+ python-dev messages:
+ msg135721
|
2011-05-09 21:10:23 | neologix | set | messages:
+ msg135651 |
2011-05-09 15:01:02 | Daniel.Evers | set | files:
+ socket_backlog0.patch
messages:
+ msg135594 |
2011-05-09 14:57:24 | Daniel.Evers | set | files:
- backlog0_complete.patch |
2011-05-09 14:57:21 | Daniel.Evers | set | files:
- client.py |
2011-05-09 14:57:19 | Daniel.Evers | set | files:
- server.py |
2011-05-09 14:57:17 | Daniel.Evers | set | files:
- socket_listen.patch |
2011-05-09 14:57:14 | Daniel.Evers | set | files:
- backlog0_incl_doc.diff |
2011-05-09 14:57:12 | Daniel.Evers | set | files:
- backlog0.diff |
2011-05-05 18:53:48 | neologix | set | keywords:
+ needs review
messages:
+ msg135230 |
2011-05-04 09:40:16 | Daniel.Evers | set | files:
+ backlog0_complete.patch
messages:
+ msg135105 |
2011-05-03 21:00:23 | neologix | set | messages:
+ msg135070 |
2011-05-02 12:12:28 | Daniel.Evers | set | files:
+ client.py
messages:
+ msg134963 |
2011-05-02 12:12:13 | Daniel.Evers | set | files:
+ server.py
messages:
+ msg134962 |
2010-04-25 01:16:20 | exarkun | set | messages:
+ msg104137 |
2010-04-24 11:58:27 | Daniel.Evers | set | files:
+ socket_listen.patch
messages:
+ msg104076 |
2010-04-24 11:54:24 | Daniel.Evers | set | files:
+ backlog0_incl_doc.diff
messages:
+ msg104075 |
2010-04-24 09:57:20 | neologix | set | messages:
+ msg104073 |
2010-04-24 09:42:26 | Daniel.Evers | set | files:
+ backlog0.diff keywords:
+ patch messages:
+ msg104072
|
2010-04-22 23:16:37 | pitrou | set | priority: low versions:
+ Python 3.1, Python 2.7, Python 3.2 |
2010-04-22 23:16:20 | pitrou | set | nosy:
+ pitrou messages:
+ msg103986
|
2010-04-22 20:02:01 | Daniel.Evers | set | messages:
+ msg103978 |
2010-04-22 13:22:25 | tarek | set | assignee: tarek -> components:
+ Library (Lib), - Distutils |
2010-04-22 13:12:44 | exarkun | set | messages:
+ msg103961 |
2010-04-22 12:00:30 | neologix | set | nosy:
+ tarek, neologix messages:
+ msg103957
assignee: tarek components:
+ Distutils, - Extension Modules |
2010-04-22 10:33:18 | pitrou | set | nosy:
+ exarkun
|
2010-04-22 10:02:51 | Daniel.Evers | create | |