diff --git a/newsfragments/247.bugfix.rst b/newsfragments/247.bugfix.rst new file mode 100644 index 0000000000..62613b8b3e --- /dev/null +++ b/newsfragments/247.bugfix.rst @@ -0,0 +1 @@ +Make trio.socket._SocketType.connect *always* close the socket on cancellation diff --git a/trio/_socket.py b/trio/_socket.py index eba29366b3..dc46faa452 100644 --- a/trio/_socket.py +++ b/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 diff --git a/trio/tests/test_socket.py b/trio/tests/test_socket.py index fdcb8dfa6b..1a3559aa46 100644 --- a/trio/tests/test_socket.py +++ b/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: