bpo-32331: Fix socket.type when SOCK_NONBLOCK is available (#4877)

7 files changed, 87 insertions, 24 deletions

diff --git a/Doc/library/socket.rst b/Doc/library/socket.rst
index 42fd7ea..db032ca 100644
--- a/Doc/library/socket.rst
+++ b/Doc/library/socket.rst
@@ -482,6 +482,20 @@ The following functions all create :ref:`socket objects <socket-objects>`.
    .. versionchanged:: 3.7
        The CAN_ISOTP protocol was added.
 
+   .. versionchanged:: 3.7
+      When :const:`SOCK_NONBLOCK` or :const:`SOCK_CLOEXEC`
+      bit flags are applied to *type* they are cleared, and
+      :attr:`socket.type` will not reflect them.  They are still passed
+      to the underlying system `socket()` call.  Therefore::
+
+          sock = socket.socket(
+              socket.AF_INET,
+              socket.SOCK_STREAM | socket.SOCK_NONBLOCK)
+
+      will still create a non-blocking socket on OSes that support
+      ``SOCK_NONBLOCK``, but ``sock.type`` will be set to
+      ``socket.SOCK_STREAM``.
+
 .. function:: socketpair([family[, type[, proto]]])
 
    Build a pair of connected socket objects using the given address family, socket
@@ -1417,6 +1431,10 @@ to sockets.
 
    * ``sock.setblocking(False)`` is equivalent to ``sock.settimeout(0.0)``
 
+   .. versionchanged:: 3.7
+      The method no longer applies :const:`SOCK_NONBLOCK` flag on
+      :attr:`socket.type`.
+
 
 .. method:: socket.settimeout(value)
 
@@ -1429,6 +1447,10 @@ to sockets.
 
    For further information, please consult the :ref:`notes on socket timeouts <socket-timeouts>`.
 
+   .. versionchanged:: 3.7
+      The method no longer toggles :const:`SOCK_NONBLOCK` flag on
+      :attr:`socket.type`.
+
 
 .. method:: socket.setsockopt(level, optname, value: int)
 .. method:: socket.setsockopt(level, optname, value: buffer)
diff --git a/Doc/whatsnew/3.7.rst b/Doc/whatsnew/3.7.rst
index 82f7cc0..e5523ff 100644
--- a/Doc/whatsnew/3.7.rst
+++ b/Doc/whatsnew/3.7.rst
@@ -892,6 +892,13 @@ Changes in the Python API
   recent to be more consistent with :mod:`traceback`.
   (Contributed by Jesse Bakker in :issue:`32121`.)
 
+* On OSes that support :const:`socket.SOCK_NONBLOCK` or
+  :const:`socket.SOCK_CLOEXEC` bit flags, the
+  :attr:`socket.type <socket.socket.type>` no longer has them applied.
+  Therefore, checks like ``if sock.type == socket.SOCK_STREAM``
+  work as expected on all platforms.
+  (Contributed by Yury Selivanov in :issue:`32331`.)
+
 .. _Unicode Technical Standard #18: https://unicode.org/reports/tr18/
 
 * On Windows the default for the *close_fds* argument of
diff --git a/Lib/socket.py b/Lib/socket.py
index 1ada24d..2d8aee3 100644
--- a/Lib/socket.py
+++ b/Lib/socket.py
@@ -203,11 +203,7 @@ class socket(_socket.socket):
         For IP sockets, the address info is a pair (hostaddr, port).
         """
         fd, addr = self._accept()
-        # If our type has the SOCK_NONBLOCK flag, we shouldn't pass it onto the
-        # new socket. We do not currently allow passing SOCK_NONBLOCK to
-        # accept4, so the returned socket is always blocking.
-        type = self.type & ~globals().get("SOCK_NONBLOCK", 0)
-        sock = socket(self.family, type, self.proto, fileno=fd)
+        sock = socket(self.family, self.type, self.proto, fileno=fd)
         # Issue #7995: if no default timeout is set and the listening
         # socket had a (non-zero) timeout, force the new socket in blocking
         # mode to override platform-specific socket flags inheritance.
diff --git a/Lib/test/test_asyncore.py b/Lib/test/test_asyncore.py
index ee0c3b3..694ddff 100644
--- a/Lib/test/test_asyncore.py
+++ b/Lib/test/test_asyncore.py
@@ -726,14 +726,10 @@ class BaseTestAPI:
     def test_create_socket(self):
         s = asyncore.dispatcher()
         s.create_socket(self.family)
+        self.assertEqual(s.socket.type, socket.SOCK_STREAM)
         self.assertEqual(s.socket.family, self.family)
-        SOCK_NONBLOCK = getattr(socket, 'SOCK_NONBLOCK', 0)
-        sock_type = socket.SOCK_STREAM | SOCK_NONBLOCK
-        if hasattr(socket, 'SOCK_CLOEXEC'):
-            self.assertIn(s.socket.type,
-                          (sock_type | socket.SOCK_CLOEXEC, sock_type))
-        else:
-            self.assertEqual(s.socket.type, sock_type)
+        self.assertEqual(s.socket.gettimeout(), 0)
+        self.assertFalse(s.socket.get_inheritable())
 
     def test_bind(self):
         if HAS_UNIX_SOCKETS and self.family == socket.AF_UNIX:
diff --git a/Lib/test/test_socket.py b/Lib/test/test_socket.py
index 5b4c5f9..43688ea 100644
--- a/Lib/test/test_socket.py
+++ b/Lib/test/test_socket.py
@@ -1577,6 +1577,22 @@ class GeneralModuleTests(unittest.TestCase):
             self.assertEqual(str(s.family), 'AddressFamily.AF_INET')
             self.assertEqual(str(s.type), 'SocketKind.SOCK_STREAM')
 
+    def test_socket_consistent_sock_type(self):
+        SOCK_NONBLOCK = getattr(socket, 'SOCK_NONBLOCK', 0)
+        SOCK_CLOEXEC = getattr(socket, 'SOCK_CLOEXEC', 0)
+        sock_type = socket.SOCK_STREAM | SOCK_NONBLOCK | SOCK_CLOEXEC
+
+        with socket.socket(socket.AF_INET, sock_type) as s:
+            self.assertEqual(s.type, socket.SOCK_STREAM)
+            s.settimeout(1)
+            self.assertEqual(s.type, socket.SOCK_STREAM)
+            s.settimeout(0)
+            self.assertEqual(s.type, socket.SOCK_STREAM)
+            s.setblocking(True)
+            self.assertEqual(s.type, socket.SOCK_STREAM)
+            s.setblocking(False)
+            self.assertEqual(s.type, socket.SOCK_STREAM)
+
     @unittest.skipIf(os.name == 'nt', 'Will not work on Windows')
     def test_uknown_socket_family_repr(self):
         # Test that when created with a family that's not one of the known
@@ -1589,9 +1605,18 @@ class GeneralModuleTests(unittest.TestCase):
         # On Windows this trick won't work, so the test is skipped.
         fd, path = tempfile.mkstemp()
         self.addCleanup(os.unlink, path)
-        with socket.socket(family=42424, type=13331, fileno=fd) as s:
-            self.assertEqual(s.family, 42424)
-            self.assertEqual(s.type, 13331)
+        unknown_family = max(socket.AddressFamily.__members__.values()) + 1
+
+        unknown_type = max(
+            kind
+            for name, kind in socket.SocketKind.__members__.items()
+            if name not in {'SOCK_NONBLOCK', 'SOCK_CLOEXEC'}
+        ) + 1
+
+        with socket.socket(
+                family=unknown_family, type=unknown_type, fileno=fd) as s:
+            self.assertEqual(s.family, unknown_family)
+            self.assertEqual(s.type, unknown_type)
 
     @unittest.skipUnless(hasattr(os, 'sendfile'), 'test needs os.sendfile()')
     def test__sendfile_use_sendfile(self):
@@ -5084,7 +5109,7 @@ class InheritanceTest(unittest.TestCase):
     def test_SOCK_CLOEXEC(self):
         with socket.socket(socket.AF_INET,
                            socket.SOCK_STREAM | socket.SOCK_CLOEXEC) as s:
-            self.assertTrue(s.type & socket.SOCK_CLOEXEC)
+            self.assertEqual(s.type, socket.SOCK_STREAM)
             self.assertFalse(s.get_inheritable())
 
     def test_default_inheritable(self):
@@ -5149,11 +5174,15 @@ class InheritanceTest(unittest.TestCase):
 class NonblockConstantTest(unittest.TestCase):
     def checkNonblock(self, s, nonblock=True, timeout=0.0):
         if nonblock:
-            self.assertTrue(s.type & socket.SOCK_NONBLOCK)
+            self.assertEqual(s.type, socket.SOCK_STREAM)
             self.assertEqual(s.gettimeout(), timeout)
+            self.assertTrue(
+                fcntl.fcntl(s, fcntl.F_GETFL, os.O_NONBLOCK) & os.O_NONBLOCK)
         else:
-            self.assertFalse(s.type & socket.SOCK_NONBLOCK)
+            self.assertEqual(s.type, socket.SOCK_STREAM)
             self.assertEqual(s.gettimeout(), None)
+            self.assertFalse(
+                fcntl.fcntl(s, fcntl.F_GETFL, os.O_NONBLOCK) & os.O_NONBLOCK)
 
     @support.requires_linux_version(2, 6, 28)
     def test_SOCK_NONBLOCK(self):
diff --git a/Misc/NEWS.d/next/Library/2017-12-15-23-48-43.bpo-32331.fIg1Uc.rst b/Misc/NEWS.d/next/Library/2017-12-15-23-48-43.bpo-32331.fIg1Uc.rst
new file mode 100644
index 0000000..53a262c
--- /dev/null
+++ b/Misc/NEWS.d/next/Library/2017-12-15-23-48-43.bpo-32331.fIg1Uc.rst
@@ -0,0 +1,4 @@
+Fix socket.settimeout() and socket.setblocking() to keep socket.type
+as is. Fix socket.socket() constructor to reset any bit flags applied to
+socket's type.  This change only affects OSes that have SOCK_NONBLOCK
+and/or SOCK_CLOEXEC.
diff --git a/Modules/socketmodule.c b/Modules/socketmodule.c
index 91c879f..d52d9db 100644
--- a/Modules/socketmodule.c
+++ b/Modules/socketmodule.c
@@ -582,12 +582,6 @@ internal_setblocking(PySocketSockObject *s, int block)
     && !((defined(HAVE_SYS_IOCTL_H) && defined(FIONBIO)))
     int delay_flag, new_delay_flag;
 #endif
-#ifdef SOCK_NONBLOCK
-    if (block)
-        s->sock_type &= (~SOCK_NONBLOCK);
-    else
-        s->sock_type |= SOCK_NONBLOCK;
-#endif
 
     Py_BEGIN_ALLOW_THREADS
 #ifndef MS_WINDOWS
@@ -876,7 +870,22 @@ init_sockobject(PySocketSockObject *s,
 {
     s->sock_fd = fd;
     s->sock_family = family;
+
     s->sock_type = type;
+
+    /* It's possible to pass SOCK_NONBLOCK and SOCK_CLOEXEC bit flags
+       on some OSes as part of socket.type.  We want to reset them here,
+       to make socket.type be set to the same value on all platforms.
+       Otherwise, simple code like 'if sock.type == SOCK_STREAM' is
+       not portable.
+    */
+#ifdef SOCK_NONBLOCK
+    s->sock_type = s->sock_type & ~SOCK_NONBLOCK;
+#endif
+#ifdef SOCK_CLOEXEC
+    s->sock_type = s->sock_type & ~SOCK_CLOEXEC;
+#endif
+
     s->sock_proto = proto;
 
     s->errorhandler = &set_error;