Merge pull request #1635 from gevent/issue1634

Pass addresses through inet_pton before getaddrinfo

appveyor.yml

@@ -4,9 +4,13 @@ shallow_clone: true
 build:
   parallel: true
   verbosity: minimal
+# The VS 2019 image doesn't have
+# the MSVC needed for Python 2.7.
+image: Visual Studio 2015
 
 environment:
   global:
+    APPVEYOR_SAVE_CACHE_ON_ERROR: "true"
     # SDK v7.0 MSVC Express 2008's SetEnv.cmd script will fail if the
     # /E:ON and /V:ON options are not enabled in the batch script interpreter
     # See: http://stackoverflow.com/a/13751649/163740
@@ -125,7 +129,7 @@ install:
   - ps: "ls \"C:/\""
 
   - ECHO "Installed SDKs:"
-  - ps: "ls \"C:/Program Files/Microsoft SDKs/Windows\""
+  - ps: "if(Test-Path(\"C:/Program Files/Microsoft SDKs/Windows\")) {ls \"C:/Program Files/Microsoft SDKs/Windows\";}"
 
   # Install Python (from the official .msi of http://python.org) and pip when
   # not already installed.
@@ -167,7 +171,7 @@ install:
 
 cache:
   - "%TMP%\\py\\"
-  - '%LOCALAPPDATA%\pip\Cache'
+  - '%LOCALAPPDATA%\pip\Cache -> appveyor.yml,setup.py'
 
 build_script:
   # Build the compiled extension

docs/changes/1634.bugfix

+``gevent.socket.create_connection`` and
+``gevent.socket.socket.connect`` no longer ignore IPv6 scope IDs.
+
+Any IP address (IPv4 or IPv6) is no longer subject to an extra call to
+``getaddrinfo``. Depending on the resolver in use, this is likely to
+change the number and order of greenlet switches. (On Windows, in
+particular test cases when there are no other greenlets running, it has
+been observed to lead to ``LoopExit`` in scenarios that didn't produce
+that before.)

src/gevent/_socket2.py

@@ -251,6 +251,14 @@ class socket(_socketcommon.SocketMixin):
         return isinstance(self._sock, _closedsocket)
 
     def connect(self, address):
+        """
+        Connect to *address*.
+
+        .. versionchanged:: NEXT
+            If the host part of the address includes an IPv6 scope ID,
+            it will be used instead of ignored, if the platform supplies
+            :func:`socket.inet_pton`.
+        """
         if self.timeout == 0.0:
             return self._sock.connect(address)
 

src/gevent/_socket3.py

@@ -389,10 +389,17 @@ class socket(_socketcommon.SocketMixin):
         return sock.detach()
 
     def connect(self, address):
+        """
+        Connect to *address*.
+
+        .. versionchanged:: NEXT
+            If the host part of the address includes an IPv6 scope ID,
+            it will be used instead of ignored, if the platform supplies
+            :func:`socket.inet_pton`.
+        """
         if self.timeout == 0.0:
             return _socket.socket.connect(self._sock, address)
         address = _socketcommon._resolve_addr(self._sock, address)
-
         with Timeout._start_new_or_dummy(self.timeout, timeout("timed out")):
             while True:
                 err = self.getsockopt(SOL_SOCKET, SO_ERROR)

src/gevent/_socketcommon.py

@@ -398,6 +398,20 @@ def _resolve_addr(sock, address):
     if sock.family not in _RESOLVABLE_FAMILIES or not isinstance(address, tuple):
         return address
     # address is (host, port) (ipv4) or (host, port, flowinfo, scopeid) (ipv6).
+    # If it's already resolved, no need to go through getaddrinfo() again.
+    # That can lose precision (e.g., on IPv6, it can lose scopeid). The standard library
+    # does this in socketmodule.c:setipaddr. (This is only part of the logic, the real
+    # thing is much more complex.)
+    try:
+        if __socket__.inet_pton(sock.family, address[0]):
+            return address
+    except AttributeError: # pragma: no cover
+        # inet_pton might not be available.
+        pass
+    except __socket__.error:
+        # Not parseable, needs resolved.
+        pass
+
 
     # We don't pass the port to getaddrinfo because the C
     # socket module doesn't either (on some systems its

src/gevent/_ssl3.py

@@ -806,5 +806,5 @@ def get_server_certificate(addr, ssl_version=PROTOCOL_SSLv23, ca_certs=None):
         with wrap_socket(sock, ssl_version=ssl_version,
                          cert_reqs=cert_reqs, ca_certs=ca_certs) as sslsock:
             dercert = sslsock.getpeercert(True)
-
+    sslsock = sock = None
     return DER_cert_to_PEM_cert(dercert)

src/gevent/socket.py

@@ -75,6 +75,11 @@ def create_connection(address, timeout=_GLOBAL_DEFAULT_TIMEOUT, source_address=N
     must be a tuple of (host, port) for the socket to bind as a source
     address before making the connection. A host of '' or port 0 tells
     the OS to use the default.
+
+    .. versionchanged:: NEXT
+        If the host part of the address includes an IPv6 scope ID,
+        it will be used instead of ignored, if the platform supplies
+        :func:`socket.inet_pton`.
     """
 
     host, port = address
@@ -85,7 +90,7 @@ def create_connection(address, timeout=_GLOBAL_DEFAULT_TIMEOUT, source_address=N
         raise error("getaddrinfo returns an empty list")
 
     for res in addrs:
-        af, socktype, proto, _, sa = res
+        af, socktype, proto, _canonname, sa = res
         sock = None
         try:
             sock = socket(af, socktype, proto)

src/gevent/testing/patched_tests_setup.py

@@ -703,6 +703,8 @@ if WIN:
     disabled_tests += [
         # Issue with Unix vs DOS newlines in the file vs from the server
         'test_ssl.ThreadedTests.test_socketserver',
+        # This sometimes hangs (only on appveyor)
+        'test_ssl.ThreadedTests.test_asyncore_server',
         # On appveyor, this sometimes produces 'A non-blocking socket
         # operation could not be completed immediately', followed by
         # 'No connection could be made because the target machine

src/gevent/testing/sysinfo.py

@@ -17,7 +17,7 @@
 # LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
 # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
 # THE SOFTWARE.
-
+import errno
 import os
 import sys
 
@@ -125,7 +125,9 @@ if RUNNING_ON_APPVEYOR:
 
 EXPECT_POOR_TIMER_RESOLUTION = (
     PYPY3
-    or RUNNING_ON_APPVEYOR
+    # Really, this is probably only in VMs. But that's all I test
+    # Windows with.
+    or WIN
     or (LIBUV and PYPY)
     or RUN_COVERAGE
     or (OSX and RUNNING_ON_CI)
@@ -133,16 +135,19 @@ EXPECT_POOR_TIMER_RESOLUTION = (
 
 
 CONN_ABORTED_ERRORS = []
-try:
-    from errno import WSAECONNABORTED
-    CONN_ABORTED_ERRORS.append(WSAECONNABORTED)
-except ImportError:
-    pass
-
-from errno import ECONNRESET
-CONN_ABORTED_ERRORS.append(ECONNRESET)
-
-CONN_ABORTED_ERRORS = frozenset(CONN_ABORTED_ERRORS)
+def _make_socket_errnos(*names):
+    result = []
+    for name in names:
+        try:
+            x = getattr(errno, name)
+        except AttributeError:
+            pass
+        else:
+            result.append(x)
+    return frozenset(result)
+
+CONN_ABORTED_ERRORS = _make_socket_errnos('WSAECONNABORTED', 'ECONNRESET')
+CONN_REFUSED_ERRORS = _make_socket_errnos('WSAECONNREFUSED', 'ECONNREFUSED')
 
 RESOLVER_ARES = os.getenv('GEVENT_RESOLVER') == 'ares'
 RESOLVER_DNSPYTHON = os.getenv('GEVENT_RESOLVER') == 'dnspython'

src/gevent/tests/test__server.py

@@ -8,10 +8,14 @@ import os
 import gevent.testing as greentest
 from gevent.testing import PY3
 from gevent.testing import DEFAULT_SOCKET_TIMEOUT as _DEFAULT_SOCKET_TIMEOUT
+from gevent.testing.timing import SMALLEST_RELIABLE_DELAY
 from gevent.testing.sockets import tcp_listener
+from gevent.testing import WIN
+
 from gevent import socket
 import gevent
 from gevent.server import StreamServer
+from gevent.exceptions import LoopExit
 
 
 class SimpleStreamServer(StreamServer):
@@ -40,6 +44,17 @@ class SimpleStreamServer(StreamServer):
         finally:
             fd.close()
 
+def sleep_to_clear_old_sockets(*_args):
+    try:
+        # Allow any queued callbacks needed to close sockets
+        # to run. On Windows, this needs to spin the event loop to
+        # allow proper FD cleanup. Otherwise we risk getting an
+        # old FD that's being closed and then get spurious connection
+        # errors.
+        gevent.sleep(0 if not WIN else SMALLEST_RELIABLE_DELAY)
+    except Exception: # pylint:disable=broad-except
+        pass
+
 
 class Settings(object):
     ServerClass = StreamServer
@@ -50,7 +65,10 @@ class Settings(object):
     @staticmethod
     def assertAcceptedConnectionError(inst):
         with inst.makefile() as conn:
-            result = conn.read()
+            try:
+                result = conn.read()
+            except socket.timeout:
+                result = None
         inst.assertFalse(result)
 
     assert500 = assertAcceptedConnectionError
@@ -86,6 +104,7 @@ class TestCase(greentest.TestCase):
         if getattr(self, 'server', None) is not None:
             self.server.stop()
             self.server = None
+        sleep_to_clear_old_sockets()
 
     def get_listener(self):
         return self._close_on_teardown(tcp_listener(backlog=5))
@@ -139,10 +158,21 @@ class TestCase(greentest.TestCase):
         # A kernel bug in OS X sometimes results in this
         LOCAL_CONN_REFUSED_ERRORS = (errno.EPROTOTYPE,)
 
-    def assertConnectionRefused(self):
-        with self.assertRaises(socket.error) as exc:
-            with self.makefile() as conn:
-                conn.close()
+    def assertConnectionRefused(self, in_proc_server=True):
+        try:
+            with self.assertRaises(socket.error) as exc:
+                with self.makefile() as conn:
+                    conn.close()
+        except LoopExit:
+            if not in_proc_server:
+                raise
+            # A LoopExit is fine. If we've killed the server
+            # and don't have any other greenlets to run, then
+            # blocking to open the connection might raise this.
+            # This became likely on Windows once we stopped
+            # passing IP addresses through an extra call to
+            # ``getaddrinfo``, which changed the number of switches
+            return
 
         ex = exc.exception
         self.assertIn(ex.args[0],
@@ -163,19 +193,23 @@ class TestCase(greentest.TestCase):
         self.Settings.assertPoolFull(self)
 
     def assertNotAccepted(self):
-        with self.makefile(include_raw_socket=True) as (conn, sock):
-            conn.write(b'GET / HTTP/1.0\r\n\r\n')
-            conn.flush()
-            result = b''
-            try:
-                while True:
-                    data = sock.recv(1)
-                    if not data:
-                        break
-                    result += data
-            except socket.timeout:
-                self.assertFalse(result)
-                return
+        try:
+            with self.makefile(include_raw_socket=True) as (conn, sock):
+                conn.write(b'GET / HTTP/1.0\r\n\r\n')
+                conn.flush()
+                result = b''
+                try:
+                    while True:
+                        data = sock.recv(1)
+                        if not data:
+                            break
+                        result += data
+                except socket.timeout:
+                    self.assertFalse(result)
+                    return
+        except LoopExit:
+            # See assertConnectionRefused
+            return
 
         self.assertTrue(result.startswith(b'HTTP/1.0 500 Internal Server Error'), repr(result))
 
@@ -207,7 +241,7 @@ class TestCase(greentest.TestCase):
     def init_server(self):
         self.server = self._create_server()
         self.server.start()
-        gevent.sleep()
+        sleep_to_clear_old_sockets()
 
     @property
     def socket(self):
@@ -258,6 +292,7 @@ class TestDefaultSpawn(TestCase):
             self.assertNotAccepted()
             self.server.start_accepting()
             self.report_netstat('after start_accepting')
+            sleep_to_clear_old_sockets()
             self.assertRequestSucceeded()
         self.stop_server()
         self.report_netstat('after stop')
@@ -276,6 +311,7 @@ class TestDefaultSpawn(TestCase):
         self.server = self.ServerSubClass(self.get_listener())
         self.assertNotAccepted()
 
+    @greentest.skipOnAppVeyor("Sometimes times out.")
     def test_subclass_with_socket(self):
         self.server = self.ServerSubClass(self.get_listener())
         # the connection won't be refused, because there exists a
@@ -295,7 +331,7 @@ class TestDefaultSpawn(TestCase):
     def _test_serve_forever(self):
         g = gevent.spawn(self.server.serve_forever)
         try:
-            gevent.sleep(0.01)
+            sleep_to_clear_old_sockets()
             self.assertRequestSucceeded()
             self.server.stop()
             self.assertFalse(self.server.started)
@@ -319,9 +355,11 @@ class TestDefaultSpawn(TestCase):
         self.assertTrue(self.server.started)
         self._test_serve_forever()
 
+    @greentest.skipIf(greentest.EXPECT_POOR_TIMER_RESOLUTION, "Sometimes spuriously fails")
     def test_server_closes_client_sockets(self):
         self.server = self.ServerClass((greentest.DEFAULT_BIND_ADDR, 0), lambda *args: [])
         self.server.start()
+        sleep_to_clear_old_sockets()
         with self.makefile() as conn:
             self.send_request_to_fd(conn)
             # use assert500 below?
@@ -330,6 +368,8 @@ class TestDefaultSpawn(TestCase):
                     result = conn.read()
                     if result:
                         assert result.startswith('HTTP/1.0 500 Internal Server Error'), repr(result)
+                except socket.timeout:
+                    pass
                 except socket.error as ex:
                     if ex.args[0] == 10053:
                         pass  # "established connection was aborted by the software in your host machine"
@@ -348,7 +388,9 @@ class TestDefaultSpawn(TestCase):
         self.init_server()
         self.assertTrue(self.server.started)
         error = ExpectedError('test_error_in_spawn')
-        self.server._spawn = lambda *args: gevent.getcurrent().throw(error)
+        def _spawn(*_args):
+            gevent.getcurrent().throw(error)
+        self.server._spawn = _spawn
         self.expect_one_error()
         self.assertAcceptedConnectionError()
         self.assert_error(ExpectedError, error)
@@ -427,7 +469,7 @@ class TestNoneSpawn(TestCase):
 
     def test_assertion_in_blocking_func(self):
         def sleep(*_args):
-            gevent.sleep(0)
+            gevent.sleep(SMALLEST_RELIABLE_DELAY)
         self.server = self.Settings.ServerClass((greentest.DEFAULT_BIND_ADDR, 0), sleep, spawn=None)
         self.server.start()
         self.expect_one_error()

src/gevent/tests/test__socket.py

@@ -562,6 +562,19 @@ class TestFunctions(greentest.TestCase):
             exclude.append('gethostbyaddr')
         self.assertMonkeyPatchedFuncSignatures('socket', exclude=exclude)
 
+    def test_resolve_ipv6_scope_id(self):
+        from gevent import _socketcommon as SC
+        if not SC.__socket__.has_ipv6:
+            self.skipTest("Needs IPv6") # pragma: no cover
+        if not hasattr(SC.__socket__, 'inet_pton'):
+            self.skipTest("Needs inet_pton") # pragma: no cover
+
+        # A valid IPv6 address, with a scope.
+        addr = ('2607:f8b0:4000:80e::200e', 80, 0, 9)
+        # Mock socket
+        class sock(object):
+            family = SC.AF_INET6 # pylint:disable=no-member
+        self.assertIs(addr, SC._resolve_addr(sock, addr))
 
 class TestSocket(greentest.TestCase):
 

src/gevent/tests/test__socket_errors.py

@@ -21,12 +21,10 @@
 
 import gevent.testing as greentest
 from gevent.testing import support
-from gevent.socket import socket, error
+from gevent.testing import sysinfo
 
-try:
-    from errno import WSAECONNREFUSED as ECONNREFUSED
-except ImportError:
-    from errno import ECONNREFUSED
+from gevent.socket import socket, error
+from gevent.exceptions import LoopExit
 
 
 class TestSocketErrors(greentest.TestCase):
@@ -35,15 +33,15 @@ class TestSocketErrors(greentest.TestCase):
 
     def test_connection_refused(self):
         port = support.find_unused_port()
-        s = socket()
-        self._close_on_teardown(s)
-        try:
-            s.connect((greentest.DEFAULT_CONNECT_HOST, port))
-        except error as ex:
-            self.assertEqual(ex.args[0], ECONNREFUSED, ex)
-            self.assertIn('refused', str(ex).lower())
-        else:
-            self.fail("Shouldn't have connected")
+        with socket() as s:
+            try:
+                with self.assertRaises(error) as exc:
+                    s.connect((greentest.DEFAULT_CONNECT_HOST, port))
+            except LoopExit:
+                return
+        ex = exc.exception
+        self.assertIn(ex.args[0], sysinfo.CONN_REFUSED_ERRORS, ex)
+        self.assertIn('refused', str(ex).lower())
 
 
 if __name__ == '__main__':