Code review comment for lp:~leonardr/lazr.restfulclient/retry_on_502

Hi Leonard,

> === modified file 'src/lazr/restfulclient/_browser.py'
> --- src/lazr/restfulclient/_browser.py 2010-04-12 19:18:31 +0000
> +++ src/lazr/restfulclient/_browser.py 2010-04-27 19:59:13 +0000

> @@ -242,6 +246,21 @@
> self._connection = service_root.httpFactory(
> credentials, cache, timeout, proxy_info)
> self.user_agent = user_agent
> + self.max_retries = max_retries
> +
> + def _request_and_retry(self, url, method, body, headers):
> + for retry_count in range(0, self.max_retries+1):
> + response, content = self._connection.request(
> + url, method=method, body=body, headers=headers)
> + if response.status in [502, 503]:
> + # The server returned a 502 or 503. Sleep for 0, 1, 2,
> + # 4, 8, 16, ... seconds and try again.
> + sleep_for = int(2**(retry_count-1))
> + sleep(sleep_for)

Even after the terminal failure you sleep again. I guess it doesn't
matter but due to the exponential wait times you may have someone
waiting for a long time even though you've given up.

> + else:
> + break
> + # Either the request succeeded or we gave up.
> + return response, content
>
> def _request(self, url, data=None, method='GET',
> media_type='application/json', extra_headers=None):
> @@ -261,7 +280,7 @@
> if extra_headers is not None:
> headers.update(extra_headers)
> # Make the request.
> - response, content = self._connection.request(
> + response, content = self._request_and_retry(
> str(url), method=method, body=data, headers=headers)
> if response.status == 304:
> # The resource didn't change.

> === added file 'src/lazr/restfulclient/docs/retry.standalone.txt'
> --- src/lazr/restfulclient/docs/retry.standalone.txt 1970-01-01 00:00:00 +0000
> +++ src/lazr/restfulclient/docs/retry.standalone.txt 2010-04-27 19:59:13 +0000
> @@ -0,0 +1,136 @@
> +Retry requests on server error
> +******************************
> +
> +If lazr.restfulclient talks to a server that sends out a server-side
> +error with status codes 502 or 503, the client will wait a few seconds
> +and try the request again. Eventually it will give up and escalate the
> +error code in the form of an exception.
> +
> +To test this, let's simulate a lazr.restful server prone to transient
> +errors using a WSGI application.
> +
> + >>> import pkg_resources
> + >>> wadl_string = pkg_resources.resource_string(
> + ... 'wadllib.tests.data', 'launchpad-wadl.xml')
> + >>> representations = { 'application/vnd.sun.wadl+xml' : wadl_string,
> + ... 'application/json' : '{}' }
> +
> +This application will cause one request to fail for every item in its
> +BROKEN_RESPONSES list.
> +
> + >>> class BrokenApplication:
> + ... BROKEN_RESPONSES = []
> + ...
> + ... def __init__(self, environ, start_response):
> + ... self.environ = environ
> + ... self.start_response = start_response
> + ...
> + ... def __iter__(self):
> + ... if len(self.BROKEN_RESPONSES) > 0:
> + ... start_response(self.BROKEN_RESPONSES.pop(),
> + ... [('Content-type', 'text/plain')])
> + ... yield "Sorry, I'm still broken."
> + ... else:
> + ... media_type = self.environ['HTTP_ACCEPT']
> + ... content = representations[media_type]
> + ... self.start_response(
> + ... '200', [('Content-type', media_type)])
> + ... yield content

On IRC we agreed BrokenApplication may not be required now.

> + >>> BROKEN_RESPONSES = []
> + >>> def broken_application(environ, start_response):
> + ... if len(BROKEN_RESPONSES) > 0: