Merge lp:~jtv/gwacl/close-body into lp:gwacl

Proposed by Jeroen T. Vermeulen on 2013-07-30
Status: Merged
Approved by: Jeroen T. Vermeulen on 2013-07-30
Approved revision: 207
Merged at revision: 205
Proposed branch: lp:~jtv/gwacl/close-body
Merge into: lp:gwacl
Diff against target: 269 lines (+82/-70)
5 files modified
storage_base.go (+19/-28)
utils.go (+13/-0)
utils_test.go (+46/-0)
x509dispatcher.go (+4/-1)
x509dispatcher_test.go (+0/-41)
To merge this branch: bzr merge lp:~jtv/gwacl/close-body
Reviewer Review Type Date Requested Status
Julian Edwards (community) Approve on 2013-07-30
Gavin Panella 2013-07-30 Approve on 2013-07-30
Review via email: mp+177511@code.launchpad.net

Commit message

Always close an http.Response.Body when done (if there is one).

Description of the change

Discussion of various problems in Go's https implementation brought up this bug: https://code.google.com/p/go/issues/detail?id=3514

This mentions that an http response body must always be closed after use. I don't think we realized this, and certainly gwacl wasn't doing it. This branch fixes that for gwacl. It's a good thing we encapsulated so well: there were only two calls to fix.

There was a complication: in one case, just one case, the storage code actually read the response body outside of the send() method. But since that was the only part of the response it was interested in, I just gave it the body as a string. This does mean that we read the whole blob into memory, unfortunately. Something to keep in mind for later. Alternatives I considered were:

(a) Making the caller responsible for closing the body, but only in the one code path where this is a problem. But that's far too baroque, and it'll make future mistakes more likely rather than less likely.

(b) Specialize the code more, so this case would be a very different code path. It would have been the right thing, but taken much more work: send() is a bit of Swiss Army Knife with excessive caller-selected complexity. That antipattern is often hard to fix because it digs its hooks into so many places.

(c) Make all call sites responsible for closing the body. But there are a lot of them, and forgetting one won't lead to a clear or reliable failure.

Jeroen

To post a comment you must log in.
Gavin Panella (allenap) :
review: Approve
Julian Edwards (julian-edwards) wrote :

Looks good!

> + return nil, fmt.Errorf("failed to read response data: %v", err)

Some context in the error message would be good - you might be staring at this
in a log one night and wondering WTF....

review: Approve
Jeroen T. Vermeulen (jtv) wrote :

Thanks. I doubt there's much context we can usefully add here, without adding the kitchen sink. If the response body is unreadable, that's a systemic problem such as a network failure or a bug in Go's standard library — it shouldn't matter much which exact request triggered it.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'storage_base.go'
2--- storage_base.go 2013-07-29 00:43:12 +0000
3+++ storage_base.go 2013-07-30 07:01:25 +0000
4@@ -258,7 +258,9 @@
5 }
6
7 // performRequest issues an HTTP request to Azure.
8-func (context *StorageContext) performRequest(params requestParams) (*http.Response, error) {
9+//
10+// It returns the response body contents.
11+func (context *StorageContext) performRequest(params requestParams) ([]byte, error) {
12 params.Check()
13 req, err := http.NewRequest(params.Method, params.URL, params.Body)
14 if err != nil {
15@@ -279,52 +281,41 @@
16
17 // Send a request to the storage service and process the response.
18 // The "res" parameter is typically an XML struct that will deserialize the
19-// raw XML into the struct data. The http Response object is returned.
20+// raw XML into the struct data. The http Response object's body is returned.
21 //
22 // If the response's HTTP status code is not the same as "expectedStatus"
23-// then an HTTPError will be returned as the error. When the returned error
24-// is an HTTPError, the request response is also returned. In other error
25-// cases, the returned response may be the one received from the server or
26-// it may be nil.
27-func (context *StorageContext) send(req *http.Request, res Deserializer, expectedStatus HTTPStatus) (*http.Response, error) {
28+// then an HTTPError will be returned as the error.
29+func (context *StorageContext) send(req *http.Request, res Deserializer, expectedStatus HTTPStatus) ([]byte, error) {
30 client := context.getClient()
31 resp, err := client.Do(req)
32 if err != nil {
33 return nil, err
34 }
35
36- var data []byte
37+ body, err := readAndClose(resp.Body)
38+ if err != nil {
39+ return nil, fmt.Errorf("failed to read response data: %v", err)
40+ }
41
42 if resp.StatusCode != int(expectedStatus) {
43- if resp.Body != nil {
44- data, err = ioutil.ReadAll(resp.Body)
45- if err != nil {
46- return resp, err
47- }
48- }
49- msg := newHTTPError(resp.StatusCode, data, "Azure request failed")
50- return resp, msg
51+ msg := newHTTPError(resp.StatusCode, body, "Azure request failed")
52+ return body, msg
53 }
54
55 // If the caller didn't supply an object to deserialize the message into
56 // then just return.
57 if res == nil {
58- return resp, nil
59+ return body, nil
60 }
61
62 // TODO: Also deserialize response headers into the "res" object.
63- data, err = ioutil.ReadAll(resp.Body)
64- if err != nil {
65- msg := fmt.Errorf("failed to read response data: %s", err)
66- return resp, msg
67- }
68- err = res.Deserialize(data)
69+ err = res.Deserialize(body)
70 if err != nil {
71 msg := fmt.Errorf("Failed to deserialize data: %s", err)
72- return resp, msg
73+ return body, msg
74 }
75
76- return resp, nil
77+ return body, nil
78 }
79
80 // getAccountURL returns the base URL for the context's storage account.
81@@ -639,17 +630,17 @@
82
83 // Get the specified blob from the given container.
84 func (context *StorageContext) GetBlob(container, filename string) (io.ReadCloser, error) {
85- response, err := context.performRequest(requestParams{
86+ body, err := context.performRequest(requestParams{
87 Method: "GET",
88 URL: context.GetFileURL(container, filename),
89 APIVersion: "2012-02-12",
90 ExpectedStatus: http.StatusOK,
91 })
92 if err != nil {
93- msg := fmt.Sprintf("failed to get blob %s: ", filename)
94+ msg := fmt.Sprintf("failed to get blob %q: ", filename)
95 return nil, extendError(err, msg)
96 }
97- return response.Body, nil
98+ return ioutil.NopCloser(bytes.NewBuffer(body)), nil
99 }
100
101 type SetContainerACLRequest struct {
102
103=== modified file 'utils.go'
104--- utils.go 2013-04-30 08:18:38 +0000
105+++ utils.go 2013-07-30 07:01:25 +0000
106@@ -5,6 +5,8 @@
107
108 import (
109 "fmt"
110+ "io"
111+ "io/ioutil"
112 "net/url"
113 )
114
115@@ -22,6 +24,17 @@
116 }
117 }
118
119+// readAndClose reads and closes the given ReadCloser.
120+//
121+// Trying to read from a nil simply returns nil, no error.
122+func readAndClose(stream io.ReadCloser) ([]byte, error) {
123+ if stream == nil {
124+ return nil, nil
125+ }
126+ defer stream.Close()
127+ return ioutil.ReadAll(stream)
128+}
129+
130 // addURLQueryParams adds query parameters to a URL (and escapes as needed).
131 // Parameters are URL, [key, value, [key, value, [...]]].
132 // The original URL must be correct, i.e. it should parse without error.
133
134=== modified file 'utils_test.go'
135--- utils_test.go 2013-04-30 08:18:38 +0000
136+++ utils_test.go 2013-07-30 07:01:25 +0000
137@@ -4,8 +4,11 @@
138 package gwacl
139
140 import (
141+ "io"
142+ "io/ioutil"
143 . "launchpad.net/gocheck"
144 "net/url"
145+ "strings"
146 )
147
148 type UtilsSuite struct{}
149@@ -25,6 +28,49 @@
150 PanicMatches, "'[.][.]' is not allowed")
151 }
152
153+func (*UtilsSuite) TestReadAndCloseReturnsEmptyStringForNil(c *C) {
154+ data, err := readAndClose(nil)
155+ c.Assert(err, IsNil)
156+ c.Check(string(data), Equals, "")
157+}
158+
159+func (*UtilsSuite) TestReadAndCloseReturnsContents(c *C) {
160+ content := "Stream contents."
161+ stream := ioutil.NopCloser(strings.NewReader(content))
162+
163+ data, err := readAndClose(stream)
164+ c.Assert(err, IsNil)
165+
166+ c.Check(string(data), Equals, content)
167+}
168+
169+// fakeStream is a very simple fake implementation of io.ReadCloser. It
170+// acts like an empty stream, but it tracks whether it's been closed yet.
171+type fakeStream struct {
172+ closed bool
173+}
174+
175+func (stream *fakeStream) Read([]byte) (int, error) {
176+ if stream.closed {
177+ panic("Read() from closed fakeStream")
178+ }
179+ return 0, io.EOF
180+}
181+
182+func (stream *fakeStream) Close() error {
183+ stream.closed = true
184+ return nil
185+}
186+
187+func (*UtilsSuite) TestReadAndCloseCloses(c *C) {
188+ stream := &fakeStream{}
189+
190+ _, err := readAndClose(stream)
191+ c.Assert(err, IsNil)
192+
193+ c.Check(stream.closed, Equals, true)
194+}
195+
196 type TestAddURLQueryParams struct{}
197
198 var _ = Suite(&TestAddURLQueryParams{})
199
200=== modified file 'x509dispatcher.go'
201--- x509dispatcher.go 2013-07-25 22:18:32 +0000
202+++ x509dispatcher.go 2013-07-30 07:01:25 +0000
203@@ -82,10 +82,13 @@
204 if err != nil {
205 return nil, err
206 }
207+ if httpResponse.Body != nil {
208+ defer httpResponse.Body.Close()
209+ }
210
211 response := &x509Response{}
212 response.StatusCode = httpResponse.StatusCode
213- response.Body, err = ioutil.ReadAll(httpResponse.Body)
214+ response.Body, err = readAndClose(httpResponse.Body)
215 if err != nil {
216 return nil, err
217 }
218
219=== modified file 'x509dispatcher_test.go'
220--- x509dispatcher_test.go 2013-07-19 14:44:21 +0000
221+++ x509dispatcher_test.go 2013-07-30 07:01:25 +0000
222@@ -66,47 +66,6 @@
223 c.Check(httpRequest.BodyContent, HasLen, 0)
224 }
225
226-/*
227-func (*x509DispatcherSuite) TestGetFollowsRedirects(c *C) {
228- redirectPath := "/redirect/path"
229- httpRequests := make(chan *Request, 10)
230- locationHeaders := http.Header{}
231- locationHeaders.Add("Location", redirectPath)
232- server := makeRecordingHTTPServer(httpRequests, http.StatusTemporaryRedirect, nil, locationHeaders)
233- defer server.Close()
234- // No real certificate needed since we're testing on http, not https.
235- session, err := newX509Session("subscriptionid", "")
236- c.Assert(err, IsNil)
237- path := "/foo/bar"
238- version := "test-version"
239- request := newX509RequestGET(server.URL+path, version)
240-
241- _, err = performX509Request(session, request)
242- c.Check(err, ErrorMatches, ".*stopped after [0-9]* redirects")
243-
244- var httpRequest *Request
245- // The original GET request has been performed.
246- select {
247- case httpRequest = <-httpRequests:
248- default:
249- c.Error("The original request has not been performed.")
250- }
251- c.Check(httpRequest.Method, Equals, "GET")
252- c.Check(httpRequest.Header[http.CanonicalHeaderKey("X-Ms-Version")], DeepEquals, []string{version})
253- c.Check(httpRequest.URL.String(), Equals, path)
254-
255- for i := 0; i < 10; i++ {
256- select {
257- case httpRequest := <-httpRequests:
258- c.Check(httpRequest.Method, Equals, "GET")
259- c.Check(httpRequest.URL.String(), Equals, redirectPath)
260- default:
261- c.Error("No redirection has happened.")
262- }
263- }
264-}
265-*/
266-
267 func (*x509DispatcherSuite) TestPostRequestDoesHTTPPOST(c *C) {
268 httpRequests := make(chan *Request, 1)
269 requestBody := []byte{1, 2, 3}

Subscribers

People subscribed via source and target branches