Merge pull request #574 from monobot/issue247

fixes #247

newsfragments/247.bugfix.rst

@@ -0,0 +1 @@
+Make trio.socket._SocketType.connect *always* close the socket on cancellation

trio/_socket.py

@@ -654,59 +654,59 @@ async def connect(self, address):
         # off, then the socket becomes writable as a completion
         # notification. This means it isn't really cancellable... we close the
         # socket if cancelled, to avoid confusion.
-        address = await self._resolve_remote_address(address)
-        async with _try_sync():
-            # An interesting puzzle: can a non-blocking connect() return EINTR
-            # (= raise InterruptedError)? PEP 475 specifically left this as
-            # the one place where it lets an InterruptedError escape instead
-            # of automatically retrying. This is based on the idea that EINTR
-            # from connect means that the connection was already started, and
-            # will continue in the background. For a blocking connect, this
-            # sort of makes sense: if it returns EINTR then the connection
-            # attempt is continuing in the background, and on many system you
-            # can't then call connect() again because there is already a
-            # connect happening. See:
-            #
-            #   http://www.madore.org/~david/computers/connect-intr.html
-            #
-            # For a non-blocking connect, it doesn't make as much sense --
-            # surely the interrupt didn't happen after we successfully
-            # initiated the connect and are just waiting for it to complete,
-            # because a non-blocking connect does not wait! And the spec
-            # describes the interaction between EINTR/blocking connect, but
-            # doesn't have anything useful to say about non-blocking connect:
-            #
-            #   http://pubs.opengroup.org/onlinepubs/007904975/functions/connect.html
-            #
-            # So we have a conundrum: if EINTR means that the connect() hasn't
-            # happened (like it does for essentially every other syscall),
-            # then InterruptedError should be caught and retried. If EINTR
-            # means that the connect() has successfully started, then
-            # InterruptedError should be caught and ignored. Which should we
-            # do?
-            #
-            # In practice, the resolution is probably that non-blocking
-            # connect simply never returns EINTR, so the question of how to
-            # handle it is moot.  Someone spelunked MacOS/FreeBSD and
-            # confirmed this is true there:
-            #
-            #   https://stackoverflow.com/questions/14134440/eintr-and-non-blocking-calls
-            #
-            # and exarkun seems to think it's true in general of non-blocking
-            # calls:
-            #
-            #   https://twistedmatrix.com/pipermail/twisted-python/2010-September/022864.html
-            # (and indeed, AFAICT twisted doesn't try to handle
-            # InterruptedError).
-            #
-            # So we don't try to catch InterruptedError. This way if it
-            # happens, someone will hopefully tell us, and then hopefully we
-            # can investigate their system to figure out what its semantics
-            # are.
-            return self._sock.connect(address)
-        # It raised BlockingIOError, meaning that it's started the
-        # connection attempt. We wait for it to complete:
         try:
+            address = await self._resolve_remote_address(address)
+            async with _try_sync():
+                # An interesting puzzle: can a non-blocking connect() return EINTR
+                # (= raise InterruptedError)? PEP 475 specifically left this as
+                # the one place where it lets an InterruptedError escape instead
+                # of automatically retrying. This is based on the idea that EINTR
+                # from connect means that the connection was already started, and
+                # will continue in the background. For a blocking connect, this
+                # sort of makes sense: if it returns EINTR then the connection
+                # attempt is continuing in the background, and on many system you
+                # can't then call connect() again because there is already a
+                # connect happening. See:
+                #
+                #   http://www.madore.org/~david/computers/connect-intr.html
+                #
+                # For a non-blocking connect, it doesn't make as much sense --
+                # surely the interrupt didn't happen after we successfully
+                # initiated the connect and are just waiting for it to complete,
+                # because a non-blocking connect does not wait! And the spec
+                # describes the interaction between EINTR/blocking connect, but
+                # doesn't have anything useful to say about non-blocking connect:
+                #
+                #   http://pubs.opengroup.org/onlinepubs/007904975/functions/connect.html
+                #
+                # So we have a conundrum: if EINTR means that the connect() hasn't
+                # happened (like it does for essentially every other syscall),
+                # then InterruptedError should be caught and retried. If EINTR
+                # means that the connect() has successfully started, then
+                # InterruptedError should be caught and ignored. Which should we
+                # do?
+                #
+                # In practice, the resolution is probably that non-blocking
+                # connect simply never returns EINTR, so the question of how to
+                # handle it is moot.  Someone spelunked MacOS/FreeBSD and
+                # confirmed this is true there:
+                #
+                #   https://stackoverflow.com/questions/14134440/eintr-and-non-blocking-calls
+                #
+                # and exarkun seems to think it's true in general of non-blocking
+                # calls:
+                #
+                #   https://twistedmatrix.com/pipermail/twisted-python/2010-September/022864.html
+                # (and indeed, AFAICT twisted doesn't try to handle
+                # InterruptedError).
+                #
+                # So we don't try to catch InterruptedError. This way if it
+                # happens, someone will hopefully tell us, and then hopefully we
+                # can investigate their system to figure out what its semantics
+                # are.
+                return self._sock.connect(address)
+            # It raised BlockingIOError, meaning that it's started the
+            # connection attempt. We wait for it to complete:
             await _core.wait_socket_writable(self._sock)
         except _core.Cancelled:
             # We can't really cancel a connect, and the socket is in an

trio/tests/test_socket.py

@@ -614,6 +614,22 @@ def connect(self, *args, **kwargs):
                 await sock.connect(("127.0.0.1", 2))
 
 
+async def test_resolve_remote_address_exception_closes_socket():
+    # Here we are testing issue 247, any cancellation will leave the socket closed
+    with _core.open_cancel_scope() as cancel_scope:
+        with tsocket.socket() as sock:
+
+            async def _resolve_remote_address(self, *args, **kwargs):
+                cancel_scope.cancel()
+                await _core.checkpoint()
+
+            sock._resolve_remote_address = _resolve_remote_address
+            with assert_checkpoints():
+                with pytest.raises(_core.Cancelled):
+                    await sock.connect('')
+            assert sock.fileno() == -1
+
+
 async def test_send_recv_variants():
     a, b = tsocket.socketpair()
     with a, b: