Merge pull request #1045 from gevent/1044

Fix #1044 by always closing opened sockets before raising

CHANGES.rst

@@ -90,6 +90,12 @@
 - ``socket.send()`` now catches ``EPROTYPE`` on macOS to handle a race
   condition during shutdown. Fixed in :pr:`1035` by Jay Oster.
 
+- :func:`gevent.socket.create_connection` now properly cleans up open
+  sockets if connecting or binding raises a :exc:`BaseException` like
+  :exc:`KeyboardInterrupt`, :exc:`greenlet.GreenletExit` or
+  :exc:`gevent.timeout.Timeout`. Reported in :issue:`1044` by
+  kochelmonster.
+
 - Update c-ares to 1.13.0. See :issue:`990`.
 
 1.2.2 (2017-06-05)

dev-requirements.txt

 setuptools
 wheel
-cython>=0.27
+cython>=0.27.3
 greenlet>=0.4.10
 pylint>=1.7.1
 prospector[with_pyroma]

src/gevent/monkey.py

@@ -716,8 +716,13 @@ def main():
 
 
 def _get_script_help():
-    from inspect import getargspec
-    patch_all_args = getargspec(patch_all)[0] # pylint:disable=deprecated-method
+    # pylint:disable=deprecated-method
+    import inspect
+    try:
+        getter = inspect.getfullargspec # deprecated in 3.5, un-deprecated in 3.6
+    except AttributeError:
+        getter = inspect.getargspec
+    patch_all_args = getter(patch_all)[0]
     modules = [x for x in patch_all_args if 'patch_' + x in globals()]
     script_help = """gevent.monkey - monkey patch the standard modules to use gevent.
 

src/gevent/socket.py

@@ -73,8 +73,13 @@ def create_connection(address, timeout=_GLOBAL_DEFAULT_TIMEOUT, source_address=N
     """
 
     host, port = address
-    err = None
-    for res in getaddrinfo(host, port, 0 if has_ipv6 else AF_INET, SOCK_STREAM):
+    # getaddrinfo is documented as returning a list, but our interface
+    # is pluggable, so be sure it does.
+    addrs = list(getaddrinfo(host, port, 0 if has_ipv6 else AF_INET, SOCK_STREAM))
+    if not addrs:
+        raise error("getaddrinfo returns an empty list")
+
+    for res in addrs:
         af, socktype, proto, _, sa = res
         sock = None
         try:
