Merge lp:~rvb/gwacl/retry-feature into lp:gwacl

[Raphaël Badin]

Add retry feature.

This branch adds a "retry feature" to both the management and the storage api. This feature allows a user to configure the api objects in such a way that certain response status codes (typically 409 or 50* — but this is entirely user-configurable) will cause the original request to be retried a certain number of times.

Note that the default behavior (when no retry policy is specified) is unchanged: no retry will occur.

The fact that we have two completely different API objects for the storage side and the management side make this change a bit repetitive but all the logic is encapsulated in the retry policy objects.

This is all really optional so I chose adding a new constructor method (NewManagementAPIWithRetryPolicy) on the management side rather than forcing the user to specify a retry policy when calling NewManagementAPI().

[Julian Edwards (julian-edwards)]

Just starting to review this. Your commit message is terrible :) I'd copy/paste the description into it instead quite frankly, it's much better.

[Julian Edwards (julian-edwards)]

Looks good and is pretty well tested, although I think the "retrier" is not really a retrier!
Please consider my suggestion about it below, although it might be a larger change than you want
to do right now. Otherwise there's a few small things you can change and then land away.

General observation - where you're using time.Millisecond in your tests for the retry
interval, please use time.Nanosecond. No need to keep the tests waiting any longer :)

117 +// It it usually created using RetryPolicy.getRetrier() and then used by

Typo "It it" -> "it is"

117 +// It it usually created using RetryPolicy.getRetrier() and then used by
118 +// performing the request governed by the policy the retrier enforces
119 +// repeatedly until retrier.shouldRetry() returns false.

This sentence is pretty hard to read, I've scanned it several times before working it out.
Let me suggest an alternative:

// retrier is usually created using RetryPolicy.getRetrier(). The retrier enforces the
// policy during repeated requests until retrier.shouldRetry() returns false.

137 + ret.retriesLeft -= 1

Go has ++ and --, might as well use -- here:
    ret.retriesLeft--

I'm not sure it's a good idea to mix the sleep inside this function, it would be better
if it stuck to the single task of making the decision of whether to retry or not and
allowing the callsite to sleep if it wants. As it stands, it's a strange mix of functionality.

It might be an idea to return a time.Delay instead of a bool, which tells the caller how
long to sleep, where zero means "don't retry". You could then fold that into another
receiver method on retrier which will do all of check, wait and retry.

We could define an interface that specs the function out like this so that we can use it in
the storage and the management side:

type RetryableRequest interface {
    func RetryRequest(http.Request) (http.Response, error)
}

and then all the retrying is *completely* encapsulated in the, well, retrier :)

171 +func (*retryPolicySuite) TestIsRetryCodeChecksStatusCode(c *C) {
172 + var testValues = []struct {
173 + retryStatusCodes []int
174 + testStatusCode int
175 + expectedOutcome bool
176 + }{
177 + {[]int{http.StatusConflict, http.StatusInternalServerError}, http.StatusConflict, true},
178 + {[]int{http.StatusConflict}, http.StatusConflict, true},
179 + {[]int{http.StatusConflict}, http.StatusOK, false},
180 + {[]int{}, http.StatusConflict, false},
181 + }
182 + for _, test := range testValues {
183 + policy := RetryPolicy{httpStatusCodes: test.retryStatusCodes}
184 + c.Check(policy.isRetryCode(test.testStatusCode), Equals, test.expectedOutcome)
185 + }

You have a habit of doing these looping tests! I'm not sure this is any more readable than four
simple calls to c.Check. In fact I'm pretty sure it isn't!

In fact there's an argument to break it into four separate tests, whose names tell me exactly what
scenario is being tested.

248 + for {
...
259 + // Check if the context's retry policy commands to retry the request.
260 + if !retrier.shouldRetry(resp.StatusCode) {
261 + bre...

review: Approve

[Raphaël Badin (rvb)]

> type RetryableRequest interface {
> func RetryRequest(http.Request) (http.Response, error)
> }
>

I think this is a great idea. It will be a bit more complicated than what you think because encapsulating the part where the request is being performed means encapsulating a 'http.client' object and we have two different client (the one from the std lib and the forked client) but with another interface, I think this can be done.

[lp:~rvb/gwacl/retry-feature]

219. By Raphaël Badin on 2013-08-09

Move retry in utility method.

[Raphaël Badin (rvb)]

Thanks for the review.

> General observation - where you're using time.Millisecond in your tests for
> the retry
> interval, please use time.Nanosecond. No need to keep the tests waiting any
> longer :)

All right :).

> 117 +// It it usually created using RetryPolicy.getRetrier() and then used
> by
>
> Typo "It it" -> "it is"

Fixed.

> 117 +// It it usually created using RetryPolicy.getRetrier() and then used
> by
> 118 +// performing the request governed by the policy the retrier enforces
> 119 +// repeatedly until retrier.shouldRetry() returns false.
>
> This sentence is pretty hard to read, I've scanned it several times before
> working it out.
> Let me suggest an alternative:
>
> // retrier is usually created using RetryPolicy.getRetrier(). The retrier
> enforces the
> // policy during repeated requests until retrier.shouldRetry() returns false.

I've changed the comment.

> 137 + ret.retriesLeft -= 1
>
> Go has ++ and --, might as well use -- here:
> ret.retriesLeft--

Done.

> I'm not sure it's a good idea to mix the sleep inside this function, it would
> be better
> if it stuck to the single task of making the decision of whether to retry or
> not and
> allowing the callsite to sleep if it wants. As it stands, it's a strange mix
> of functionality.
>
> It might be an idea to return a time.Delay instead of a bool, which tells the
> caller how
> long to sleep, where zero means "don't retry". You could then fold that into
> another
> receiver method on retrier which will do all of check, wait and retry.
>
> We could define an interface that specs the function out like this so that we
> can use it in
> the storage and the management side:
>
> type RetryableRequest interface {
> func RetryRequest(http.Request) (http.Response, error)
> }
>
> and then all the retrying is *completely* encapsulated in the, well, retrier
> :)
>
> 171 +func (*retryPolicySuite) TestIsRetryCodeChecksStatusCode(c *C) {
> 172 + var testValues = []struct {
> 173 + retryStatusCodes []int
> 174 + testStatusCode int
> 175 + expectedOutcome bool
> 176 + }{
> 177 + {[]int{http.StatusConflict, http.StatusInternalServerError},
> http.StatusConflict, true},
> 178 + {[]int{http.StatusConflict}, http.StatusConflict, true},
> 179 + {[]int{http.StatusConflict}, http.StatusOK, false},
> 180 + {[]int{}, http.StatusConflict, false},
> 181 + }
> 182 + for _, test := range testValues {
> 183 + policy := RetryPolicy{httpStatusCodes: test.retryStatusCodes}
> 184 + c.Check(policy.isRetryCode(test.testStatusCode), Equals,
> test.expectedOutcome)
> 185 + }
>
> You have a habit of doing these looping tests! I'm not sure this is any more
> readable than four
> simple calls to c.Check. In fact I'm pretty sure it isn't!

That's right, that construction rarely pays off in practice :/. Fixed

> Again this could be done much sooner inside a smaller loop, I'm not sure of
> the benefit of
> all the other body processing if we are simply going to retry anyway.

All right, I've refactored the code significant...

[lp:~rvb/gwacl/retry-feature]

220. By Raphaël Badin on 2013-08-09

Export variables so this can be used from outside the library.

[Julian Edwards (julian-edwards)]

I can't help feeling that we're missing an interface somewhere that would simplify this considerably, but this is already looking much better than before. I'd comment a bit more in the two retrier objects that they are split because of storage and management needing two different clients.

Land away!

[Raphaël Badin (rvb)]

> I can't help feeling that we're missing an interface somewhere that would
> simplify this considerably, but this is already looking much better than
> before.

Well, the problem is that the two sides don't deal with the same response/request objects. That's why I think it's not really possible to simplify this with a common interface. The only way it could work is if we were to define abstractions over the different request/response objects but the benefit-cost ratio would be rather poor I'm afraid.

=== modified file 'management_base.go'
--- management_base.go	2013-08-06 05:11:29 +0000
+++ management_base.go	2013-08-09 15:00:01 +0000
@@ -32,15 +32,22 @@
 // The default duration after which the polling is terminated.
 const DefaultPollerTimeout = 20 * time.Minute
 
+// NewManagementAPIWithRetryPolicy creates an object used to interact with
+// Windows Azure's API.
+// http://msdn.microsoft.com/en-us/library/windowsazure/ff800682.aspx
+func NewManagementAPIWithRetryPolicy(subscriptionId, certFile, location string, policy RetryPolicy) (*ManagementAPI, error) {
+    session, err := newX509Session(subscriptionId, certFile, location, policy)
+    if err != nil {
+        return nil, err
+    }
+    api := ManagementAPI{session, DefaultPollerInterval, DefaultPollerTimeout}
+    return &api, nil
+}
+
 // NewManagementAPI creates an object used to interact with Windows Azure's API.
 // http://msdn.microsoft.com/en-us/library/windowsazure/ff800682.aspx
 func NewManagementAPI(subscriptionId, certFile, location string) (*ManagementAPI, error) {
-    session, err := newX509Session(subscriptionId, certFile, location)
-    if err != nil {
-        return nil, err
-    }
-    api := ManagementAPI{session, DefaultPollerInterval, DefaultPollerTimeout}
-    return &api, nil
+    return NewManagementAPIWithRetryPolicy(subscriptionId, certFile, location, NoRetryPolicy)
 }
 
 var operationIDHeaderName = http.CanonicalHeaderKey("x-ms-request-id")
@@ -56,6 +63,10 @@
     return "", err
 }
 
+func (api *ManagementAPI) GetRetryPolicy() RetryPolicy {
+    return api.session.retryPolicy
+}
+
 // blockUntilCompleted blocks and polls for completion of an Azure operation.
 // The "response" parameter is the result of the request that started the
 // operation.  If the response says that the operation is running

=== modified file 'management_base_test.go'
--- management_base_test.go	2013-08-06 05:31:49 +0000
+++ management_base_test.go	2013-08-09 15:00:01 +0000
@@ -234,11 +234,28 @@
     c.Assert(err, IsNil)
 
     api, err := NewManagementAPI(subscriptionId, certFile, "West US")
-
-    c.Assert(err, IsNil)
-    session, err := newX509Session(subscriptionId, certFile, "West US")
-    c.Assert(api.session.subscriptionId, DeepEquals, session.subscriptionId)
-    c.Assert(api.session.certFile, DeepEquals, session.certFile)
+    c.Assert(err, IsNil)
+
+    c.Assert(api.session.subscriptionId, DeepEquals, subscriptionId)
+    c.Assert(api.session.certFile, DeepEquals, certFile)
+    c.Assert(api.session.retryPolicy, DeepEquals, NoRetryPolicy)
+}
+
+func (suite *managementBaseAPISuite) TestNewManagementAPIWithRetryPolicy(c *C) {
+    subscriptionId := "subscriptionId"
+    certDir := c.MkDir()
+    certFile := certDir + "/cert.pem"
+    err := ioutil.WriteFile(certFile, []byte(testCert), 0600)
+    c.Assert(err, IsNil)
+    retryPolicy := RetryPolicy{NbRetries: 5, HttpStatusCodes: []int{409}, Delay: time.Minute}
+
+    api, err := NewManagementAPIWithRetryPolicy(subscriptionId, certFile, "West US", retryPolicy)
+    c.Assert(err, IsNil)
+
+    c.Assert(api.session.subscriptionId, DeepEquals, subscriptionId)
+    c.Assert(api.session.certFile, DeepEquals, certFile)
+    c.Assert(api.session.retryPolicy, DeepEquals, retryPolicy)
+    c.Assert(api.GetRetryPolicy(), DeepEquals, retryPolicy)
 }
 
 func (suite *managementBaseAPISuite) TestNewManagementAPISetsDefaultPollerInterval(c *C) {

=== added file 'retry_policy.go'
--- retry_policy.go	1970-01-01 00:00:00 +0000
+++ retry_policy.go	2013-08-09 15:00:01 +0000
@@ -0,0 +1,122 @@
+// Copyright 2013 Canonical Ltd.  This software is licensed under the
+// GNU Lesser General Public License version 3 (see the file COPYING).
+
+package gwacl
+
+import (
+    forkedHttp "launchpad.net/gwacl/fork/http"
+    "net/http"
+    "time"
+)
+
+// A RetryPolicy object encapsulates all the information needed to define how
+// requests should be retried when particular response codes are returned by
+// the Windows Azure server.
+type RetryPolicy struct {
+    // The number of times a request could be retried.  This does not account
+    // for the initial request so a value of 3 means that the request might be
+    // performed 4 times in total.
+    NbRetries int
+    // The HTTP status codes of the response for which the request should be
+    // retried.
+    HttpStatusCodes []int
+    // How long the client should wait between retries.
+    Delay time.Duration
+}
+
+var (
+    NoRetryPolicy = RetryPolicy{NbRetries: 0}
+)
+
+// isRetryCode returns whether or not the given http status code indicates that
+// the request should be retried according to this policy.
+func (policy RetryPolicy) isRetryCode(httpStatusCode int) bool {
+    for _, code := range policy.HttpStatusCodes {
+        if code == httpStatusCode {
+            return true
+        }
+    }
+    return false
+}
+
+func (policy RetryPolicy) getRetryHelper() *retryHelper {
+    return &retryHelper{retriesLeft: policy.NbRetries, policy: &policy}
+}
+
+// A retryHelper is a utility object used to enforce a retry policy when
+// performing requests.
+type retryHelper struct {
+    // The maximum number of retries left to perform.
+    retriesLeft int
+    // The `RetryPolicy` enforced by this retrier.
+    policy *RetryPolicy
+}
+
+// shouldRetry returns whether or not a request governed by the underlying
+// retry policy should be retried.  When it returns 'true', `shouldRetry` also
+// waits for the specified amount of time, as dictated by the retry policy.
+func (ret *retryHelper) shouldRetry(httpStatusCode int) bool {
+    if ret.retriesLeft > 0 && ret.policy.isRetryCode(httpStatusCode) {
+        ret.retriesLeft--
+        return true
+    }
+    return false
+}
+
+// A retrier is a struct used to repeat a request as governed by a retry
+// policy.  retrier is usually created using RetryPolicy.getRetrier().
+type retrier struct {
+    *retryHelper
+
+    // The client used to perform requests.
+    client *http.Client
+}
+
+func (ret *retrier) RetryRequest(request *http.Request) (*http.Response, error) {
+    for {
+        response, err := ret.client.Do(request)
+        if err != nil {
+            return nil, err
+        }
+        if !ret.shouldRetry(response.StatusCode) {
+            return response, nil
+        }
+        time.Sleep(ret.policy.Delay)
+    }
+}
+
+// getRetrier returns a `retrier` object used to enforce the retry policy.
+func (policy RetryPolicy) getRetrier(client *http.Client) *retrier {
+    helper := policy.getRetryHelper()
+    return &retrier{retryHelper: helper, client: client}
+}
+
+// A forkedHttpRetrier is a struct used to repeat a request as governed by a
+// retry policy.  forkedHttpRetrier is usually created using
+// RetryPolicy.getForkedHttpRetrier().  It's the same as the `retrier` struct
+// except it deals with the forked version of the http package.
+type forkedHttpRetrier struct {
+    *retryHelper
+
+    // The client used to perform requests.
+    client *forkedHttp.Client
+}
+
+func (ret *forkedHttpRetrier) RetryRequest(request *forkedHttp.Request) (*forkedHttp.Response, error) {
+    for {
+        response, err := ret.client.Do(request)
+        if err != nil {
+            return nil, err
+        }
+        if !ret.shouldRetry(response.StatusCode) {
+            return response, nil
+        }
+        time.Sleep(ret.policy.Delay)
+    }
+}
+
+// getRetrier returns a `retrier` object used to enforce the retry policy.
+func (policy RetryPolicy) getForkedHttpRetrier(client *forkedHttp.Client) *forkedHttpRetrier {
+    helper := policy.getRetryHelper()
+    return &forkedHttpRetrier{retryHelper: helper, client: client}
+}

=== added file 'retry_policy_test.go'
--- retry_policy_test.go	1970-01-01 00:00:00 +0000
+++ retry_policy_test.go	2013-08-09 15:00:01 +0000
@@ -0,0 +1,150 @@
+// Copyright 2013 Canonical Ltd.  This software is licensed under the
+// GNU Lesser General Public License version 3 (see the file COPYING).
+
+package gwacl
+
+import (
+    "fmt"
+    . "launchpad.net/gocheck"
+    forkedHttp "launchpad.net/gwacl/fork/http"
+    "net/http"
+    "time"
+)
+
+type retryPolicySuite struct{}
+
+var _ = Suite(&retryPolicySuite{})
+
+func (*retryPolicySuite) TestNoRetryPolicyDoesNotRetry(c *C) {
+    c.Check(NoRetryPolicy.NbRetries, Equals, 0)
+}
+
+func (*retryPolicySuite) TestDefaultPolicyIsNoRetryPolicy(c *C) {
+    c.Check(NoRetryPolicy, DeepEquals, RetryPolicy{})
+}
+
+func (*retryPolicySuite) TestIsRetryCodeChecksStatusCode(c *C) {
+    c.Check(
+        RetryPolicy{HttpStatusCodes: []int{http.StatusConflict}}.isRetryCode(http.StatusConflict),
+        Equals, true)
+    c.Check(
+        RetryPolicy{HttpStatusCodes: []int{}}.isRetryCode(http.StatusOK),
+        Equals, false)
+    c.Check(
+        RetryPolicy{HttpStatusCodes: []int{http.StatusConflict}}.isRetryCode(http.StatusOK),
+        Equals, false)
+
+}
+
+func (*retryPolicySuite) TestGetRetryHelperReturnsHelper(c *C) {
+    policy := RetryPolicy{NbRetries: 7, HttpStatusCodes: []int{http.StatusConflict}, Delay: time.Minute}
+    helper := policy.getRetryHelper()
+    c.Check(*helper.policy, DeepEquals, policy)
+    c.Check(helper.retriesLeft, Equals, policy.NbRetries)
+}
+
+type retryHelperSuite struct{}
+
+var _ = Suite(&retryHelperSuite{})
+
+func (*retryHelperSuite) TestShouldRetryExhaustsRetries(c *C) {
+    nbTries := 3
+    policy := RetryPolicy{NbRetries: nbTries, HttpStatusCodes: []int{http.StatusConflict}, Delay: time.Nanosecond}
+    helper := policy.getRetryHelper()
+    retries := []bool{}
+    for i := 0; i < nbTries+1; i++ {
+        retries = append(retries, helper.shouldRetry(http.StatusConflict))
+    }
+    expectedRetries := []bool{true, true, true, false}
+    c.Check(retries, DeepEquals, expectedRetries)
+}
+
+func (*retryHelperSuite) TestShouldRetryReturnsFalseIfCodeNotInHttpStatusCodes(c *C) {
+    policy := RetryPolicy{NbRetries: 10, HttpStatusCodes: []int{http.StatusConflict}, Delay: time.Nanosecond}
+    helper := policy.getRetryHelper()
+    c.Check(helper.shouldRetry(http.StatusOK), Equals, false)
+}
+
+type retrierSuite struct{}
+
+var _ = Suite(&retrierSuite{})
+
+func (*retrierSuite) TestGetRetrier(c *C) {
+    client := &http.Client{}
+    policy := RetryPolicy{NbRetries: 10, HttpStatusCodes: []int{http.StatusConflict}, Delay: time.Nanosecond}
+    retrier := policy.getRetrier(client)
+    c.Check(*retrier.policy, DeepEquals, policy)
+    c.Check(retrier.client, DeepEquals, client)
+}
+
+func (*retrierSuite) TestRetryRequest(c *C) {
+    nbTries := 3
+    transport := &MockingTransport{}
+    client := &http.Client{Transport: transport}
+    for i := 0; i < nbTries; i++ {
+        response := makeHttpResponse(http.StatusConflict, "")
+        transport.AddExchange(response, nil)
+    }
+    response := makeHttpResponse(http.StatusOK, "")
+    transport.AddExchange(response, nil)
+
+    policy := RetryPolicy{NbRetries: nbTries, HttpStatusCodes: []int{http.StatusConflict}, Delay: time.Nanosecond}
+    retrier := policy.getRetrier(client)
+    req, err := http.NewRequest("GET", "http://example.com/", nil)
+    c.Assert(err, IsNil)
+
+    resp, err := retrier.RetryRequest(req)
+    c.Assert(err, IsNil)
+
+    c.Check(resp.StatusCode, Equals, http.StatusOK)
+    c.Check(transport.ExchangeCount, Equals, nbTries+1)
+}
+
+func (*retrierSuite) TestRetryRequestBailsOutWhenError(c *C) {
+    nbTries := 3
+    transport := &MockingTransport{}
+    client := &http.Client{Transport: transport}
+    transport.AddExchange(nil, fmt.Errorf("request error"))
+
+    policy := RetryPolicy{NbRetries: nbTries, HttpStatusCodes: []int{http.StatusConflict}, Delay: time.Nanosecond}
+    retrier := policy.getRetrier(client)
+    req, err := http.NewRequest("GET", "http://example.com/", nil)
+    c.Assert(err, IsNil)
+
+    _, err = retrier.RetryRequest(req)
+    c.Check(err, ErrorMatches, ".*request error.*")
+
+    c.Check(transport.ExchangeCount, Equals, 1)
+}
+
+type forkedHttpRetrierSuite struct{}
+
+var _ = Suite(&forkedHttpRetrierSuite{})
+
+func (*forkedHttpRetrierSuite) TestGetRetrier(c *C) {
+    client := &forkedHttp.Client{}
+    policy := RetryPolicy{NbRetries: 10, HttpStatusCodes: []int{forkedHttp.StatusConflict}, Delay: time.Nanosecond}
+    retrier := policy.getForkedHttpRetrier(client)
+    c.Check(*retrier.policy, DeepEquals, policy)
+    c.Check(retrier.client, DeepEquals, client)
+}
+
+func (*forkedHttpRetrierSuite) TestRetryRequest(c *C) {
+    nbTries := 3
+    nbRequests := nbTries + 1
+    client := &forkedHttp.Client{}
+    httpRequests := make(chan *Request, nbRequests)
+    server := makeRecordingHTTPServer(httpRequests, http.StatusConflict, nil, nil)
+    defer server.Close()
+
+    policy := RetryPolicy{NbRetries: nbTries, HttpStatusCodes: []int{forkedHttp.StatusConflict}, Delay: time.Nanosecond}
+    retrier := policy.getForkedHttpRetrier(client)
+    req, err := forkedHttp.NewRequest("GET", server.URL, nil)
+    c.Assert(err, IsNil)
+
+    resp, err := retrier.RetryRequest(req)
+    c.Assert(err, IsNil)
+
+    c.Check(resp.StatusCode, Equals, forkedHttp.StatusConflict)
+    c.Check(len(httpRequests), Equals, nbTries+1)
+}

=== modified file 'storage_base.go'
--- storage_base.go	2013-08-07 02:13:25 +0000
+++ storage_base.go	2013-08-09 15:00:01 +0000
@@ -214,6 +214,8 @@
     AzureEndpoint APIEndpoint
 
     client *http.Client
+
+    RetryPolicy RetryPolicy
 }
 
 // getClient is used when sending a request. If a custom client is specified
@@ -295,7 +297,9 @@
 // then an HTTPError will be returned as the error.
 func (context *StorageContext) send(req *http.Request, res Deserializer, expectedStatus HTTPStatus) ([]byte, http.Header, error) {
     client := context.getClient()
-    resp, err := client.Do(req)
+
+    retrier := context.RetryPolicy.getRetrier(client)
+    resp, err := retrier.RetryRequest(req)
     if err != nil {
         return nil, nil, err
     }

=== modified file 'storage_base_test.go'
--- storage_base_test.go	2013-08-07 02:13:25 +0000
+++ storage_base_test.go	2013-08-09 15:00:01 +0000
@@ -67,6 +67,28 @@
     c.Check(observed, Equals, expected)
 }
 
+type TestRetryRequests struct{}
+
+var _ = Suite(&TestRetryRequests{})
+
+func (suite *TestRetryRequests) TestRequestIsRetried(c *C) {
+    transport := &MockingTransport{}
+    body := []byte("data")
+    transport.AddExchange(&http.Response{StatusCode: http.StatusConflict, Body: Empty}, nil)
+    transport.AddExchange(&http.Response{StatusCode: http.StatusConflict, Body: Empty}, nil)
+    transport.AddExchange(&http.Response{StatusCode: http.StatusOK, Body: makeResponseBody(string(body))}, nil)
+    retryPolicy := RetryPolicy{NbRetries: 3, HttpStatusCodes: []int{http.StatusConflict}, Delay: time.Nanosecond}
+    context := makeStorageContext(transport)
+    context.RetryPolicy = retryPolicy
+    req, err := http.NewRequest("GET", "http://example.com", nil)
+    c.Assert(err, IsNil)
+
+    resBody, _, err := context.send(req, nil, http.StatusOK)
+    c.Assert(err, IsNil)
+    c.Assert(transport.ExchangeCount, Equals, 3)
+    c.Check(resBody, DeepEquals, body)
+}
+
 type TestComposeCanonicalizedResource struct{}
 
 var _ = Suite(&TestComposeCanonicalizedResource{})

=== modified file 'x509dispatcher.go'
--- x509dispatcher.go	2013-07-30 06:49:18 +0000
+++ x509dispatcher.go	2013-08-09 15:00:01 +0000
@@ -1,3 +1,6 @@
+// Copyright 2013 Canonical Ltd.  This software is licensed under the
+// GNU Lesser General Public License version 3 (see the file COPYING).
+
 package gwacl
 
 import (
@@ -66,6 +69,8 @@
 }
 
 func performX509Request(session *x509Session, request *X509Request) (*x509Response, error) {
+    response := &x509Response{}
+
     Debugf("Request: %s %s", request.Method, request.URL)
     if len(request.Payload) > 0 {
         Debugf("Request body:\n%s", request.Payload)
@@ -78,15 +83,12 @@
     }
     httpRequest.Header.Set("Content-Type", request.ContentType)
     httpRequest.Header.Set("x-ms-version", request.APIVersion)
-    httpResponse, err := session.client.Do(httpRequest)
+    retrier := session.retryPolicy.getForkedHttpRetrier(session.client)
+    httpResponse, err := retrier.RetryRequest(httpRequest)
     if err != nil {
         return nil, err
     }
-    if httpResponse.Body != nil {
-        defer httpResponse.Body.Close()
-    }
 
-    response := &x509Response{}
     response.StatusCode = httpResponse.StatusCode
     response.Body, err = readAndClose(httpResponse.Body)
     if err != nil {

=== modified file 'x509dispatcher_test.go'
--- x509dispatcher_test.go	2013-08-06 05:11:29 +0000
+++ x509dispatcher_test.go	2013-08-09 15:00:01 +0000
@@ -5,6 +5,7 @@
     . "launchpad.net/gocheck"
     "net/http"
     "net/http/httptest"
+    "time"
 )
 
 type x509DispatcherSuite struct{}
@@ -49,7 +50,7 @@
     server := makeRecordingHTTPServer(httpRequests, http.StatusOK, nil, nil)
     defer server.Close()
     // No real certificate needed since we're testing on http, not https.
-    session, err := newX509Session("subscriptionid", "", "West US")
+    session, err := newX509Session("subscriptionid", "", "West US", NoRetryPolicy)
     c.Assert(err, IsNil)
     path := "/foo/bar"
     version := "test-version"
@@ -66,6 +67,28 @@
     c.Check(httpRequest.BodyContent, HasLen, 0)
 }
 
+func (*x509DispatcherSuite) TestRetryPolicyCausesRequestsToBeRetried(c *C) {
+    nbRetries := 2
+    nbRequests := nbRetries + 1
+    httpRequests := make(chan *Request, nbRequests)
+    server := makeRecordingHTTPServer(httpRequests, http.StatusConflict, nil, nil)
+    defer server.Close()
+    // No real certificate needed since we're testing on http, not https.
+    retryPolicy := RetryPolicy{NbRetries: nbRetries, HttpStatusCodes: []int{http.StatusConflict}, Delay: time.Nanosecond}
+    session, err := newX509Session("subscriptionid", "", "West US", retryPolicy)
+    c.Assert(err, IsNil)
+    path := "/foo/bar"
+    version := "test-version"
+    request := newX509RequestGET(server.URL+path, version)
+
+    response, err := performX509Request(session, request)
+    c.Assert(err, IsNil)
+    c.Assert(response.StatusCode, Equals, http.StatusConflict)
+
+    // nbRequests request were performed.
+    c.Check(httpRequests, HasLen, nbRequests)
+}
+
 func (*x509DispatcherSuite) TestPostRequestDoesHTTPPOST(c *C) {
     httpRequests := make(chan *Request, 1)
     requestBody := []byte{1, 2, 3}
@@ -74,7 +97,7 @@
     server := makeRecordingHTTPServer(httpRequests, http.StatusOK, responseBody, nil)
     defer server.Close()
     // No real certificate needed since we're testing on http, not https.
-    session, err := newX509Session("subscriptionid", "", "West US")
+    session, err := newX509Session("subscriptionid", "", "West US", NoRetryPolicy)
     c.Assert(err, IsNil)
     path := "/foo/bar"
     version := "test-version"
@@ -98,7 +121,7 @@
     server := makeRecordingHTTPServer(httpRequests, http.StatusOK, nil, nil)
     defer server.Close()
     // No real certificate needed since we're testing on http, not https.
-    session, err := newX509Session("subscriptionid", "", "West US")
+    session, err := newX509Session("subscriptionid", "", "West US", NoRetryPolicy)
     c.Assert(err, IsNil)
     path := "/foo/bar"
     version := "test-version"
@@ -122,7 +145,7 @@
     server := makeRecordingHTTPServer(httpRequests, http.StatusOK, responseBody, nil)
     defer server.Close()
     // No real certificate needed since we're testing on http, not https.
-    session, err := newX509Session("subscriptionid", "", "West US")
+    session, err := newX509Session("subscriptionid", "", "West US", NoRetryPolicy)
     c.Assert(err, IsNil)
     path := "/foo/bar"
     version := "test-version"
@@ -151,7 +174,7 @@
     serveMux.HandleFunc("/", returnRequest)
     server := httptest.NewServer(serveMux)
     defer server.Close()
-    session, err := newX509Session("subscriptionid", "", "West US")
+    session, err := newX509Session("subscriptionid", "", "West US", NoRetryPolicy)
     c.Assert(err, IsNil)
     path := "/foo/bar"
     request := newX509RequestGET(server.URL+path, "testversion")

=== modified file 'x509session.go'
--- x509session.go	2013-08-06 05:31:49 +0000
+++ x509session.go	2013-08-09 15:00:01 +0000
@@ -16,13 +16,14 @@
     certFile       string
     client         *http.Client
     baseURL        *url.URL
+    retryPolicy    RetryPolicy
 }
 
 // newX509Session creates and returns a new x509Session based on credentials
 // and X509 certificate files.
 // For testing purposes, certFile can be passed as the empty string and it
 // will be ignored.
-func newX509Session(subscriptionId, certFile, location string) (*x509Session, error) {
+func newX509Session(subscriptionId, certFile, location string, retryPolicy RetryPolicy) (*x509Session, error) {
     certs := []tls.Certificate{}
     if certFile != "" {
         //
@@ -51,6 +52,7 @@
         certFile:       certFile,
         client:         &client,
         baseURL:        baseURL,
+        retryPolicy:    retryPolicy,
     }
     return &session, nil
 }

=== modified file 'x509session_test.go'
--- x509session_test.go	2013-08-06 05:31:49 +0000
+++ x509session_test.go	2013-08-09 15:00:01 +0000
@@ -109,7 +109,7 @@
 }
 
 func (suite *x509SessionSuite) TestNewX509Session(c *C) {
-    session, err := newX509Session("subscriptionid", "", "China East")
+    session, err := newX509Session("subscriptionid", "", "China East", NoRetryPolicy)
     c.Assert(err, IsNil)
     c.Assert(session.baseURL, NotNil)
     c.Check(session.baseURL.String(), Equals, GetEndpoint("China East").ManagementAPI())
@@ -118,7 +118,7 @@
 func (suite *x509SessionSuite) TestComposeURLComposesURLWithRelativePath(c *C) {
     const subscriptionID = "subscriptionid"
     const path = "foo/bar"
-    session, err := newX509Session(subscriptionID, "", "West US")
+    session, err := newX509Session(subscriptionID, "", "West US", NoRetryPolicy)
     c.Assert(err, IsNil)
 
     url := session.composeURL(path)
@@ -132,7 +132,7 @@
         c.Assert(err, NotNil)
         c.Check(err, ErrorMatches, ".*absolute.*path.*")
     }()
-    session, err := newX509Session("subscriptionid", "", "West US")
+    session, err := newX509Session("subscriptionid", "", "West US", NoRetryPolicy)
     c.Assert(err, IsNil)
 
     // This panics because we're passing an absolute path.
@@ -142,7 +142,7 @@
 func (suite *x509SessionSuite) TestGetServerErrorProducesServerError(c *C) {
     msg := "huhwhat"
     status := http.StatusNotFound
-    session, err := newX509Session("subscriptionid", "", "West US")
+    session, err := newX509Session("subscriptionid", "", "West US", NoRetryPolicy)
     c.Assert(err, IsNil)
 
     err = session.getServerError(status, []byte{}, msg)
@@ -158,7 +158,7 @@
         http.StatusOK,
         http.StatusNoContent,
     }
-    session, err := newX509Session("subscriptionid", "", "West US")
+    session, err := newX509Session("subscriptionid", "", "West US", NoRetryPolicy)
     c.Assert(err, IsNil)
 
     for _, status := range goodCodes {
@@ -176,7 +176,7 @@
         http.StatusInternalServerError,
         http.StatusNotImplemented,
     }
-    session, err := newX509Session("subscriptionid", "", "West US")
+    session, err := newX509Session("subscriptionid", "", "West US", NoRetryPolicy)
     c.Assert(err, IsNil)
 
     for _, status := range badCodes {
@@ -187,7 +187,7 @@
 func (suite *x509SessionSuite) TestGetIssuesRequest(c *C) {
     subscriptionID := "subscriptionID"
     uri := "resource"
-    session, err := newX509Session(subscriptionID, "", "West US")
+    session, err := newX509Session(subscriptionID, "", "West US", NoRetryPolicy)
     c.Assert(err, IsNil)
     // Record incoming requests, and have them return a given reply.
     fixedResponse := x509Response{
@@ -211,7 +211,7 @@
 }
 
 func (suite *x509SessionSuite) TestGetReportsClientSideError(c *C) {
-    session, err := newX509Session("subscriptionid", "", "West US")
+    session, err := newX509Session("subscriptionid", "", "West US", NoRetryPolicy)
     msg := "could not dispatch request"
     rigFailingDispatcher(fmt.Errorf(msg))
 
@@ -223,7 +223,7 @@
 }
 
 func (suite *x509SessionSuite) TestGetReportsServerSideError(c *C) {
-    session, err := newX509Session("subscriptionid", "", "West US")
+    session, err := newX509Session("subscriptionid", "", "West US", NoRetryPolicy)
     fixedResponse := x509Response{
         StatusCode: http.StatusForbidden,
         Body:       []byte("Body"),
@@ -244,7 +244,7 @@
     version := "test-version"
     requestBody := []byte("Request body")
     requestContentType := "bogusContentType"
-    session, err := newX509Session(subscriptionID, "", "West US")
+    session, err := newX509Session(subscriptionID, "", "West US", NoRetryPolicy)
     c.Assert(err, IsNil)
     // Record incoming requests, and have them return a given reply.
     fixedResponse := x509Response{
@@ -269,7 +269,7 @@
 }
 
 func (suite *x509SessionSuite) TestPostReportsClientSideError(c *C) {
-    session, err := newX509Session("subscriptionid", "", "West US")
+    session, err := newX509Session("subscriptionid", "", "West US", NoRetryPolicy)
     msg := "could not dispatch request"
     rigFailingDispatcher(fmt.Errorf(msg))
 
@@ -281,7 +281,7 @@
 }
 
 func (suite *x509SessionSuite) TestPostReportsServerSideError(c *C) {
-    session, err := newX509Session("subscriptionid", "", "West US")
+    session, err := newX509Session("subscriptionid", "", "West US", NoRetryPolicy)
     fixedResponse := x509Response{
         StatusCode: http.StatusForbidden,
         Body:       []byte("Body"),
@@ -300,7 +300,7 @@
     subscriptionID := "subscriptionID"
     uri := "resource"
     version := "test-version"
-    session, err := newX509Session(subscriptionID, "", "West US")
+    session, err := newX509Session(subscriptionID, "", "West US", NoRetryPolicy)
     c.Assert(err, IsNil)
     // Record incoming requests, and have them return a given reply.
     fixedResponse := x509Response{StatusCode: http.StatusOK}
@@ -324,7 +324,7 @@
     uri := "resource"
     version := "test-version"
     requestBody := []byte("Request body")
-    session, err := newX509Session(subscriptionID, "", "West US")
+    session, err := newX509Session(subscriptionID, "", "West US", NoRetryPolicy)
     c.Assert(err, IsNil)
     // Record incoming requests, and have them return a given reply.
     fixedResponse := x509Response{