classification
Title: add SOCK_NONBLOCK and SOCK_CLOEXEC to socket module
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.2
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: BreamoreBoy, exarkun, giampaolo.rodola, lekma, nvetoshkin, pitrou, r.david.murray
Priority: normal Keywords: patch

Created on 2009-12-16 11:00 by lekma, last changed 2011-10-27 16:45 by nvetoshkin. This issue is now closed.

Files
File name Uploaded Description Edit
Issue7523.diff lekma, 2009-12-16 12:31
Issue7523_2.diff lekma, 2009-12-17 07:24
Issue7523_3.diff lekma, 2009-12-20 09:18
issue7523_py3k.diff nvetoshkin, 2010-10-12 18:53 Ported to py3k patch
issue7523_py3k_accept4.diff nvetoshkin, 2010-10-12 20:49 Added accept4 via ifdef
issue7523_py3k_accept4_1.diff nvetoshkin, 2010-10-13 19:41 new test and proper accept4 check
issue7523_py3k_accept4_2.diff nvetoshkin, 2010-10-13 21:07
Messages (25)
msg96482 - (view) Author: (lekma) Date: 2009-12-16 11:00
It would be nice to have those.
See http://udrepper.livejournal.com/20407.html for background.
msg96484 - (view) Author: (lekma) Date: 2009-12-16 12:31
First attempt, against trunk.
msg96504 - (view) Author: (lekma) Date: 2009-12-17 07:24
better patch (mainly test refactoring).
still against trunk.

should I provide one against py3k?
msg96513 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-12-17 13:50
A couple of things:
- the test shouldn't be marked as Linux-specific since perhaps these
constants will be supported by other systems some day
- when importing fcntl, bear in mind that some systems don't have this
module (e.g. Windows), so you should catch and silence the ImportError
msg96551 - (view) Author: (lekma) Date: 2009-12-18 07:52
> - when importing fcntl, bear in mind that some systems don't have this
> module (e.g. Windows), so you should catch and silence the ImportError

would something like the following be ok?:

try:
    import fcntl
except ImportError:
    fcntl = False

and then:

if hasattr(socket, "SOCK_CLOEXEC") and fcntl:
    tests.append(CloexecLinuxConstantTest)
msg96559 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2009-12-18 11:14
It would be better to use test skipping: (eg: @unittest.SkipUnless
before the test class).
msg96598 - (view) Author: (lekma) Date: 2009-12-19 09:58
> It would be better to use test skipping: (eg: @unittest.SkipUnless
> before the test class).

I didn't know about this feature, thanks for the tip.

Now I wonder if it would be better to do it this way:
@unittest.SkipUnless(hasattr(socket, "SOCK_CLOEXEC") and fcntl,
"SOCK_CLOEXEC not defined OR module fcntl not available")

or this way:
@unittest.SkipUnless(hasattr(socket, "SOCK_CLOEXEC"), "SOCK_CLOEXEC not
defined")
@unittest.SkipUnless(fcntl, "module fcntl not available")

the second option seems better to me (obvious reason why the test was
skipped), what do you guys think? (it doesn't really matter, I know, but
while we're here...)
msg96609 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2009-12-19 16:09
I lean toward the second way, myself.
msg96666 - (view) Author: (lekma) Date: 2009-12-20 09:18
this one addresses Antoines's comments (with the help of R. David Murray).
msg97636 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-01-12 17:53
The patch is basically fine. I'll add a "try .. finally" to the tests.
lekma, do you have a real name that we should add to the ACKS file?
msg97641 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-01-12 18:05
Looking at it again, there's the question of accept() behaviour. In the original code, it will call internal_setblocking() on the new fd if the listening socket is non-blocking. In the new code, if SOCK_NONBLOCK is defined it will not call any OS function to set the new fd in non-blocking mode.

Here is what the man page has to say:

       On Linux, the new socket returned by accept() does not inherit file status  flags  such  as
       O_NONBLOCK and O_ASYNC from the listening socket.  This behavior differs from the canonical
       BSD sockets implementation.  Portable programs should not rely on inheritance or non-inher‐
       itance  of  file  status  flags  and always explicitly set all required flags on the socket
       returned from accept().

Linux has defined accept4() precisely for this purpose:

       If flags is 0, then accept4() is the same as accept().  The following values can be bitwise
       ORed in flags to obtain different behavior:

       SOCK_NONBLOCK   Set the O_NONBLOCK file status flag  on  the  new  open  file  description.
                       Using this flag saves extra calls to fcntl(2) to achieve the same result.

       SOCK_CLOEXEC    Set  the  close-on-exec  (FD_CLOEXEC) flag on the new file descriptor.  See
                       the description of the O_CLOEXEC flag in open(2) for reasons why  this  may
                       be useful.
msg97699 - (view) Author: (lekma) Date: 2010-01-13 08:32
> lekma, do you have a real name that we should add to the ACKS file?
no, lekma is fine (that's a nick I carry since childhood)

> Looking at it again, there's the question of accept() behaviour. [...]
Sorry for not having seen that earlier and thanks for pointing it out to me.
Unfortunately, I don't think there is a portable solution. We could restrict the patch to linux 2.6.28 and above (there is already an awful lot of ifdefs in this file).
If you think that's the way to go, I can rework the patch, otherwise I think it's better to close the issue (maybe reopen when bsds catch up, if ever).
Ideas?
msg111182 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2010-07-22 15:37
@Antoine could you respond to msg97699, thanks.
msg111183 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-07-22 15:46
> @Antoine could you respond to msg97699, thanks.

Well my comments and the patch itself are outdated now that 2.7 is in bugfix mode. The socket implementation in 3.2 is slightly different, which means the patch should be updated (it isn't necessarily difficult to do so), and the accept() issue should be examined again.
msg118457 - (view) Author: Vetoshkin Nikita (nvetoshkin) Date: 2010-10-12 18:53
Made an attempt to port lekma's patch to py3k-trunk. No (logical) changes needed.

Don't know about accept4() issue. As I saw in Qt sources, they ifdef'ed CLOEXEC by default on file descriptors. Don't think it's acceptable :) in this particular case. So, @Antoine, what do you say?
msg118463 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-10-12 19:29
The accept() issue is the following: the socket created by accept() (in Lib/socket.py) will formally inherit its parent's `type` attribute (including any SOCK_NONBLOCK and SOCK_CLOEXEC flags).

However, the underlying Linux socket is created by the accept() system call, which doesn't inherit flags as mentioned in the aforementioned man page. Therefore, the Python socket gives the wrong information about the socket's real flags.

This can be witnessed quickly:

>>> s = socket.socket(socket.AF_INET, socket.SOCK_STREAM | socket.SOCK_CLOEXEC)
>>> s.bind(("", 0))
>>> s.getsockname()
('0.0.0.0', 34634)
>>> s.listen(5)
>>> c, a = s.accept()
   # Here, just start a "telnet" or "nc" session from another term
>>> import fcntl
>>> fcntl.fcntl(s, fcntl.F_GETFD)
1
>>> fcntl.fcntl(c, fcntl.F_GETFD)
0
>>> fcntl.fcntl(c, fcntl.F_GETFD) & fcntl.FD_CLOEXEC
0
>>> c.type & socket.SOCK_CLOEXEC
524288


The quick solution would be to mask out these flags when creating the Python socket in accept(). A better solution might be to inherit these flags by using the accept4() system call when possible (this is useful especially for SOCK_CLOEXEC, of course).


Apart from that, the patch looks ok, but it would be nice to test that at least the underlying socket is really in non-blocking mode, like is done in NonBlockingTCPTests.testSetBlocking.
msg118471 - (view) Author: Vetoshkin Nikita (nvetoshkin) Date: 2010-10-12 20:49
Thanks! I can see the problem now, but I think checking should be done like this:
>>> fcntl.fcntl(c, fcntl.F_GETFD) & fcntl.FD_CLOEXEC
0
>>> fcntl.fcntl(s, fcntl.F_GETFD) & fcntl.FD_CLOEXEC
1
and with accept4() call I've got flag set:
>>> fcntl.fcntl(c, fcntl.F_GETFD) & fcntl.FD_CLOEXEC
1
>>> fcntl.fcntl(s, fcntl.F_GETFD) & fcntl.FD_CLOEXEC
1

Don't know how to properly check if accept4 is available.

Second attempt - dropping flags from sock_type should be done on Python level in socket.py and I don't quite like idea to check if SOCK_CLOEXEC is in locals every time.
msg118473 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-10-12 21:03
You can check accept4() presence by adding it to the AC_CHECK_FUNCS macro in configure.in around line 2553, and add the corresponding HAVE_ACCEPT4 lines in pyconfig.h.in. Then run "autoconf" and you will have access to a HAVE_ACCEPT4 constant when accept4() exists.

By the way, you shouldn't use C++-style comments ("// ..."), because some C compilers refuse them.
msg118569 - (view) Author: Vetoshkin Nikita (nvetoshkin) Date: 2010-10-13 19:41
Another patch with:
 - testInitBlocking method
 - no c++ style comments
 - a newly generated configure script (almost 1.5k lines diff)
 - proper accept4 availability check

With this patch I've got 
Traceback (most recent call last):
  File "/home/nekto/workspace/py3k/Lib/test/test_socket.py", line 1564, in testInterruptedTimeout
    foo = self.serv.accept()
socket.timeout: timed out

Can someone test if there's a real regression?
msg118581 - (view) Author: Vetoshkin Nikita (nvetoshkin) Date: 2010-10-13 20:49
Here's what strace on FAIL shows (print "in alarm_handler" added)

alarm(2)                                = 0
poll([{fd=3, events=POLLIN}], 1, 5000)  = ? ERESTART_RESTARTBLOCK (To be restarted)
--- SIGALRM (Alarm clock) @ 0 (0) ---
rt_sigreturn(0xffffffff)                = -1 EINTR (Interrupted system call)
accept4(3, 0x7fffa94c4780, [16], 0)     = -1 EAGAIN (Resource temporarily unavailable)
poll([{fd=3, events=POLLIN}], 1, 2999)  = 0 (Timeout)
accept4(3, 0x7fffa94c4780, [16], 0)     = -1 EAGAIN (Resource temporarily unavailable)
write(1, "in alarm_handler\n", 17)      = 17
alarm(0)                                = 0

Here's on OK:

alarm(2)                                = 0
poll([{fd=3, events=POLLIN}], 1, 5000)  = ? ERESTART_RESTARTBLOCK (To be restarted)
--- SIGALRM (Alarm clock) @ 0 (0) ---
rt_sigreturn(0xffffffff)                = -1 EINTR (Interrupted system call)
write(1, "in alarm_handler\n", 17)      = 17
alarm(0)

For some reason does another trip through BEGIN_SELECT_LOOP() macro
msg118583 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-10-13 21:04
> For some reason does another trip through BEGIN_SELECT_LOOP() macro

Indeed:

     if (!timeout)
+#ifdef HAVE_ACCEPT4
+        /* inherit socket flags and use accept4 call */
+        flags = s->sock_type & (SOCK_CLOEXEC | SOCK_NONBLOCK);
+        newfd = accept4(s->sock_fd, SAS2SA(&addrbuf), &addrlen, flags);
+#else


There's a missing curly brace after "if (!timeout)", so accept4() is always called.
msg118584 - (view) Author: Vetoshkin Nikita (nvetoshkin) Date: 2010-10-13 21:07
@Antoine already found that myself, patched and tested :) thanks!
msg118668 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-10-14 15:22
Patch committed in r85480, thanks!
msg146502 - (view) Author: Vetoshkin Nikita (nvetoshkin) Date: 2011-10-27 16:43
Started implementing accept4() socket method and stuck on socket object's timeout attribute. What value should we assign to sock->sock_timeout if SOCK_NONBLOCK was specified in accept4() call? And in socket.py should we check as in original accept:
if getdefaulttimeout() is None and self.gettimeout():
    sock.setblocking(True)
msg146503 - (view) Author: Vetoshkin Nikita (nvetoshkin) Date: 2011-10-27 16:45
Sorry, wrong ticket. Right one is 10115
History
Date User Action Args
2011-10-27 16:45:04nvetoshkinsetmessages: + msg146503
2011-10-27 16:43:56nvetoshkinsetmessages: + msg146502
2010-10-14 15:22:38pitrousetstatus: open -> closed
resolution: fixed
messages: + msg118668

stage: needs patch -> resolved
2010-10-13 21:07:38nvetoshkinsetfiles: + issue7523_py3k_accept4_2.diff

messages: + msg118584
2010-10-13 21:04:12pitrousetmessages: + msg118583
2010-10-13 20:49:51nvetoshkinsetmessages: + msg118581
2010-10-13 19:41:40nvetoshkinsetfiles: + issue7523_py3k_accept4_1.diff

messages: + msg118569
2010-10-13 14:12:20giampaolo.rodolasetnosy: + giampaolo.rodola
2010-10-12 21:03:36pitrousetmessages: + msg118473
2010-10-12 20:49:13nvetoshkinsetfiles: + issue7523_py3k_accept4.diff

messages: + msg118471
2010-10-12 19:29:47pitrousetmessages: + msg118463
2010-10-12 18:53:27nvetoshkinsetfiles: + issue7523_py3k.diff
nosy: + nvetoshkin
messages: + msg118457

2010-07-22 15:46:41pitrousetnosy: + exarkun
messages: + msg111183

components: - IO
stage: patch review -> needs patch
2010-07-22 15:37:16BreamoreBoysetnosy: + BreamoreBoy

messages: + msg111182
versions: - Python 2.7
2010-01-13 08:32:44lekmasetmessages: + msg97699
2010-01-12 18:05:51pitrousetmessages: + msg97641
2010-01-12 17:53:05pitrousetmessages: + msg97636
2009-12-20 09:18:33lekmasetfiles: + Issue7523_3.diff

messages: + msg96666
2009-12-19 16:09:05r.david.murraysetmessages: + msg96609
2009-12-19 09:58:45lekmasetmessages: + msg96598
2009-12-18 11:14:49r.david.murraysetpriority: normal

nosy: + r.david.murray
messages: + msg96559

stage: patch review
2009-12-18 07:53:00lekmasetmessages: + msg96551
2009-12-17 13:50:46pitrousetnosy: + pitrou
messages: + msg96513
2009-12-17 07:24:52lekmasetfiles: + Issue7523_2.diff

messages: + msg96504
components: + IO
2009-12-16 12:31:33lekmasetfiles: + Issue7523.diff
keywords: + patch
messages: + msg96484
2009-12-16 11:00:36lekmacreate