launchpad-reviewers team mailing list archive

[Julian Edwards <julian.edwards@xxxxxxxxxxxxx>]

Julian Edwards has proposed merging lp:~julian-edwards/maas/cobbler-timeouts into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~julian-edwards/maas/cobbler-timeouts/+merge/90402

Mostly self explanatory except for a couple of things:

 * AsynchronousDeferredRunTestForBrokenTwisted is needed when you have leftover unfired Deferreds, it cleans them up.
 * RecordingSession is fixed so you can actually pass in a proxy to use like its docstring said you could :)
-- 
https://code.launchpad.net/~julian-edwards/maas/cobbler-timeouts/+merge/90402
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~julian-edwards/maas/cobbler-timeouts into lp:maas.

=== modified file 'src/provisioningserver/cobblerclient.py'
--- src/provisioningserver/cobblerclient.py	2012-01-26 22:33:02 +0000
+++ src/provisioningserver/cobblerclient.py	2012-01-27 12:48:09 +0000
@@ -23,6 +23,7 @@
 
 import xmlrpclib
 
+from twisted.internet import reactor as default_reactor
 from twisted.internet.defer import (
     DeferredLock,
     inlineCallbacks,
@@ -123,6 +124,24 @@
         else:
             return arg
 
+    def _with_timeout(self, d, timeout=30, reactor=None):
+        """Wrap the xmlrpc call that returns "d" so that it is cancelled if
+        it exceeds a timeout.
+
+        :param d: The Deferred to cancel.
+        :param timeout: timeout in seconds, defaults to 30.
+        :param reactor: override the default reactor, useful for testing.
+        """
+        if reactor is None:
+            reactor = default_reactor
+        delayed_call = reactor.callLater(timeout, d.cancel)
+
+        def cancel_timeout(passthrough):
+            if not delayed_call.called:
+                delayed_call.cancel()
+            return passthrough
+        return d.addBoth(cancel_timeout)
+
     def _issue_call(self, method, *args):
         """Initiate call to XMLRPC method.
 
@@ -133,7 +152,7 @@
         :return: `Deferred`.
         """
         args = map(self.substitute_token, args)
-        d = self.proxy.callRemote(method, *args)
+        d = self._with_timeout(self.proxy.callRemote(method, *args))
         return d
 
     @inlineCallbacks

=== modified file 'src/provisioningserver/tests/test_cobblersession.py'
--- src/provisioningserver/tests/test_cobblersession.py	2012-01-26 22:33:02 +0000
+++ src/provisioningserver/tests/test_cobblersession.py	2012-01-27 12:48:09 +0000
@@ -11,21 +11,29 @@
 __metaclass__ = type
 __all__ = []
 
+import fixtures
+
 from random import Random
 from xmlrpclib import Fault
 
 from provisioningserver import cobblerclient
 from provisioningserver.testing.fakecobbler import fake_token
 from testtools.content import text_content
-from testtools.deferredruntest import AsynchronousDeferredRunTest
+from testtools.deferredruntest import (
+    assert_fails_with,
+    AsynchronousDeferredRunTest,
+    AsynchronousDeferredRunTestForBrokenTwisted,
+    )
 from testtools.testcase import (
     ExpectedException,
     TestCase,
     )
+from twisted.internet import defer
 from twisted.internet.defer import (
     inlineCallbacks,
     returnValue,
     )
+from twisted.internet.task import Clock
 
 
 randomizer = Random()
@@ -72,10 +80,9 @@
         """
         self.return_values = values
 
-    @inlineCallbacks
     def callRemote(self, method, *args):
         if method == 'login':
-            returnValue(self.fake_token)
+            return defer.succeed(self.fake_token)
 
         self.calls.append((method, ) + args)
         if self.return_values:
@@ -85,8 +92,14 @@
         if isinstance(value, Exception):
             raise value
         else:
-            value = yield value
-            returnValue(value)
+            return defer.succeed(value)
+
+
+class DeadProxy(RecordingFakeProxy):
+    """Fake proxy that returns nothing. Useful for timeout testing."""
+
+    def callRemote(self, method, *args):
+        return defer.Deferred()
 
 
 class RecordingSession(cobblerclient.CobblerSession):
@@ -104,17 +117,19 @@
         that the session should pretend it gets from the server on login.
         """
         fake_token = kwargs.pop('fake_token')
-        self.fake_proxy = RecordingFakeProxy(fake_token)
+        proxy_class = kwargs.pop('fake_proxy', RecordingFakeProxy)
+        if proxy_class is None:
+            proxy_class = RecordingFakeProxy
+
+        self.fake_proxy = proxy_class(fake_token)
         super(RecordingSession, self).__init__(*args, **kwargs)
 
     def _make_twisted_proxy(self):
         return self.fake_proxy
 
 
-class TestCobblerSession(TestCase):
-    """Test session management against a fake XMLRPC session."""
-
-    run_tests_with = AsynchronousDeferredRunTest.make_factory()
+class TestCobblerSessionBase(TestCase):
+    """Base class helpers for testing `CobblerSession`."""
 
     def make_url_user_password(self):
         """Produce arbitrary API URL, username, and password."""
@@ -124,13 +139,21 @@
             'password%d' % pick_number(),
             )
 
-    def make_recording_session(self, session_args=None, token=None):
+    def make_recording_session(self, session_args=None, token=None,
+                               fake_proxy=RecordingFakeProxy):
         """Create a `RecordingSession`."""
         if session_args is None:
             session_args = self.make_url_user_password()
         if token is None:
             token = fake_token()
-        return RecordingSession(*session_args, fake_token=token)
+        return RecordingSession(
+            *session_args, fake_token=token, fake_proxy=fake_proxy)
+
+
+class TestCobblerSession(TestCobblerSessionBase):
+    """Test session management against a fake XMLRPC session."""
+
+    run_tests_with = AsynchronousDeferredRunTest.make_factory()
 
     def test_initializes_but_does_not_authenticate_on_creation(self):
         url, user, password = self.make_url_user_password()
@@ -307,6 +330,41 @@
             [('authenticate_me_first', session.token)], session.proxy.calls)
 
 
+class TestConnectionTimeouts(TestCobblerSessionBase,
+                             fixtures.TestWithFixtures):
+    """Tests for connection timeouts on `CobblerSession`."""
+
+    run_tests_with =  AsynchronousDeferredRunTestForBrokenTwisted
+
+    def test__with_timeout_cancels(self):
+        clock = Clock()
+        session = self.make_recording_session()
+        d = session._with_timeout(defer.Deferred(), 1, clock)
+        clock.advance(2)
+        return assert_fails_with(d, defer.CancelledError)
+
+    def test__with_timeout_not_cancelled(self):
+        clock = Clock()
+        session = self.make_recording_session()
+        d = session._with_timeout(defer.succeed("frobnicle"), 1, clock)
+        clock.advance(2)
+        def result(value):
+            self.assertEqual(value, "frobnicle")
+            self.assertEqual([], clock.getDelayedCalls())
+        return d.addCallback(result)
+
+    def test__issue_call_times_out(self):
+        clock = Clock()
+        patch = fixtures.MonkeyPatch(
+            "provisioningserver.cobblerclient.default_reactor", clock)
+        self.useFixture(patch)
+
+        session = self.make_recording_session(fake_proxy=DeadProxy)
+        d = session._issue_call("login", "foo")
+        clock.advance(31)
+        return assert_fails_with(d, defer.CancelledError)
+
+
 class CobblerObject(TestCase):
     """Tests for the `CobblerObject` classes."""
 
@@ -319,3 +377,4 @@
         self.assertEqual(
             'x_systems_y',
             cobblerclient.CobblerSystem.name_method('x_%s_y', plural=True))
+