classification
Title: Support accept4() for atomic setting of flags at socket creation
Type: enhancement Stage: needs patch
Components: Extension Modules Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: anacrolix, doko, exarkun, haypo, lekma, loewis, mmarkk, neologix, nvetoshkin, pitrou
Priority: normal Keywords: patch

Created on 2010-10-15 13:23 by pitrou, last changed 2013-08-27 23:23 by haypo. This issue is now closed.

Files
File name Uploaded Description Edit
issue10115_first_attempt.diff nvetoshkin, 2010-10-16 23:16 review
Messages (27)
msg118767 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-10-15 13:23
When a machine has a newer glibc and an old kernel, accept4 can fail and then Python accept() is unusable. For example:

Traceback (most recent call last):
  File "/home/pybot/buildarea-sid/3.x.klose-debian-sparc/build/Lib/threading.py", line 525, in _bootstrap_inner
    self.run()
  File "/home/pybot/buildarea-sid/3.x.klose-debian-sparc/build/Lib/test/test_asynchat.py", line 37, in run
    conn, client = self.sock.accept()
  File "/home/pybot/buildarea-sid/3.x.klose-debian-sparc/build/Lib/socket.py", line 132, in accept
    fd, addr = self._accept()
socket.error: [Errno 90] Function not implemented

(from http://www.python.org/dev/buildbot/builders/sparc%20Debian%203.x/builds/147/steps/test/logs/stdio )


Improving our configure check wouldn't do a lot of good, since people can reuse Python binaries with different kernels. Perhaps we have to use some kind of runtime fallback.

(Strangely, errno 90 is EMSGSIZE here, and ENOSYS is 38)
msg118768 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-10-15 13:29
Or perhaps we should back out the accept4 change and live with the fact that SOCK_CLOEXEC and SOCK_NONBLOCK can't be inherited by calling accept().
msg118769 - (view) Author: Vetoshkin Nikita (nvetoshkin) Date: 2010-10-15 13:30
Falling back on result 90 is not that difficult, I think I can submit a patch today. What should be checked ENOSYS or 90?
msg118771 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-10-15 13:39
> Falling back on result 90 is not that difficult, I think I can submit
> a patch today. What should be checked ENOSYS or 90?

That's a rather good question. I will enable some debug printout on the
buildbot.
msg118775 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-10-15 13:52
Ok, 90 is ENOSYS on that buildbot :)
msg118781 - (view) Author: Vetoshkin Nikita (nvetoshkin) Date: 2010-10-15 15:23
What about exposing accept4() to python level?
msg118784 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-10-15 15:34
> What about exposing accept4() to python level?

That's another possibility, in which case we would first remove the current accept4-calling code in order to fix the buildbot failure.
msg118787 - (view) Author: Vetoshkin Nikita (nvetoshkin) Date: 2010-10-15 16:02
Weekend is coming, so I can lend a hand in implementing whatever you choose.

Summary:
  * remove accept4() as default and expose it as separate method
  * add runtime fallback if accept4() returns ENOSYS
msg118885 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-10-16 19:36
> Weekend is coming, so I can lend a hand in implementing whatever you
> choose.

I don't have any strong opinion about it, so it's whichever you prefer really :)
msg118906 - (view) Author: Vetoshkin Nikita (nvetoshkin) Date: 2010-10-16 23:16
Made a proof of concept patch (no doc updates yet). Decided to implement separate accept4() method, cause we have already spent enough time dealing with it and rollback would be pity.
msg118939 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-10-17 12:22
There would need to be some tests.

Also, this last part of the patch looks strange:

@@ -3001,6 +3072,10 @@
             PyErr_SetString(PyExc_ValueError,
                             "can't use invalid socket value");
             return -1;
+#ifdef HAVE_ACCEPT4
+        /* These flags are not inherited after accept */
+        type &= ~(SOCK_NONBLOCK & SOCK_CLOEXEC);
+#endif /* HAVE_ACCEPT4 */


What is it meant for? And why does it come right after a "return" statement?
msg118951 - (view) Author: Vetoshkin Nikita (nvetoshkin) Date: 2010-10-17 17:23
>What is it meant for? And why does it come right after a "return" statement?
@Antoine, if fd was supplied and it was correct (not returned with -1), let's drop flags that can't be inherited.
It's a mistake, at that level we don't know whether we should inherit flags or not... I'm a bit confused...
msg118960 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2010-10-17 18:24
> That's another possibility, in which case we would first remove the
> current accept4-calling code in order to fix the buildbot failure.

In Python, the lowest layer facing the operating system always directly
exposes the API as-is, without reinterpreting the user's request. Not
following this principle leads exactly to this kind of problem.

So I think .accept() should only call accept(2), and accept4() should
only be called if explicitly requested by the application.

Exposing it as .accept4(flags) is certainly the most straight-forward
way of doing it, but I could also live with .accept(flags) (i.e.
call accept4 if flags are being passed, hoping that no other system
comes up with another accept extension that has a different integer
argument).
msg119398 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-10-22 18:45
I've removed the accept4() call in the meantime (in r85796), so that this issue can be re-classified as a feature request.
msg128209 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-02-09 12:42
Duplicated in issue11157.
msg137766 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-06-06 18:56
@nvetoshkin
Could you update your patch against py3k?

I've got a couple comments (can't login to Rietveld, it's probably due to the change of my tracker account name):

if(flags & ~(SOCK_NONBLOCK | SOCK_CLOEXEC)) {
    PyErr_SetString(PyExc_ValueError,
        "Wrong flag value");
    return NULL;
}

I'm not sure that's necessary:
- accept4() can sanitize its input itself if necessary (will fail with EINVAL)
- if some flag is added, or another OS doesn't use the same flags, it'll fall out of sync


#ifdef HAVE_ACCEPT4
         /* These flags are not inherited after accept */
         type &= ~(SOCK_NONBLOCK & SOCK_CLOEXEC);
#endif /* HAVE_ACCEPT4 */

SOCK_NONBLOCK & SOCK_CLOEXEC == 0, so this doesn't do much.
Second, you should probably reuse what's done in Lib/socket.py for timeout inheritance upon accept (except that here you certainly don't want to set the socket to blocking if SOCK_NONBLOCK flag is passed).

You should add a test for that: have a look at test_pipe2 in Lib/test/test_posix.py.
msg137812 - (view) Author: Vetoshkin Nikita (nvetoshkin) Date: 2011-06-07 11:45
Yes, I can.
We decided to expose accept4() as another socket method, not accept() replacement?
msg138164 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-06-11 16:27
No one seems to object, and since this approach has been suggested by
Martin and  is consistent with the posix module's policy (i.e. a thin
wrapper around the underlying syscall), I guess you can go ahead.
msg146504 - (view) Author: Vetoshkin Nikita (nvetoshkin) Date: 2011-10-27 16:45
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)
msg147164 - (view) Author: Vetoshkin Nikita (nvetoshkin) Date: 2011-11-06 17:19
up?
msg147166 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-11-06 17:54
> What value should we assign to sock->sock_timeout if SOCK_NONBLOCK was 
> specified in accept4() call?

The same value as for other non-blocking sockets, IMO.

> And in socket.py should we check as in original accept:
> if getdefaulttimeout() is None and self.gettimeout():
>    sock.setblocking(True)

I don't think so, since the whole point of accept4() is to override normal flags creation.
msg147167 - (view) Author: Vetoshkin Nikita (nvetoshkin) Date: 2011-11-06 18:12
> The same value as for other non-blocking sockets, IMO.
There are three possible values I think:
1. parent's current sock_timeout
2. global default socket timeout
3. 0

Can you please tell which one? I assume it should be number 3.
msg147168 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-11-06 18:13
> > The same value as for other non-blocking sockets, IMO.
> There are three possible values I think:
> 1. parent's current sock_timeout
> 2. global default socket timeout
> 3. 0
> 
> Can you please tell which one? I assume it should be number 3.

Yes (again, IMO).
msg154553 - (view) Author: Matt Joiner (anacrolix) Date: 2012-02-28 14:49
Can we get this exposed as an os.accept4, and an optional flags parameter to socket.socket.accept?
msg179839 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2013-01-12 23:45
See the PEP 433 which proposes an unified API to set/unset close-on-exec flag on any function creating a file descriptor.
msg180192 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2013-01-18 13:56
My implementation of the PEP 433 uses accept4() for socket.accept() if the (new) cloexec parameter is True:
http://hg.python.org/features/pep-433/file/46b7a077ae87/Modules/socketmodule.c#l1961

The code fallbacks to accept() if accept4() fails with ENOSYS. It happens if the glibc version is 2.10 or newer, whereas the Linux kernel is older than 2.6.28. I didn't test the fallback yet.

I see in changeset 12442ac3f7dd (issue #7523) that SOCK_NONBLOCK is also set in the flags parameters of accept4(). I should probably also do that.
msg196330 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2013-08-27 23:23
Python 3.4 now uses accept4() internally for socket.socket.accept(), the new socket is created non-inheritable. See the PEP 446 for more information (PEP implemented in the issue #18571).
History
Date User Action Args
2013-08-27 23:23:20hayposetversions: + Python 3.4, - Python 3.3
2013-08-27 23:23:11hayposetstatus: open -> closed
resolution: fixed
messages: + msg196330
2013-01-18 13:56:23hayposetmessages: + msg180192
2013-01-12 23:45:06hayposetmessages: + msg179839
2012-02-28 14:49:12anacrolixsetnosy: + anacrolix
messages: + msg154553
2011-11-06 18:13:38pitrousetmessages: + msg147168
2011-11-06 18:12:12nvetoshkinsetmessages: + msg147167
2011-11-06 17:54:05pitrousetmessages: + msg147166
versions: + Python 3.3, - Python 3.2
2011-11-06 17:19:52nvetoshkinsetmessages: + msg147164
2011-10-27 16:45:09nvetoshkinsetmessages: + msg146504
2011-06-11 16:27:34neologixsetmessages: + msg138164
2011-06-07 11:45:38nvetoshkinsetmessages: + msg137812
2011-06-06 18:56:30neologixsetmessages: + msg137766
2011-05-23 11:46:44pitrousetnosy: + haypo, neologix
2011-02-09 12:42:25pitrousetnosy: + mmarkk
messages: + msg128209
2011-02-09 12:42:00pitroulinkissue11157 superseder
2010-10-22 18:45:46pitrousetpriority: critical -> normal
type: behavior -> enhancement
messages: + msg119398

title: accept4 can fail with errno 90 -> Support accept4() for atomic setting of flags at socket creation
2010-10-17 18:24:59loewissetmessages: + msg118960
2010-10-17 17:23:57nvetoshkinsetmessages: + msg118951
2010-10-17 12:22:10pitrousetmessages: + msg118939
2010-10-16 23:17:01nvetoshkinsetfiles: + issue10115_first_attempt.diff
keywords: + patch
messages: + msg118906
2010-10-16 19:36:28pitrousetmessages: + msg118885
2010-10-15 16:07:38pitrousetnosy: + exarkun
2010-10-15 16:02:37nvetoshkinsetmessages: + msg118787
2010-10-15 15:34:15pitrousetnosy: + loewis
messages: + msg118784
2010-10-15 15:23:05nvetoshkinsetmessages: + msg118781
2010-10-15 13:52:34pitrousetmessages: + msg118775
2010-10-15 13:39:31pitrousetmessages: + msg118771
2010-10-15 13:30:49nvetoshkinsetmessages: + msg118769
2010-10-15 13:29:43pitrousetmessages: + msg118768
2010-10-15 13:23:23pitroucreate