Rev 6144: It turns out that if we don't explicitly close the socket, it hangs around somewhere. in http://bazaar.launchpad.net/~jameinel/bzr/drop-idle-connections-824797

[John Arbash Meinel]

At http://bazaar.launchpad.net/~jameinel/bzr/drop-idle-connections-824797

------------------------------------------------------------
revno: 6144
revision-id: john at arbash-meinel.com-20110914125817-h9csfegz6bvz93yj
parent: john at arbash-meinel.com-20110914124017-f9wqua8et07udz61
committer: John Arbash Meinel <john at arbash-meinel.com>
branch nick: drop-idle-connections-824797
timestamp: Wed 2011-09-14 14:58:17 +0200
message:
  It turns out that if we don't explicitly close the socket, it hangs around somewhere.
  
  Which means that the client doesn't *know* that it has been disconnected.
-------------- next part --------------
=== modified file 'bzrlib/errors.py'
--- a/bzrlib/errors.py	2011-09-07 10:17:23 +0000
+++ b/bzrlib/errors.py	2011-09-14 12:58:17 +0000
@@ -1710,6 +1710,11 @@
     _fmt = "Connection closed: %(msg)s %(orig_error)s"
 
 
+class ConnectionTimeout(ConnectionError):
+
+    _fmt = "Connection Timeout: %(msg)s %(orig_error)s"
+
+
 class InvalidRange(TransportError):
 
     _fmt = "Invalid range access in %(path)s at %(offset)s: %(msg)s"

=== modified file 'bzrlib/smart/medium.py'
--- a/bzrlib/smart/medium.py	2011-09-14 12:40:17 +0000
+++ b/bzrlib/smart/medium.py	2011-09-14 12:58:17 +0000
@@ -229,10 +229,32 @@
             while not self.finished:
                 server_protocol = self._build_protocol()
                 self._serve_one_request(server_protocol)
+        except errors.ConnectionTimeout, e:
+            trace.note('%s' % (e,))
+            self._close()
+            raise
         except Exception, e:
             stderr.write("%s terminating on exception %s\n" % (self, e))
             raise
 
+    def _close(self):
+        """Close the current connection. We stopped due to a timeout/etc."""
+        # The default implementation is a no-op, because that is all we used to
+        # do when disconnecting from a client. I suppose we never had the
+        # *server* initiate a disconnect, before
+
+    def _wait_for_bytes_with_timeout(self, timeout_seconds):
+        """Wait for more bytes to be read, but timeout if none available.
+
+        This allows us to detect idle connections, and stop trying to read from
+        them, without setting the socket itself to non-blocking. This also
+        allows us to specify when we watch for idle timeouts.
+
+        :return: Did we timeout? (True if we timed out, False if there is data
+            to be read)
+        """
+        raise NotImplementedError(self._wait_for_bytes_with_timeout)
+
     def _build_protocol(self):
         """Identifies the version of the incoming request, and returns an
         a protocol object that can interpret it.
@@ -248,8 +270,9 @@
             #       connecting for us to give a more useful message. :(
             #       (eg, who is on the other side that we are disconnecting
             #       from)
-            raise errors.BzrError('Timeout after %.1f seconds, disconnecting'
-                                  % (self._stream_medium_timeout))
+            raise errors.ConnectionTimeout(
+                'disconnecting client after %.1f seconds'
+                % (self._stream_medium_timeout))
         bytes = self._get_line()
         protocol_factory, unused_bytes = _get_protocol_factory_for_bytes(bytes)
         protocol = protocol_factory(
@@ -342,6 +365,10 @@
 
         self._push_back(protocol.unused_data)
 
+    def _close(self):
+        """Close the current connection. We stopped due to a timeout/etc."""
+        self.socket.close()
+
     def _wait_for_bytes_with_timeout(self, timeout_seconds):
         """Wait for more bytes to be read, but timeout if none available.
 
@@ -412,6 +439,10 @@
                 return
             protocol.accept_bytes(bytes)
 
+    def _close(self):
+        self._in.close()
+        self._out.close()
+
     def _wait_for_bytes_with_timeout(self, timeout_seconds):
         """Wait for more bytes to be read, but timeout if none available.
 

=== modified file 'bzrlib/tests/test_smart_transport.py'
--- a/bzrlib/tests/test_smart_transport.py	2011-09-14 12:40:17 +0000
+++ b/bzrlib/tests/test_smart_transport.py	2011-09-14 12:58:17 +0000
@@ -972,6 +972,16 @@
         data = server.read_bytes(1)
         self.assertEqual('', data)
 
+    def test_socket_serve_timeout_closes_socket(self):
+        server_sock, client_sock = self.portable_socket_pair()
+        server = medium.SmartServerSocketStreamMedium(
+            server_sock, None)
+        # This should timeout quickly, and then close the connection so that
+        # client_sock recv doesn't block.
+        server._stream_medium_timeout = 0.1
+        self.assertRaises(errors.ConnectionTimeout, server.serve)
+        self.assertEqual('', client_sock.recv(1))
+
     def test_pipe_wait_for_bytes_with_timeout_with_data(self):
         # We intentionally use a real pipe here, so that we can 'select' on it.
         # You can't select() on a StringIO