fix(core): ensure timeout argument is positive (#534)

Previously, a gap between calls to `time.time()` could lead to a
situation where the current time was less than `end` during the
`while` condition, but it was greater than `end` when assigning
a value to `timeout_at`.

Add tests to ensure a socket.error is raised instead of passing a
nonpositive value as a timeout to socket.create_connection.

Author: cdonati

kazoo/handlers/utils.py

@@ -198,8 +198,13 @@ def create_tcp_connection(module, address, timeout=None,
         end = time.time() + timeout
     sock = None
 
-    while end is None or time.time() < end:
+    while True:
         timeout_at = end if end is None else end - time.time()
+        # The condition is not '< 0' here because socket.settimeout treats 0 as
+        # a special case to put the socket in non-blocking mode.
+        if timeout_at is not None and timeout_at <= 0:
+            break
+
         if use_ssl:
             # Disallow use of SSLv2 and V3 (meaning we require TLSv1.0+)
             context = ssl.SSLContext(ssl.PROTOCOL_SSLv23)

kazoo/tests/test_utils.py

@@ -0,0 +1,78 @@
+import unittest
+
+from mock import patch
+from nose import SkipTest
+
+try:
+    from kazoo.handlers.eventlet import green_socket as socket
+    EVENTLET_HANDLER_AVAILABLE = True
+except ImportError:
+    EVENTLET_HANDLER_AVAILABLE = False
+
+
+class TestCreateTCPConnection(unittest.TestCase):
+    def test_timeout_arg(self):
+        from kazoo.handlers import utils
+        from kazoo.handlers.utils import create_tcp_connection, socket, time
+
+        with patch.object(socket, 'create_connection') as create_connection:
+            with patch.object(utils, '_set_default_tcpsock_options'):
+                # Ensure a gap between calls to time.time() does not result in
+                # create_connection being called with a negative timeout
+                # argument.
+                with patch.object(time, 'time', side_effect=range(10)):
+                    create_tcp_connection(socket, ('127.0.0.1', 2181),
+                                          timeout=1.5)
+
+                for call_args in create_connection.call_args_list:
+                    timeout = call_args[0][1]
+                    assert timeout >= 0, 'socket timeout must be nonnegative'
+
+    def test_timeout_arg_eventlet(self):
+        if not EVENTLET_HANDLER_AVAILABLE:
+            raise SkipTest('eventlet handler not available.')
+
+        from kazoo.handlers import utils
+        from kazoo.handlers.utils import create_tcp_connection, time
+
+        with patch.object(socket, 'create_connection') as create_connection:
+            with patch.object(utils, '_set_default_tcpsock_options'):
+                # Ensure a gap between calls to time.time() does not result in
+                # create_connection being called with a negative timeout
+                # argument.
+                with patch.object(time, 'time', side_effect=range(10)):
+                    create_tcp_connection(socket, ('127.0.0.1', 2181),
+                                          timeout=1.5)
+
+                for call_args in create_connection.call_args_list:
+                    timeout = call_args[0][1]
+                    assert timeout >= 0, 'socket timeout must be nonnegative'
+
+    def test_slow_connect(self):
+        # Currently, create_tcp_connection will raise a socket timeout if it
+        # takes longer than the specified "timeout" to create a connection.
+        # In the future, "timeout" might affect only the created socket and not
+        # the time it takes to create it.
+        from kazoo.handlers.utils import create_tcp_connection, socket, time
+
+        # Simulate a second passing between calls to check the current time.
+        with patch.object(time, 'time', side_effect=range(10)):
+            with self.assertRaises(socket.error):
+                create_tcp_connection(socket, ('127.0.0.1', 2181), timeout=0.5)
+
+    def test_negative_timeout(self):
+        from kazoo.handlers.utils import create_tcp_connection, socket
+        with self.assertRaises(socket.error):
+            create_tcp_connection(socket, ('127.0.0.1', 2181), timeout=-1)
+
+    def test_zero_timeout(self):
+        # Rather than pass '0' through as a timeout to
+        # socket.create_connection, create_tcp_connection should raise
+        # socket.error. This is because the socket library treats '0' as an
+        # indicator to create a non-blocking socket.
+        from kazoo.handlers.utils import create_tcp_connection, socket, time
+
+        # Simulate no time passing between calls to check the current time.
+        with patch.object(time, 'time', return_value=time.time()):
+            with self.assertRaises(socket.error):
+                create_tcp_connection(socket, ('127.0.0.1', 2181), timeout=0)