bpo-28134: Auto-detect socket values from file descriptor #1349

tiran · 2017-04-28T19:07:29Z

Fix socket(fileno=fd) by auto-detecting the socket's family, type,
and proto from the file descriptor. The auto-detection can be overruled
by passing in family, type, and proto explicitly.

Without the fix, all socket except for TCP/IP over IPv4 are basically broken:

>>> s = socket.create_connection(('www.python.org', 443))
>>> s
<socket.socket fd=3, family=AddressFamily.AF_INET6, type=SocketKind.SOCK_STREAM, proto=6, laddr=('2003:58:bc4a:3b00:56ee:75ff:fe47:ca7b', 59730, 0, 0), raddr=('2a04:4e42:1b::223', 443, 0, 0)>
>>> socket.socket(fileno=s.fileno())
<socket.socket fd=3, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=0, laddr=('2003:58:bc4a:3b00::%2550471192', 59730, 0, 2550471192), raddr=('2a04:4e42:1b:0:700c:e70b:ff7f:0%2550471192', 443, 0, 2550471192)>

Signed-off-by: Christian Heimes christian@python.org

https://bugs.python.org/issue28134

tiran · 2017-04-28T19:08:30Z

Lib/socket.py

        # For user code address family and type values are IntEnum members, but
        # for the underlying _socket.socket they're just integers. The
        # constructor of _socket.socket converts the given argument to an
        # integer automatically.
+        if fileno is None:


This block is not necessary but makes the code easier to understand for non-C coders

Do you propose to remove it? If not, I suggest None for default values rather than −1.

agreed, while -1 falling through to socketmodule.c would trigger its default behavior, writing this here in Python would be nicer using the =None idiom.

nascheme · 2017-04-28T20:35:12Z

I like this better than my fromfd2() patch (https://bugs.python.org/issue27377). A few comments:

I think the tests need to be conditional based on platform. I suspect they only pass on Linux as they are.
I suggest adding a version change note to socket.socket, e.g.

.. versionchanged:: 3.7
If supported by platform, get socket family, type and protocol from
passed fileno.

I think querying a file descriptor for this information could be a useful operation without tying it to socket.socket(). I'm going to create a separate pull request that adds socket.fdtype(). The unit tests you have added could check for socket.fdtype before enabling them (e.g. hasattr(socket, 'fdtype')). I have created a HAVE_FDTYPE define that should only be enabled on platforms that support the operation.

tiran · 2017-04-29T10:14:23Z

@nascheme my patch has a big catch. On some platforms getsockname() does not work with sockets that are neither bound nor connected. For that reason my tests are currently failing on Windows. IIRC you were running into the same issue with socket.fdtype(). https://msdn.microsoft.com/de-de/library/windows/desktop/ms738543(v=vs.85).aspx

gpshead · 2017-09-05T17:19:51Z

Lib/test/_test_multiprocessing.py

@@ -4083,7 +4083,7 @@ def get_high_socket_fd(self):

    def close(self, fd):
        if WIN32:
-            socket.socket(fileno=fd).close()
+            socket.socket(socket.AF_INET, socket.SOCK_STREAM, fileno=fd).close()


Use keyword arguments for all of these.

gpshead · 2017-09-05T17:20:17Z

Lib/socket.py

        # For user code address family and type values are IntEnum members, but
        # for the underlying _socket.socket they're just integers. The
        # constructor of _socket.socket converts the given argument to an
        # integer automatically.
+        if fileno is None:


agreed, while -1 falling through to socketmodule.c would trigger its default behavior, writing this here in Python would be nicer using the =None idiom.

gpshead · 2017-09-05T17:37:00Z

Modules/socketmodule.c

+#else
+                    PyErr_SetFromErrnoWithFilename(PyExc_OSError, "type");
+#endif
+                    return -1;


any idea if SO_TYPE or SO_PROTOCOL are ever actually undefined on any platforms? If so, it'd be nice if we could make the exception raised as a result of this contain error message text indicating that the type and protocol must be supplied on the given platform.

gpshead · 2017-12-22T23:55:44Z

is this going to go in?

nascheme · 2017-12-23T19:11:03Z

I have no cycles to review it again but I will say I'd be happy if 3.7 had this functionality. When you try to write Python code that uses systemd inherited sockets, you need something like it.

tiran · 2017-12-24T18:20:27Z

The changes may break applications that are handling socket fds wrongly. I'm going to drop a mail on python-dev first.

Fix socket(fileno=fd) by auto-detecting the socket's family, type, and proto from the file descriptor. The auto-detection can be overruled by passing in family, type, and proto explicitly. Without the fix, all socket except for TCP/IP over IPv4 are basically broken: >>> s = socket.create_connection(('www.python.org', 443)) >>> s <socket.socket fd=3, family=AddressFamily.AF_INET6, type=SocketKind.SOCK_STREAM, proto=6, laddr=('2003:58:bc4a:3b00:56ee:75ff:fe47:ca7b', 59730, 0, 0), raddr=('2a04:4e42:1b::223', 443, 0, 0)> >>> socket.socket(fileno=s.fileno()) <socket.socket fd=3, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=0, laddr=('2003:58:bc4a:3b00::%2550471192', 59730, 0, 2550471192), raddr=('2a04:4e42:1b:0:700c:e70b:ff7f:0%2550471192', 443, 0, 2550471192)> Signed-off-by: Christian Heimes <christian@python.org>

tiran · 2018-01-16T15:40:01Z

@gpshead @nascheme I like to land #5048 first, so I can remove some hacks for closing the socket fd on Windows.

gpshead · 2018-01-29T17:26:20Z

3.7 deadline is at today... 2018-01-29 ~23:59 Anywhere on Earth (UTC-12:00). merge me maybe?

tiran · 2018-01-29T21:37:52Z

Yeah, I'm pressing the big green button now...

bedevere-bot · 2018-01-29T21:38:00Z

@tiran: Please replace # with GH- in the commit message next time. Thanks!

1st1 · 2018-01-29T21:41:13Z

You need to add versionchanged: 3.7 tag.

1st1 · 2018-01-29T21:43:52Z

What happens if SO_PROTOCOL isn't defined?

the-knights-who-say-ni added the CLA signed label Apr 28, 2017

tiran commented Apr 28, 2017

View reviewed changes

tiran mentioned this pull request Apr 28, 2017

bpo-27377: Add socket.fdtype() #1348

Closed

tiran force-pushed the bpo-28134-socket-fd branch 2 times, most recently from 67264b2 to a3650c7 Compare April 28, 2017 19:50

tiran force-pushed the bpo-28134-socket-fd branch from a3650c7 to c634ce9 Compare April 28, 2017 22:18

tiran force-pushed the bpo-28134-socket-fd branch 4 times, most recently from 0be56ed to c375e06 Compare April 29, 2017 20:25

brettcannon added the type-feature A feature request or enhancement label May 1, 2017

tiran force-pushed the bpo-28134-socket-fd branch from c375e06 to cc4cb87 Compare July 20, 2017 16:03

tiran force-pushed the bpo-28134-socket-fd branch from cc4cb87 to bf90728 Compare September 5, 2017 17:12

tiran assigned gpshead Sep 5, 2017

gpshead reviewed Sep 5, 2017

View reviewed changes

gpshead approved these changes Sep 6, 2017

View reviewed changes

bedevere-bot added the awaiting merge label Sep 6, 2017

gpshead approved these changes Sep 6, 2017

View reviewed changes

tiran force-pushed the bpo-28134-socket-fd branch 2 times, most recently from 533c3e6 to 883e44a Compare December 24, 2017 19:01

tiran force-pushed the bpo-28134-socket-fd branch from 883e44a to e5ff880 Compare January 14, 2018 15:08

tiran force-pushed the bpo-28134-socket-fd branch from e5ff880 to 28b925f Compare January 14, 2018 15:44

tiran merged commit b6e43af into python:master Jan 29, 2018

bedevere-bot removed the awaiting merge label Jan 29, 2018

njsmith mentioned this pull request Aug 31, 2018

On Linux, set up socket object correctly when creating from an fd python-trio/trio#251

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-28134: Auto-detect socket values from file descriptor #1349

bpo-28134: Auto-detect socket values from file descriptor #1349

tiran commented Apr 28, 2017 •

edited by bedevere-bot

Loading

tiran Apr 28, 2017

vadmium Apr 29, 2017

gpshead Sep 5, 2017

nascheme commented Apr 28, 2017

tiran commented Apr 29, 2017

gpshead Sep 5, 2017

gpshead Sep 5, 2017

gpshead Sep 5, 2017

gpshead commented Dec 22, 2017

nascheme commented Dec 23, 2017

tiran commented Dec 24, 2017

tiran commented Jan 16, 2018

gpshead commented Jan 29, 2018

tiran commented Jan 29, 2018

bedevere-bot commented Jan 29, 2018

1st1 commented Jan 29, 2018

1st1 commented Jan 29, 2018

bpo-28134: Auto-detect socket values from file descriptor #1349

bpo-28134: Auto-detect socket values from file descriptor #1349

Conversation

tiran commented Apr 28, 2017 • edited by bedevere-bot Loading

tiran Apr 28, 2017

Choose a reason for hiding this comment

vadmium Apr 29, 2017

Choose a reason for hiding this comment

gpshead Sep 5, 2017

Choose a reason for hiding this comment

nascheme commented Apr 28, 2017

tiran commented Apr 29, 2017

gpshead Sep 5, 2017

Choose a reason for hiding this comment

gpshead Sep 5, 2017

Choose a reason for hiding this comment

gpshead Sep 5, 2017

Choose a reason for hiding this comment

gpshead commented Dec 22, 2017

nascheme commented Dec 23, 2017

tiran commented Dec 24, 2017

tiran commented Jan 16, 2018

gpshead commented Jan 29, 2018

tiran commented Jan 29, 2018

bedevere-bot commented Jan 29, 2018

1st1 commented Jan 29, 2018

1st1 commented Jan 29, 2018

tiran commented Apr 28, 2017 •

edited by bedevere-bot

Loading