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

Proposed by Raphaël Badin on 2013-08-08
Status: Merged
Approved by: Raphaël Badin on 2013-08-12
Approved revision: 220
Merged at revision: 217
Proposed branch: lp:~rvb/gwacl/retry-feature
Merge into: lp:gwacl
Diff against target: 702 lines (+390/-37)
10 files modified
management_base.go (+17/-6)
management_base_test.go (+22/-5)
retry_policy.go (+122/-0)
retry_policy_test.go (+150/-0)
storage_base.go (+5/-1)
storage_base_test.go (+22/-0)
x509dispatcher.go (+7/-5)
x509dispatcher_test.go (+28/-5)
x509session.go (+3/-1)
x509session_test.go (+14/-14)
To merge this branch: bzr merge lp:~rvb/gwacl/retry-feature
Reviewer Review Type Date Requested Status
Julian Edwards (community) 2013-08-08 Approve on 2013-08-09
Review via email: mp+179212@code.launchpad.net

Commit message

Add retry feature.

Description of the change

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().

To post a comment you must log in.
Julian Edwards (julian-edwards) wrote :

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) wrote :
Download full text (4.1 KiB)

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...

Read more...

review: Approve
Raphaël Badin (rvb) wrote :

> 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 updated on 2013-08-09
219. By Raphaël Badin on 2013-08-09

Move retry in utility method.

Raphaël Badin (rvb) wrote :
Download full text (3.9 KiB)

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...

Read more...

lp:~rvb/gwacl/retry-feature updated on 2013-08-09
220. By Raphaël Badin on 2013-08-09

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

Julian Edwards (julian-edwards) wrote :

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) wrote :

> 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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'management_base.go'
2--- management_base.go 2013-08-06 05:11:29 +0000
3+++ management_base.go 2013-08-09 15:00:01 +0000
4@@ -32,15 +32,22 @@
5 // The default duration after which the polling is terminated.
6 const DefaultPollerTimeout = 20 * time.Minute
7
8+// NewManagementAPIWithRetryPolicy creates an object used to interact with
9+// Windows Azure's API.
10+// http://msdn.microsoft.com/en-us/library/windowsazure/ff800682.aspx
11+func NewManagementAPIWithRetryPolicy(subscriptionId, certFile, location string, policy RetryPolicy) (*ManagementAPI, error) {
12+ session, err := newX509Session(subscriptionId, certFile, location, policy)
13+ if err != nil {
14+ return nil, err
15+ }
16+ api := ManagementAPI{session, DefaultPollerInterval, DefaultPollerTimeout}
17+ return &api, nil
18+}
19+
20 // NewManagementAPI creates an object used to interact with Windows Azure's API.
21 // http://msdn.microsoft.com/en-us/library/windowsazure/ff800682.aspx
22 func NewManagementAPI(subscriptionId, certFile, location string) (*ManagementAPI, error) {
23- session, err := newX509Session(subscriptionId, certFile, location)
24- if err != nil {
25- return nil, err
26- }
27- api := ManagementAPI{session, DefaultPollerInterval, DefaultPollerTimeout}
28- return &api, nil
29+ return NewManagementAPIWithRetryPolicy(subscriptionId, certFile, location, NoRetryPolicy)
30 }
31
32 var operationIDHeaderName = http.CanonicalHeaderKey("x-ms-request-id")
33@@ -56,6 +63,10 @@
34 return "", err
35 }
36
37+func (api *ManagementAPI) GetRetryPolicy() RetryPolicy {
38+ return api.session.retryPolicy
39+}
40+
41 // blockUntilCompleted blocks and polls for completion of an Azure operation.
42 // The "response" parameter is the result of the request that started the
43 // operation. If the response says that the operation is running
44
45=== modified file 'management_base_test.go'
46--- management_base_test.go 2013-08-06 05:31:49 +0000
47+++ management_base_test.go 2013-08-09 15:00:01 +0000
48@@ -234,11 +234,28 @@
49 c.Assert(err, IsNil)
50
51 api, err := NewManagementAPI(subscriptionId, certFile, "West US")
52-
53- c.Assert(err, IsNil)
54- session, err := newX509Session(subscriptionId, certFile, "West US")
55- c.Assert(api.session.subscriptionId, DeepEquals, session.subscriptionId)
56- c.Assert(api.session.certFile, DeepEquals, session.certFile)
57+ c.Assert(err, IsNil)
58+
59+ c.Assert(api.session.subscriptionId, DeepEquals, subscriptionId)
60+ c.Assert(api.session.certFile, DeepEquals, certFile)
61+ c.Assert(api.session.retryPolicy, DeepEquals, NoRetryPolicy)
62+}
63+
64+func (suite *managementBaseAPISuite) TestNewManagementAPIWithRetryPolicy(c *C) {
65+ subscriptionId := "subscriptionId"
66+ certDir := c.MkDir()
67+ certFile := certDir + "/cert.pem"
68+ err := ioutil.WriteFile(certFile, []byte(testCert), 0600)
69+ c.Assert(err, IsNil)
70+ retryPolicy := RetryPolicy{NbRetries: 5, HttpStatusCodes: []int{409}, Delay: time.Minute}
71+
72+ api, err := NewManagementAPIWithRetryPolicy(subscriptionId, certFile, "West US", retryPolicy)
73+ c.Assert(err, IsNil)
74+
75+ c.Assert(api.session.subscriptionId, DeepEquals, subscriptionId)
76+ c.Assert(api.session.certFile, DeepEquals, certFile)
77+ c.Assert(api.session.retryPolicy, DeepEquals, retryPolicy)
78+ c.Assert(api.GetRetryPolicy(), DeepEquals, retryPolicy)
79 }
80
81 func (suite *managementBaseAPISuite) TestNewManagementAPISetsDefaultPollerInterval(c *C) {
82
83=== added file 'retry_policy.go'
84--- retry_policy.go 1970-01-01 00:00:00 +0000
85+++ retry_policy.go 2013-08-09 15:00:01 +0000
86@@ -0,0 +1,122 @@
87+// Copyright 2013 Canonical Ltd. This software is licensed under the
88+// GNU Lesser General Public License version 3 (see the file COPYING).
89+
90+package gwacl
91+
92+import (
93+ forkedHttp "launchpad.net/gwacl/fork/http"
94+ "net/http"
95+ "time"
96+)
97+
98+// A RetryPolicy object encapsulates all the information needed to define how
99+// requests should be retried when particular response codes are returned by
100+// the Windows Azure server.
101+type RetryPolicy struct {
102+ // The number of times a request could be retried. This does not account
103+ // for the initial request so a value of 3 means that the request might be
104+ // performed 4 times in total.
105+ NbRetries int
106+ // The HTTP status codes of the response for which the request should be
107+ // retried.
108+ HttpStatusCodes []int
109+ // How long the client should wait between retries.
110+ Delay time.Duration
111+}
112+
113+var (
114+ NoRetryPolicy = RetryPolicy{NbRetries: 0}
115+)
116+
117+// isRetryCode returns whether or not the given http status code indicates that
118+// the request should be retried according to this policy.
119+func (policy RetryPolicy) isRetryCode(httpStatusCode int) bool {
120+ for _, code := range policy.HttpStatusCodes {
121+ if code == httpStatusCode {
122+ return true
123+ }
124+ }
125+ return false
126+}
127+
128+func (policy RetryPolicy) getRetryHelper() *retryHelper {
129+ return &retryHelper{retriesLeft: policy.NbRetries, policy: &policy}
130+}
131+
132+// A retryHelper is a utility object used to enforce a retry policy when
133+// performing requests.
134+type retryHelper struct {
135+ // The maximum number of retries left to perform.
136+ retriesLeft int
137+ // The `RetryPolicy` enforced by this retrier.
138+ policy *RetryPolicy
139+}
140+
141+// shouldRetry returns whether or not a request governed by the underlying
142+// retry policy should be retried. When it returns 'true', `shouldRetry` also
143+// waits for the specified amount of time, as dictated by the retry policy.
144+func (ret *retryHelper) shouldRetry(httpStatusCode int) bool {
145+ if ret.retriesLeft > 0 && ret.policy.isRetryCode(httpStatusCode) {
146+ ret.retriesLeft--
147+ return true
148+ }
149+ return false
150+}
151+
152+// A retrier is a struct used to repeat a request as governed by a retry
153+// policy. retrier is usually created using RetryPolicy.getRetrier().
154+type retrier struct {
155+ *retryHelper
156+
157+ // The client used to perform requests.
158+ client *http.Client
159+}
160+
161+func (ret *retrier) RetryRequest(request *http.Request) (*http.Response, error) {
162+ for {
163+ response, err := ret.client.Do(request)
164+ if err != nil {
165+ return nil, err
166+ }
167+ if !ret.shouldRetry(response.StatusCode) {
168+ return response, nil
169+ }
170+ time.Sleep(ret.policy.Delay)
171+ }
172+}
173+
174+// getRetrier returns a `retrier` object used to enforce the retry policy.
175+func (policy RetryPolicy) getRetrier(client *http.Client) *retrier {
176+ helper := policy.getRetryHelper()
177+ return &retrier{retryHelper: helper, client: client}
178+}
179+
180+// A forkedHttpRetrier is a struct used to repeat a request as governed by a
181+// retry policy. forkedHttpRetrier is usually created using
182+// RetryPolicy.getForkedHttpRetrier(). It's the same as the `retrier` struct
183+// except it deals with the forked version of the http package.
184+type forkedHttpRetrier struct {
185+ *retryHelper
186+
187+ // The client used to perform requests.
188+ client *forkedHttp.Client
189+}
190+
191+func (ret *forkedHttpRetrier) RetryRequest(request *forkedHttp.Request) (*forkedHttp.Response, error) {
192+ for {
193+ response, err := ret.client.Do(request)
194+ if err != nil {
195+ return nil, err
196+ }
197+ if !ret.shouldRetry(response.StatusCode) {
198+ return response, nil
199+ }
200+ time.Sleep(ret.policy.Delay)
201+ }
202+}
203+
204+// getRetrier returns a `retrier` object used to enforce the retry policy.
205+func (policy RetryPolicy) getForkedHttpRetrier(client *forkedHttp.Client) *forkedHttpRetrier {
206+ helper := policy.getRetryHelper()
207+ return &forkedHttpRetrier{retryHelper: helper, client: client}
208+}
209
210=== added file 'retry_policy_test.go'
211--- retry_policy_test.go 1970-01-01 00:00:00 +0000
212+++ retry_policy_test.go 2013-08-09 15:00:01 +0000
213@@ -0,0 +1,150 @@
214+// Copyright 2013 Canonical Ltd. This software is licensed under the
215+// GNU Lesser General Public License version 3 (see the file COPYING).
216+
217+package gwacl
218+
219+import (
220+ "fmt"
221+ . "launchpad.net/gocheck"
222+ forkedHttp "launchpad.net/gwacl/fork/http"
223+ "net/http"
224+ "time"
225+)
226+
227+type retryPolicySuite struct{}
228+
229+var _ = Suite(&retryPolicySuite{})
230+
231+func (*retryPolicySuite) TestNoRetryPolicyDoesNotRetry(c *C) {
232+ c.Check(NoRetryPolicy.NbRetries, Equals, 0)
233+}
234+
235+func (*retryPolicySuite) TestDefaultPolicyIsNoRetryPolicy(c *C) {
236+ c.Check(NoRetryPolicy, DeepEquals, RetryPolicy{})
237+}
238+
239+func (*retryPolicySuite) TestIsRetryCodeChecksStatusCode(c *C) {
240+ c.Check(
241+ RetryPolicy{HttpStatusCodes: []int{http.StatusConflict}}.isRetryCode(http.StatusConflict),
242+ Equals, true)
243+ c.Check(
244+ RetryPolicy{HttpStatusCodes: []int{}}.isRetryCode(http.StatusOK),
245+ Equals, false)
246+ c.Check(
247+ RetryPolicy{HttpStatusCodes: []int{http.StatusConflict}}.isRetryCode(http.StatusOK),
248+ Equals, false)
249+
250+}
251+
252+func (*retryPolicySuite) TestGetRetryHelperReturnsHelper(c *C) {
253+ policy := RetryPolicy{NbRetries: 7, HttpStatusCodes: []int{http.StatusConflict}, Delay: time.Minute}
254+ helper := policy.getRetryHelper()
255+ c.Check(*helper.policy, DeepEquals, policy)
256+ c.Check(helper.retriesLeft, Equals, policy.NbRetries)
257+}
258+
259+type retryHelperSuite struct{}
260+
261+var _ = Suite(&retryHelperSuite{})
262+
263+func (*retryHelperSuite) TestShouldRetryExhaustsRetries(c *C) {
264+ nbTries := 3
265+ policy := RetryPolicy{NbRetries: nbTries, HttpStatusCodes: []int{http.StatusConflict}, Delay: time.Nanosecond}
266+ helper := policy.getRetryHelper()
267+ retries := []bool{}
268+ for i := 0; i < nbTries+1; i++ {
269+ retries = append(retries, helper.shouldRetry(http.StatusConflict))
270+ }
271+ expectedRetries := []bool{true, true, true, false}
272+ c.Check(retries, DeepEquals, expectedRetries)
273+}
274+
275+func (*retryHelperSuite) TestShouldRetryReturnsFalseIfCodeNotInHttpStatusCodes(c *C) {
276+ policy := RetryPolicy{NbRetries: 10, HttpStatusCodes: []int{http.StatusConflict}, Delay: time.Nanosecond}
277+ helper := policy.getRetryHelper()
278+ c.Check(helper.shouldRetry(http.StatusOK), Equals, false)
279+}
280+
281+type retrierSuite struct{}
282+
283+var _ = Suite(&retrierSuite{})
284+
285+func (*retrierSuite) TestGetRetrier(c *C) {
286+ client := &http.Client{}
287+ policy := RetryPolicy{NbRetries: 10, HttpStatusCodes: []int{http.StatusConflict}, Delay: time.Nanosecond}
288+ retrier := policy.getRetrier(client)
289+ c.Check(*retrier.policy, DeepEquals, policy)
290+ c.Check(retrier.client, DeepEquals, client)
291+}
292+
293+func (*retrierSuite) TestRetryRequest(c *C) {
294+ nbTries := 3
295+ transport := &MockingTransport{}
296+ client := &http.Client{Transport: transport}
297+ for i := 0; i < nbTries; i++ {
298+ response := makeHttpResponse(http.StatusConflict, "")
299+ transport.AddExchange(response, nil)
300+ }
301+ response := makeHttpResponse(http.StatusOK, "")
302+ transport.AddExchange(response, nil)
303+
304+ policy := RetryPolicy{NbRetries: nbTries, HttpStatusCodes: []int{http.StatusConflict}, Delay: time.Nanosecond}
305+ retrier := policy.getRetrier(client)
306+ req, err := http.NewRequest("GET", "http://example.com/", nil)
307+ c.Assert(err, IsNil)
308+
309+ resp, err := retrier.RetryRequest(req)
310+ c.Assert(err, IsNil)
311+
312+ c.Check(resp.StatusCode, Equals, http.StatusOK)
313+ c.Check(transport.ExchangeCount, Equals, nbTries+1)
314+}
315+
316+func (*retrierSuite) TestRetryRequestBailsOutWhenError(c *C) {
317+ nbTries := 3
318+ transport := &MockingTransport{}
319+ client := &http.Client{Transport: transport}
320+ transport.AddExchange(nil, fmt.Errorf("request error"))
321+
322+ policy := RetryPolicy{NbRetries: nbTries, HttpStatusCodes: []int{http.StatusConflict}, Delay: time.Nanosecond}
323+ retrier := policy.getRetrier(client)
324+ req, err := http.NewRequest("GET", "http://example.com/", nil)
325+ c.Assert(err, IsNil)
326+
327+ _, err = retrier.RetryRequest(req)
328+ c.Check(err, ErrorMatches, ".*request error.*")
329+
330+ c.Check(transport.ExchangeCount, Equals, 1)
331+}
332+
333+type forkedHttpRetrierSuite struct{}
334+
335+var _ = Suite(&forkedHttpRetrierSuite{})
336+
337+func (*forkedHttpRetrierSuite) TestGetRetrier(c *C) {
338+ client := &forkedHttp.Client{}
339+ policy := RetryPolicy{NbRetries: 10, HttpStatusCodes: []int{forkedHttp.StatusConflict}, Delay: time.Nanosecond}
340+ retrier := policy.getForkedHttpRetrier(client)
341+ c.Check(*retrier.policy, DeepEquals, policy)
342+ c.Check(retrier.client, DeepEquals, client)
343+}
344+
345+func (*forkedHttpRetrierSuite) TestRetryRequest(c *C) {
346+ nbTries := 3
347+ nbRequests := nbTries + 1
348+ client := &forkedHttp.Client{}
349+ httpRequests := make(chan *Request, nbRequests)
350+ server := makeRecordingHTTPServer(httpRequests, http.StatusConflict, nil, nil)
351+ defer server.Close()
352+
353+ policy := RetryPolicy{NbRetries: nbTries, HttpStatusCodes: []int{forkedHttp.StatusConflict}, Delay: time.Nanosecond}
354+ retrier := policy.getForkedHttpRetrier(client)
355+ req, err := forkedHttp.NewRequest("GET", server.URL, nil)
356+ c.Assert(err, IsNil)
357+
358+ resp, err := retrier.RetryRequest(req)
359+ c.Assert(err, IsNil)
360+
361+ c.Check(resp.StatusCode, Equals, forkedHttp.StatusConflict)
362+ c.Check(len(httpRequests), Equals, nbTries+1)
363+}
364
365=== modified file 'storage_base.go'
366--- storage_base.go 2013-08-07 02:13:25 +0000
367+++ storage_base.go 2013-08-09 15:00:01 +0000
368@@ -214,6 +214,8 @@
369 AzureEndpoint APIEndpoint
370
371 client *http.Client
372+
373+ RetryPolicy RetryPolicy
374 }
375
376 // getClient is used when sending a request. If a custom client is specified
377@@ -295,7 +297,9 @@
378 // then an HTTPError will be returned as the error.
379 func (context *StorageContext) send(req *http.Request, res Deserializer, expectedStatus HTTPStatus) ([]byte, http.Header, error) {
380 client := context.getClient()
381- resp, err := client.Do(req)
382+
383+ retrier := context.RetryPolicy.getRetrier(client)
384+ resp, err := retrier.RetryRequest(req)
385 if err != nil {
386 return nil, nil, err
387 }
388
389=== modified file 'storage_base_test.go'
390--- storage_base_test.go 2013-08-07 02:13:25 +0000
391+++ storage_base_test.go 2013-08-09 15:00:01 +0000
392@@ -67,6 +67,28 @@
393 c.Check(observed, Equals, expected)
394 }
395
396+type TestRetryRequests struct{}
397+
398+var _ = Suite(&TestRetryRequests{})
399+
400+func (suite *TestRetryRequests) TestRequestIsRetried(c *C) {
401+ transport := &MockingTransport{}
402+ body := []byte("data")
403+ transport.AddExchange(&http.Response{StatusCode: http.StatusConflict, Body: Empty}, nil)
404+ transport.AddExchange(&http.Response{StatusCode: http.StatusConflict, Body: Empty}, nil)
405+ transport.AddExchange(&http.Response{StatusCode: http.StatusOK, Body: makeResponseBody(string(body))}, nil)
406+ retryPolicy := RetryPolicy{NbRetries: 3, HttpStatusCodes: []int{http.StatusConflict}, Delay: time.Nanosecond}
407+ context := makeStorageContext(transport)
408+ context.RetryPolicy = retryPolicy
409+ req, err := http.NewRequest("GET", "http://example.com", nil)
410+ c.Assert(err, IsNil)
411+
412+ resBody, _, err := context.send(req, nil, http.StatusOK)
413+ c.Assert(err, IsNil)
414+ c.Assert(transport.ExchangeCount, Equals, 3)
415+ c.Check(resBody, DeepEquals, body)
416+}
417+
418 type TestComposeCanonicalizedResource struct{}
419
420 var _ = Suite(&TestComposeCanonicalizedResource{})
421
422=== modified file 'x509dispatcher.go'
423--- x509dispatcher.go 2013-07-30 06:49:18 +0000
424+++ x509dispatcher.go 2013-08-09 15:00:01 +0000
425@@ -1,3 +1,6 @@
426+// Copyright 2013 Canonical Ltd. This software is licensed under the
427+// GNU Lesser General Public License version 3 (see the file COPYING).
428+
429 package gwacl
430
431 import (
432@@ -66,6 +69,8 @@
433 }
434
435 func performX509Request(session *x509Session, request *X509Request) (*x509Response, error) {
436+ response := &x509Response{}
437+
438 Debugf("Request: %s %s", request.Method, request.URL)
439 if len(request.Payload) > 0 {
440 Debugf("Request body:\n%s", request.Payload)
441@@ -78,15 +83,12 @@
442 }
443 httpRequest.Header.Set("Content-Type", request.ContentType)
444 httpRequest.Header.Set("x-ms-version", request.APIVersion)
445- httpResponse, err := session.client.Do(httpRequest)
446+ retrier := session.retryPolicy.getForkedHttpRetrier(session.client)
447+ httpResponse, err := retrier.RetryRequest(httpRequest)
448 if err != nil {
449 return nil, err
450 }
451- if httpResponse.Body != nil {
452- defer httpResponse.Body.Close()
453- }
454
455- response := &x509Response{}
456 response.StatusCode = httpResponse.StatusCode
457 response.Body, err = readAndClose(httpResponse.Body)
458 if err != nil {
459
460=== modified file 'x509dispatcher_test.go'
461--- x509dispatcher_test.go 2013-08-06 05:11:29 +0000
462+++ x509dispatcher_test.go 2013-08-09 15:00:01 +0000
463@@ -5,6 +5,7 @@
464 . "launchpad.net/gocheck"
465 "net/http"
466 "net/http/httptest"
467+ "time"
468 )
469
470 type x509DispatcherSuite struct{}
471@@ -49,7 +50,7 @@
472 server := makeRecordingHTTPServer(httpRequests, http.StatusOK, nil, nil)
473 defer server.Close()
474 // No real certificate needed since we're testing on http, not https.
475- session, err := newX509Session("subscriptionid", "", "West US")
476+ session, err := newX509Session("subscriptionid", "", "West US", NoRetryPolicy)
477 c.Assert(err, IsNil)
478 path := "/foo/bar"
479 version := "test-version"
480@@ -66,6 +67,28 @@
481 c.Check(httpRequest.BodyContent, HasLen, 0)
482 }
483
484+func (*x509DispatcherSuite) TestRetryPolicyCausesRequestsToBeRetried(c *C) {
485+ nbRetries := 2
486+ nbRequests := nbRetries + 1
487+ httpRequests := make(chan *Request, nbRequests)
488+ server := makeRecordingHTTPServer(httpRequests, http.StatusConflict, nil, nil)
489+ defer server.Close()
490+ // No real certificate needed since we're testing on http, not https.
491+ retryPolicy := RetryPolicy{NbRetries: nbRetries, HttpStatusCodes: []int{http.StatusConflict}, Delay: time.Nanosecond}
492+ session, err := newX509Session("subscriptionid", "", "West US", retryPolicy)
493+ c.Assert(err, IsNil)
494+ path := "/foo/bar"
495+ version := "test-version"
496+ request := newX509RequestGET(server.URL+path, version)
497+
498+ response, err := performX509Request(session, request)
499+ c.Assert(err, IsNil)
500+ c.Assert(response.StatusCode, Equals, http.StatusConflict)
501+
502+ // nbRequests request were performed.
503+ c.Check(httpRequests, HasLen, nbRequests)
504+}
505+
506 func (*x509DispatcherSuite) TestPostRequestDoesHTTPPOST(c *C) {
507 httpRequests := make(chan *Request, 1)
508 requestBody := []byte{1, 2, 3}
509@@ -74,7 +97,7 @@
510 server := makeRecordingHTTPServer(httpRequests, http.StatusOK, responseBody, nil)
511 defer server.Close()
512 // No real certificate needed since we're testing on http, not https.
513- session, err := newX509Session("subscriptionid", "", "West US")
514+ session, err := newX509Session("subscriptionid", "", "West US", NoRetryPolicy)
515 c.Assert(err, IsNil)
516 path := "/foo/bar"
517 version := "test-version"
518@@ -98,7 +121,7 @@
519 server := makeRecordingHTTPServer(httpRequests, http.StatusOK, nil, nil)
520 defer server.Close()
521 // No real certificate needed since we're testing on http, not https.
522- session, err := newX509Session("subscriptionid", "", "West US")
523+ session, err := newX509Session("subscriptionid", "", "West US", NoRetryPolicy)
524 c.Assert(err, IsNil)
525 path := "/foo/bar"
526 version := "test-version"
527@@ -122,7 +145,7 @@
528 server := makeRecordingHTTPServer(httpRequests, http.StatusOK, responseBody, nil)
529 defer server.Close()
530 // No real certificate needed since we're testing on http, not https.
531- session, err := newX509Session("subscriptionid", "", "West US")
532+ session, err := newX509Session("subscriptionid", "", "West US", NoRetryPolicy)
533 c.Assert(err, IsNil)
534 path := "/foo/bar"
535 version := "test-version"
536@@ -151,7 +174,7 @@
537 serveMux.HandleFunc("/", returnRequest)
538 server := httptest.NewServer(serveMux)
539 defer server.Close()
540- session, err := newX509Session("subscriptionid", "", "West US")
541+ session, err := newX509Session("subscriptionid", "", "West US", NoRetryPolicy)
542 c.Assert(err, IsNil)
543 path := "/foo/bar"
544 request := newX509RequestGET(server.URL+path, "testversion")
545
546=== modified file 'x509session.go'
547--- x509session.go 2013-08-06 05:31:49 +0000
548+++ x509session.go 2013-08-09 15:00:01 +0000
549@@ -16,13 +16,14 @@
550 certFile string
551 client *http.Client
552 baseURL *url.URL
553+ retryPolicy RetryPolicy
554 }
555
556 // newX509Session creates and returns a new x509Session based on credentials
557 // and X509 certificate files.
558 // For testing purposes, certFile can be passed as the empty string and it
559 // will be ignored.
560-func newX509Session(subscriptionId, certFile, location string) (*x509Session, error) {
561+func newX509Session(subscriptionId, certFile, location string, retryPolicy RetryPolicy) (*x509Session, error) {
562 certs := []tls.Certificate{}
563 if certFile != "" {
564 //
565@@ -51,6 +52,7 @@
566 certFile: certFile,
567 client: &client,
568 baseURL: baseURL,
569+ retryPolicy: retryPolicy,
570 }
571 return &session, nil
572 }
573
574=== modified file 'x509session_test.go'
575--- x509session_test.go 2013-08-06 05:31:49 +0000
576+++ x509session_test.go 2013-08-09 15:00:01 +0000
577@@ -109,7 +109,7 @@
578 }
579
580 func (suite *x509SessionSuite) TestNewX509Session(c *C) {
581- session, err := newX509Session("subscriptionid", "", "China East")
582+ session, err := newX509Session("subscriptionid", "", "China East", NoRetryPolicy)
583 c.Assert(err, IsNil)
584 c.Assert(session.baseURL, NotNil)
585 c.Check(session.baseURL.String(), Equals, GetEndpoint("China East").ManagementAPI())
586@@ -118,7 +118,7 @@
587 func (suite *x509SessionSuite) TestComposeURLComposesURLWithRelativePath(c *C) {
588 const subscriptionID = "subscriptionid"
589 const path = "foo/bar"
590- session, err := newX509Session(subscriptionID, "", "West US")
591+ session, err := newX509Session(subscriptionID, "", "West US", NoRetryPolicy)
592 c.Assert(err, IsNil)
593
594 url := session.composeURL(path)
595@@ -132,7 +132,7 @@
596 c.Assert(err, NotNil)
597 c.Check(err, ErrorMatches, ".*absolute.*path.*")
598 }()
599- session, err := newX509Session("subscriptionid", "", "West US")
600+ session, err := newX509Session("subscriptionid", "", "West US", NoRetryPolicy)
601 c.Assert(err, IsNil)
602
603 // This panics because we're passing an absolute path.
604@@ -142,7 +142,7 @@
605 func (suite *x509SessionSuite) TestGetServerErrorProducesServerError(c *C) {
606 msg := "huhwhat"
607 status := http.StatusNotFound
608- session, err := newX509Session("subscriptionid", "", "West US")
609+ session, err := newX509Session("subscriptionid", "", "West US", NoRetryPolicy)
610 c.Assert(err, IsNil)
611
612 err = session.getServerError(status, []byte{}, msg)
613@@ -158,7 +158,7 @@
614 http.StatusOK,
615 http.StatusNoContent,
616 }
617- session, err := newX509Session("subscriptionid", "", "West US")
618+ session, err := newX509Session("subscriptionid", "", "West US", NoRetryPolicy)
619 c.Assert(err, IsNil)
620
621 for _, status := range goodCodes {
622@@ -176,7 +176,7 @@
623 http.StatusInternalServerError,
624 http.StatusNotImplemented,
625 }
626- session, err := newX509Session("subscriptionid", "", "West US")
627+ session, err := newX509Session("subscriptionid", "", "West US", NoRetryPolicy)
628 c.Assert(err, IsNil)
629
630 for _, status := range badCodes {
631@@ -187,7 +187,7 @@
632 func (suite *x509SessionSuite) TestGetIssuesRequest(c *C) {
633 subscriptionID := "subscriptionID"
634 uri := "resource"
635- session, err := newX509Session(subscriptionID, "", "West US")
636+ session, err := newX509Session(subscriptionID, "", "West US", NoRetryPolicy)
637 c.Assert(err, IsNil)
638 // Record incoming requests, and have them return a given reply.
639 fixedResponse := x509Response{
640@@ -211,7 +211,7 @@
641 }
642
643 func (suite *x509SessionSuite) TestGetReportsClientSideError(c *C) {
644- session, err := newX509Session("subscriptionid", "", "West US")
645+ session, err := newX509Session("subscriptionid", "", "West US", NoRetryPolicy)
646 msg := "could not dispatch request"
647 rigFailingDispatcher(fmt.Errorf(msg))
648
649@@ -223,7 +223,7 @@
650 }
651
652 func (suite *x509SessionSuite) TestGetReportsServerSideError(c *C) {
653- session, err := newX509Session("subscriptionid", "", "West US")
654+ session, err := newX509Session("subscriptionid", "", "West US", NoRetryPolicy)
655 fixedResponse := x509Response{
656 StatusCode: http.StatusForbidden,
657 Body: []byte("Body"),
658@@ -244,7 +244,7 @@
659 version := "test-version"
660 requestBody := []byte("Request body")
661 requestContentType := "bogusContentType"
662- session, err := newX509Session(subscriptionID, "", "West US")
663+ session, err := newX509Session(subscriptionID, "", "West US", NoRetryPolicy)
664 c.Assert(err, IsNil)
665 // Record incoming requests, and have them return a given reply.
666 fixedResponse := x509Response{
667@@ -269,7 +269,7 @@
668 }
669
670 func (suite *x509SessionSuite) TestPostReportsClientSideError(c *C) {
671- session, err := newX509Session("subscriptionid", "", "West US")
672+ session, err := newX509Session("subscriptionid", "", "West US", NoRetryPolicy)
673 msg := "could not dispatch request"
674 rigFailingDispatcher(fmt.Errorf(msg))
675
676@@ -281,7 +281,7 @@
677 }
678
679 func (suite *x509SessionSuite) TestPostReportsServerSideError(c *C) {
680- session, err := newX509Session("subscriptionid", "", "West US")
681+ session, err := newX509Session("subscriptionid", "", "West US", NoRetryPolicy)
682 fixedResponse := x509Response{
683 StatusCode: http.StatusForbidden,
684 Body: []byte("Body"),
685@@ -300,7 +300,7 @@
686 subscriptionID := "subscriptionID"
687 uri := "resource"
688 version := "test-version"
689- session, err := newX509Session(subscriptionID, "", "West US")
690+ session, err := newX509Session(subscriptionID, "", "West US", NoRetryPolicy)
691 c.Assert(err, IsNil)
692 // Record incoming requests, and have them return a given reply.
693 fixedResponse := x509Response{StatusCode: http.StatusOK}
694@@ -324,7 +324,7 @@
695 uri := "resource"
696 version := "test-version"
697 requestBody := []byte("Request body")
698- session, err := newX509Session(subscriptionID, "", "West US")
699+ session, err := newX509Session(subscriptionID, "", "West US", NoRetryPolicy)
700 c.Assert(err, IsNil)
701 // Record incoming requests, and have them return a given reply.
702 fixedResponse := x509Response{

Subscribers

People subscribed via source and target branches

to all changes: