Merge lp:~rvb/maas/retry-bug-1398082 into lp:~maas-committers/maas/trunk

[Raphaël Badin]

Fix the retry code so that it copes with API views: for these views the request is the second argument passed to the view and not the first as with UI views.

extract_request() is very conservative (i.e. it doesn't return the first http.HttpRequest object it finds in the arguments) because the nature and the ordering of the arguments passed to views is very well defined so I figured it would be much better to be on the safe side and raise assertion errors if we ever encounter a case we didn't plan for.

[Graham Binns (gmb)]

LGTM. Nice to have your Django-wisdom back :)

review: Approve

=== modified file 'src/maasserver/utils/tests/test_views.py'
--- src/maasserver/utils/tests/test_views.py	2014-11-12 23:32:25 +0000
+++ src/maasserver/utils/tests/test_views.py	2014-12-02 10:25:00 +0000
@@ -205,6 +205,20 @@
 
         self.assertThat(response, Is(sentinel.response))
 
+    def test__retries_API_calls(self):
+
+        def api_view(handler, request, attempt=count(1)):
+            if next(attempt) == 3:
+                return sentinel.response
+            else:
+                request_transaction_retry()
+
+        retry_view = views.RetryView.make(api_view, 4)
+
+        response = retry_view(factory.make_name(), HttpRequest())
+
+        self.assertThat(response, Is(sentinel.response))
+
     def test__make_does_not_double_wrap(self):
         view = lambda: None
         retry_view = views.RetryView.make(view)

=== modified file 'src/maasserver/utils/views.py'
--- src/maasserver/utils/views.py	2014-11-12 23:32:03 +0000
+++ src/maasserver/utils/views.py	2014-12-02 10:25:00 +0000
@@ -20,6 +20,7 @@
 from inspect import getmodule
 from textwrap import dedent
 
+from django import http
 from django.conf.urls import url
 from django.db import transaction
 from maasserver.utils.orm import is_serialization_failure
@@ -89,7 +90,30 @@
             }
         )
 
-    def __call__(self, request, *args, **kwargs):
+    def extract_request(self, *args):
+        """Extract the request object from the arguments passed to a view.
+
+        The position of the request in the list of arguments is different
+        depending on the nature of the view: if the view is a UI view it's
+        the first argument and if the view is a piston-based API view, it's
+        the second argument.
+        """
+        if len(args) < 1:
+            assert False, (
+                "Couldn't find request in arguments (no argument).")
+        elif isinstance(args[0], http.HttpRequest):
+            # This is a UI view, the request is the first argument.
+            return args[0]
+        if len(args) < 2:
+            assert False, (
+                "Couldn't find request in arguments (no second argument).")
+        elif isinstance(args[1], http.HttpRequest):
+            # This is an API view, the request is the second argument (the
+            # handler is the first argument).
+            return args[1]
+        assert False, "Couldn't find request in arguments."
+
+    def __call__(self, *args, **kwargs):
         """Invoke the wrapped view with retry logic.
 
         :returns: The response from `self.atomic_view`, or, if the number of
@@ -97,9 +121,10 @@
         """
         for attempt in xrange(1, self.retries + 1):
             try:
-                return self.atomic_view(request, *args, **kwargs)
+                return self.atomic_view(*args, **kwargs)
             except Exception as e:
                 if is_serialization_failure(e):
+                    request = self.extract_request(*args)
                     log_retry(request, attempt)
                     reset_request(request)
                     continue
@@ -107,7 +132,7 @@
                     raise
         else:
             # Last chance, unadorned by retry logic.
-            return self.atomic_view(request, *args, **kwargs)
+            return self.atomic_view(*args, **kwargs)
 
 
 def retry_url(regex, view, kwargs=None, name=None, prefix=''):