@@ -84,24 +89,36 @@ def create_connection(address, timeout=_GLOBAL_DEFAULT_TIMEOUT, source_address=N
             if source_address:
                 sock.bind(source_address)
             sock.connect(sa)
-            return sock
-        except error as ex:
-            # without exc_clear(), if connect() fails once, the socket is referenced by the frame in exc_info
-            # and the next bind() fails (see test__socket.TestCreateConnection)
-            # that does not happen with regular sockets though, because _socket.socket.connect() is a built-in.
-            # this is similar to "getnameinfo loses a reference" failure in test_socket.py
-            if not PY3:
-                sys.exc_clear() # pylint:disable=no-member,useless-suppression
+        except error:
             if sock is not None:
                 sock.close()
-            err = ex
-    if err is not None:
-        try:
-            raise err # pylint:disable=raising-bad-type
-        finally:
-            err = None
-    else:
-        raise error("getaddrinfo returns an empty list")
+            sock = None
+            if res is addrs[-1]:
+                raise
+            # without exc_clear(), if connect() fails once, the socket
+            # is referenced by the frame in exc_info and the next
+            # bind() fails (see test__socket.TestCreateConnection)
+            # that does not happen with regular sockets though,
+            # because _socket.socket.connect() is a built-in. this is
+            # similar to "getnameinfo loses a reference" failure in
+            # test_socket.py
+            try:
+                c = sys.exc_clear
+            except AttributeError:
+                pass # Python 3 doesn't have this
+            else:
+                c()
+        except BaseException:
+            # Things like GreenletExit,  Timeout and KeyboardInterrupt.
+            # These get raised immediately, being sure to
+            # close the socket
+            if sock is not None:
+                sock.close()
+            sock = None
+            raise
+        else:
+            return sock
+
 
 # This is promised to be in the __all__ of the _source, but, for circularity reasons,
 # we implement it in this module. Mostly for documentation purposes, put it

src/greentest/greentest.py

@@ -403,6 +403,7 @@ class TestCase(TestCaseMetaClass("NewBase", (BaseTestCase,), {})):
         if hasattr(self, 'cleanup'):
             self.cleanup()
         self._error = self._none
+        # XXX: Should probably reverse this
         for x in self.close_on_teardown:
             close = getattr(x, 'close', x)
             try:
@@ -430,6 +431,10 @@ class TestCase(TestCaseMetaClass("NewBase", (BaseTestCase,), {})):
         super(TestCase, cls).tearDownClass()
 
     def _close_on_teardown(self, resource):
+        """
+        *resource* either has a ``close`` method, or is a
+        callable.
+        """
         if 'close_on_teardown' not in self.__dict__:
             self.close_on_teardown = []
         self.close_on_teardown.append(resource)

src/greentest/test___monkey_patching.py

@@ -47,17 +47,18 @@ def TESTRUNNER(tests=None):
     if tests:
         atexit.register(os.system, 'rm -f */@test*')
 
+    basic_args = [sys.executable, '-u', '-W', 'ignore', '-m' 'monkey_test']
     for filename in tests:
         if filename in version_tests:
             util.log("Overriding %s from %s with file from %s", filename, directory, full_directory)
             continue
-        yield [sys.executable, '-u', '-m', 'monkey_test', filename], options.copy()
-        yield [sys.executable, '-u', '-m', 'monkey_test', '--Event', filename], options.copy()
+        yield basic_args + [filename], options.copy()
+        yield basic_args + ['--Event', filename], options.copy()
 
     options['cwd'] = full_directory
     for filename in version_tests:
-        yield [sys.executable, '-u', '-m', 'monkey_test', filename], options.copy()
-        yield [sys.executable, '-u', '-m', 'monkey_test', '--Event', filename], options.copy()
+        yield basic_args + [filename], options.copy()
+        yield basic_args + ['--Event', filename], options.copy()
 
 
 def main():

src/greentest/test__example_udp_client.py

@@ -17,7 +17,8 @@ class Test_udp_client(TestCase):
         server = DatagramServer('127.0.0.1:9000', handle)
         server.start()
         try:
-            run([sys.executable, '-u', 'udp_client.py', 'Test_udp_client'], timeout=10, cwd='../../examples/')
+            run([sys.executable, '-W', 'ignore' '-u', 'udp_client.py', 'Test_udp_client'],
+                timeout=10, cwd='../../examples/')
         finally:
             server.close()
         self.assertEqual(log, [b'Test_udp_client'])

src/greentest/test__greenio.py

@@ -18,6 +18,7 @@
 # THE SOFTWARE.
 
 from greentest import TestCase, main, tcp_listener
+from greentest import skipOnPyPy
 import gevent
 from gevent import socket
 import sys
@@ -87,17 +88,19 @@ class TestGreenIo(TestCase):
         did_it_work(server)
         server_greenlet.kill()
 
+    @skipOnPyPy("GC is different")
     def test_del_closes_socket(self):
-        if PYPY:
-            return
         timer = gevent.Timeout.start_new(0.5)
 
         def accept_once(listener):
             # delete/overwrite the original conn
             # object, only keeping the file object around
             # closing the file object should close everything
+
+            # XXX: This is not exactly true on Python 3.
+            # This produces a ResourceWarning.
             try:
-                conn, addr = listener.accept()
+                conn, _ = listener.accept()
                 conn = conn.makefile(mode='wb')
                 conn.write(b'hello\n')
                 conn.close()

src/greentest/test__hub.py

@@ -32,14 +32,13 @@ DELAY = 0.1
 class TestCloseSocketWhilePolling(greentest.TestCase):
 
     def test(self):
-        try:
+        with self.assertRaises(Exception):
             sock = socket.socket()
+            self._close_on_teardown(sock)
             get_hub().loop.timer(0, sock.close)
             sock.connect(('python.org', 81))
-        except Exception:
-            gevent.sleep(0)
-        else:
-            assert False, 'expected an error here'
+
+        gevent.sleep(0)
 
 
 class TestExceptionInMainloop(greentest.TestCase):

src/greentest/test__makefile_ref.py

 from __future__ import print_function
 import os
 from gevent import monkey; monkey.patch_all()
-import re
 import socket
 import ssl
 import threading
 import unittest
 import errno
 
+from greentest import TestCase
+
 dirname = os.path.dirname(os.path.abspath(__file__))
 certfile = os.path.join(dirname, '2.7/keycert.pem')
 pid = os.getpid()
@@ -27,13 +28,13 @@ except ImportError:
     psutil = None
 
 
-class Test(unittest.TestCase):
+class Test(TestCase):
 
     extra_allowed_open_states = ()
 
     def tearDown(self):
         self.extra_allowed_open_states = ()
-        unittest.TestCase.tearDown(self)
+        super(Test, self).tearDown()
 
     def assert_raises_EBADF(self, func):
         try:
@@ -156,6 +157,7 @@ class TestSocket(Test):
         listener.listen(1)
 
         connector = socket.socket()
+        self._close_on_teardown(connector)
 
         def connect():
             connector.connect(('127.0.0.1', port))
@@ -180,6 +182,7 @@ class TestSocket(Test):
         listener.listen(1)
 
         connector = socket.socket()
+        self._close_on_teardown(connector)
 
         def connect():
             connector.connect(('127.0.0.1', port))
@@ -213,6 +216,7 @@ class TestSocket(Test):
         listener.listen(1)
 
         connector = socket.socket()
+        self._close_on_teardown(connector)
 
         def connect():
             connector.connect(('127.0.0.1', port))
@@ -282,10 +286,12 @@ class TestSSL(Test):
         listener.listen(1)
 
         connector = socket.socket()
+        self._close_on_teardown(connector)
 
         def connect():
             connector.connect(('127.0.0.1', port))
-            ssl.wrap_socket(connector)
+            x = ssl.wrap_socket(connector)
+            self._close_on_teardown(x)
 
         t = threading.Thread(target=connect)
         t.start()
@@ -303,15 +309,18 @@ class TestSSL(Test):
 
     def test_server_makefile1(self):
         listener = socket.socket()
+        self._close_on_teardown(listener)
         listener.bind(('127.0.0.1', 0))
         port = listener.getsockname()[1]
         listener.listen(1)
 
         connector = socket.socket()
+        self._close_on_teardown(connector)
 
         def connect():
             connector.connect(('127.0.0.1', port))
-            ssl.wrap_socket(connector)
+            x = ssl.wrap_socket(connector)
+            self._close_on_teardown(x)
 
         t = threading.Thread(target=connect)
         t.start()
@@ -338,10 +347,12 @@ class TestSSL(Test):
         listener.listen(1)
 
         connector = socket.socket()
+        self._close_on_teardown(connector)
 
         def connect():
             connector.connect(('127.0.0.1', port))
-            ssl.wrap_socket(connector)
+            x = ssl.wrap_socket(connector)
+            self._close_on_teardown(x)
 
         t = threading.Thread(target=connect)
         t.start()
@@ -372,10 +383,12 @@ class TestSSL(Test):
         listener = ssl.wrap_socket(listener, keyfile=certfile, certfile=certfile)
 
         connector = socket.socket()
+        self._close_on_teardown(connector)
 
         def connect():
             connector.connect(('127.0.0.1', port))
-            ssl.wrap_socket(connector)
+            x = ssl.wrap_socket(connector)
+            self._close_on_teardown(x)
 
         t = threading.Thread(target=connect)
         t.start()

src/greentest/test__refcount.py

@@ -70,7 +70,7 @@ def init_server():
 
 def handle_request(s, raise_on_timeout):
     try:
-        conn, address = s.accept()
+        conn, _ = s.accept()
     except socket.timeout:
         if raise_on_timeout:
             raise
@@ -83,7 +83,7 @@ def handle_request(s, raise_on_timeout):
     res = conn.send(b'bye')
     #print('handle_request - sent %r' % res)
     #print('handle_request - conn refcount: %s' % sys.getrefcount(conn))
-    #conn.close()
+    conn.close()
 
 
 def make_request(port):
@@ -96,7 +96,7 @@ def make_request(port):
     res = s.recv(100)
     assert res == b'bye', repr(res)
     #print('make_request - recvd %r' % res)
-    #s.close()
+    s.close()
 
 
 def run_interaction(run_client):

src/greentest/test__refcount_core.py

@@ -9,8 +9,11 @@ try:
     assert weakref.ref(Dummy())() is None
 
     from gevent import socket
-
-    assert weakref.ref(socket.socket())() is None
+    s = socket.socket()
+    r = weakref.ref(s)
+    s.close()
+    del s
+    assert r() is None
 except AssertionError:
     import sys
     if hasattr(sys, 'pypy_version_info'):

src/greentest/test__server.py

@@ -383,6 +383,7 @@ class TestPoolSpawn(TestDefaultSpawn):
         return 2
 
     @greentest.skipOnAppVeyor("Connection timeouts are flaky")
+    @greentest.skipOnPyPy3OnCI("Doesn't always raise timeout for some reason")
     def test_pool_full(self):
         self.init_server()
         short_request = self.send_request('/short')

src/greentest/test__socket.py

@@ -324,17 +324,58 @@ class TestCreateConnection(greentest.TestCase):
 
     __timeout__ = 5
 
-    def test(self):
-        try:
+    def test_refuses(self):
+        with self.assertRaises(socket.error) as cm:
             socket.create_connection((greentest.DEFAULT_BIND_ADDR, get_port()),
                                      timeout=30,
                                      source_address=('', get_port()))
-        except socket.error as ex:
-            if 'refused' not in str(ex).lower():
-                raise
-        else:
-            raise AssertionError('create_connection did not raise socket.error as expected')
+        ex = cm.exception
+        self.assertIn('refused', str(ex).lower())
+
+    @greentest.ignores_leakcheck
+    def test_base_exception(self):
+        # such as a GreenletExit or a gevent.timeout.Timeout
+
+        class E(BaseException):
+            pass
+
+        class MockSocket(object):
+
+            created = ()
+            closed = False
+
+            def __init__(self, *_):
+                MockSocket.created += (self,)
+
+            def connect(self, _):
+                raise E()
 
+            def close(self):
+                self.closed = True
+
+        def mockgetaddrinfo(*_):
+            return [(1, 2, 3, 3, 5),]
+
+        import gevent.socket as gsocket
+        # Make sure we're monkey patched
+        self.assertEqual(gsocket.create_connection, socket.create_connection)
+        orig_socket = gsocket.socket
+        orig_getaddrinfo = gsocket.getaddrinfo
+
+        try:
+            gsocket.socket = MockSocket
+            gsocket.getaddrinfo = mockgetaddrinfo
+
+            with self.assertRaises(E):
+                socket.create_connection(('host', 'port'))
+
+            self.assertEqual(1, len(MockSocket.created))
+            self.assertTrue(MockSocket.created[0].closed)
+
+        finally:
+            MockSocket.created = ()
+            gsocket.socket = orig_socket
+            gsocket.getaddrinfo = orig_getaddrinfo
 
 class TestFunctions(greentest.TestCase):
 

src/greentest/test__socket_dns.py

@@ -363,11 +363,14 @@ class TestLocalhost(TestCase):
                 result = (result[0], result[1], ['127.0.0.1'])
             return result
 
-add(TestLocalhost, 'localhost')
+
 if not greentest.RUNNING_ON_TRAVIS:
     # ares fails here, for some reason, presumably a badly
     # configured /etc/hosts
     add(TestLocalhost, 'ip6-localhost')
+    # Beginning Dec 1 2017, ares started returning ip6-localhost
+    # instead of localhost
+    add(TestLocalhost, 'localhost')
 
 
 class TestNonexistent(TestCase):
@@ -385,7 +388,10 @@ add(Test1234, '1.2.3.4')
 class Test127001(TestCase):
     pass
 
-add(Test127001, '127.0.0.1')
+if not greentest.RUNNING_ON_TRAVIS:
+    # Beginning Dec 1 2017, ares started returning ip6-localhost
+    # instead of localhost
+    add(Test127001, '127.0.0.1')
 
 
 class TestBroadcast(TestCase):
@@ -593,6 +599,7 @@ class TestBadIP(TestCase):
 add(TestBadIP, '1.2.3.400')
 
 
+@greentest.skipIf(greentest.RUNNING_ON_TRAVIS, "Travis began returning ip6-localhost")
 class Test_getnameinfo_127001(TestCase):
 
     def test(self):

src/greentest/test__socket_errors.py

@@ -34,6 +34,7 @@ class TestSocketErrors(greentest.TestCase):
 
     def test_connection_refused(self):
         s = socket()
+        self._close_on_teardown(s)
         try:
             s.connect(('127.0.0.1', 81))
         except error as ex:

src/greentest/test__socket_send_memoryview.py

@@ -12,12 +12,13 @@ def _send(socket):
         sock = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
         sock.connect(('127.0.0.1', 12345))
         getattr(sock, meth)(anStructure)
+        sock.close()
 
         sock = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
         sock.connect(('127.0.0.1', 12345))
         sock.settimeout(1.0)
         getattr(sock, meth)(anStructure)
-
+        sock.close()
 
 def TestSendBuiltinSocket():
     import socket

src/greentest/test__socket_timeout.py

@@ -5,36 +5,49 @@ import greentest
 
 class Test(greentest.TestCase):
 
-    def start(self):
+    server = None
+    acceptor = None
+    server_port = None
+
+    def _accept(self):
+        self.server.listen(1)
+        try:
+            conn, _ = self.server.accept()
+            self._close_on_teardown(conn)
+        except socket.error:
+            pass
+
+    def setUp(self):
+        super(Test, self).setUp()
         self.server = socket.socket()
+        self._close_on_teardown(self.server)
         self.server.bind(('127.0.0.1', 0))
-        self.server.listen(1)
         self.server_port = self.server.getsockname()[1]
-        self.acceptor = gevent.spawn(self.server.accept)
+        self.acceptor = gevent.spawn(self._accept)
+        gevent.sleep(0)
 
-    def stop(self):
-        self.server.close()
-        self.acceptor.kill()
-        del self.acceptor
-        del self.server
+    def tearDown(self):
+        if self.acceptor is not None:
+            self.acceptor.kill()
+            self.acceptor = None
+        if self.server is not None:
+            self.server.close()
+            self.server = None
+        super(Test, self).tearDown()
 
-    def test(self):
-        self.start()
-        try:
-            sock = socket.socket()
-            sock.connect(('127.0.0.1', self.server_port))
-            try:
-                sock.settimeout(0.1)
-                try:
-                    result = sock.recv(1024)
-                    raise AssertionError('Expected timeout to be raised, instead recv() returned %r' % (result, ))
-                except socket.error as ex:
-                    self.assertEqual(ex.args, ('timed out',))
-                    self.assertEqual(str(ex), 'timed out')
-            finally:
-                sock.close()
-        finally:
-            self.stop()
+    def test_timeout(self):
+        gevent.sleep(0)
+        sock = socket.socket()
+        self._close_on_teardown(sock)
+        sock.connect(('127.0.0.1', self.server_port))
+
+        sock.settimeout(0.1)
+        with self.assertRaises(socket.error) as cm:
+            sock.recv(1024)
+
+        ex = cm.exception
+        self.assertEqual(ex.args, ('timed out',))
+        self.assertEqual(str(ex), 'timed out')
 
 
 if __name__ == '__main__':

src/greentest/test__subprocess.py

@@ -71,7 +71,8 @@ class Test(greentest.TestCase):
 
 
     def test_communicate(self):
-        p = subprocess.Popen([sys.executable, "-c",
+        p = subprocess.Popen([sys.executable, "-W", "ignore",
+                              "-c",
                               'import sys,os;'
                               'sys.stderr.write("pineapple");'
                               'sys.stdout.write(sys.stdin.read())'],
@@ -91,7 +92,9 @@ class Test(greentest.TestCase):
         # Native string all the things. See https://github.com/gevent/gevent/issues/1039
         p = subprocess.Popen(
             [
-                sys.executable, "-c",
+                sys.executable,
+                "-W", "ignore",
+                "-c",
                 'import sys,os;'
                 'sys.stderr.write("pineapple\\r\\n\\xff\\xff\\xf2\\xf9\\r\\n");'
                 'sys.stdout.write(sys.stdin.read())'

src/greentest/test_threading_2.py

@@ -307,7 +307,7 @@ class ThreadTests(unittest.TestCase):
         del ctypes  # pyflakes fix
 
         import subprocess
-        rc = subprocess.call([sys.executable, "-c", """if 1:
+        rc = subprocess.call([sys.executable, "-W", "ignore", "-c", """if 1:
 %s
             import ctypes, sys, time
             try:
@@ -343,7 +343,7 @@ class ThreadTests(unittest.TestCase):
         # Issue 1722344
         # Raising SystemExit skipped threading._shutdown
         import subprocess
-        p = subprocess.Popen([sys.executable, "-c", """if 1:
+        p = subprocess.Popen([sys.executable, "-W", "ignore", "-c", """if 1:
 %s
                 import threading
                 from time import sleep
@@ -432,7 +432,7 @@ class ThreadJoinOnShutdown(unittest.TestCase):
         \n""" % setup_3 + script
 
         import subprocess
-        p = subprocess.Popen([sys.executable, "-c", script], stdout=subprocess.PIPE)
+        p = subprocess.Popen([sys.executable, "-W", "ignore", "-c", script], stdout=subprocess.PIPE)
         rc = p.wait()
         data = p.stdout.read().replace(b'\r', b'')
         self.assertEqual(data, b"end of main\nend of thread\n")

src/greentest/testrunner.py

@@ -297,6 +297,16 @@ def main():
             config_data = f.read()
         six.exec_(config_data, config)
         FAILING_TESTS = config['FAILING_TESTS']
+
+    if 'PYTHONWARNINGS' not in os.environ and not sys.warnoptions:
+        # Enable default warnings such as ResourceWarning.
+        # On Python 3[.6], the system site.py module has
+        # "open(fullname, 'rU')" which produces the warning that
+        # 'U' is deprecated, so ignore warnings from site.py
+        os.environ['PYTHONWARNINGS'] = 'default,ignore:::site:'
+    if 'PYTHONFAULTHANDLER' not in os.environ:
+        os.environ['PYTHONFAULTHANDLER'] = 'true'
+
     tests = discover(options.tests, options.ignore, coverage)
     if options.discover:
         for cmd, options in tests: