Merge lp:~jtv/gwacl/factor-storage-requests into lp:gwacl

[Jeroen T. Vermeulen]

Factor out "perform HTTP request" method in StorageContext.

This combines a few tightly-related refactorings in the storage-API code (mostly in separate commits):

 * Move signRequest() into StorageAPI.

 * Move the signRequest() call out of addStandardheaders().

 * Fix up a few bits of dead code where call sites tried to check for unexpected HTTP statuses — after send() would already have returned an error.

 * Factor out a performRequest() method, with a parameter-object struct, to encapsulate the repeated code to perform HTTP requests.

The parameter-object initializer makes the calls to performRequest() rather verbose, but I think it's better than passing lots of positional arguments, especially when several are of the same type and easily confused. Personally I find named arguments preferable to positional ones once you have more than four of them. It would be possible to move some of the required fields into the call as positional parameters, but it would become either an arbitrary selection, or if we move all required fields, we'd be back to the long parameter list. In which case the parameter object would just make things more irregular so we'd want to get rid of it, and we'd have truly ridiculously long argument lists.

Some small changes in addition to strict refactoring:

 * Document that you can't change a request further after signing — I lost some time to that trap in Brisbane, since signing was hidden inside addStandardHeaders().

 * Added some more detail to error messages.

To help keep future development smooth I also added some sanity-checking to the parameter object. No, it's not technically needed, but it helped make sure that I didn't make any of the obvious mistakes during refactoring — without having to test the same things again for every call. We can take it out now if we want, but I figure it's an easy way to find out about mistakes sooner.

Jeroen

[lp:~jtv/gwacl/factor-storage-requests]

110. By Jeroen T. Vermeulen on 2013-04-30

Removed a bit of Java-like silliness.

[Gavin Panella (allenap)]

Looks good.

[1]

+func (params *requestParams) Check() {

This isn't used anywhere.

[2]

+func (context *StorageContext) performRequest(params requestParams) ...

So, does this mean that a copy of requestParams gets passed in? Does
that mean that copies of params' fields are made too? Body is an
io.Reader, so does that mean it's always a pointer/reference type, and
so only the pointer is copied? Same with Deserializer.

[3]

Is it worth keeping send() distinct from performRequest()?

review: Approve

[Julian Edwards (julian-edwards)]

Very nice change, thanks Jeroen.

75 + methods := map[string]bool{"GET": true, "PUT": true, "POST": true, "DELETE": true}
76 + if _, ok := methods[params.Method]; !ok {

This could possibly be the best 2 lines of Go I've ever seen :)

81 +// performRequest issues an HTTP request to Azure.
82 +func (context *StorageContext) performRequest(params requestParams) (*http.Response, error) {

Make sure you take a pointer for params here, we don't want to copy them. Been doing Python lately? :)
Also you forgot to call Check().

On another note, I wish there were a way of linking the doc for params into the doc for performRequest. Param descriptions are nowhere near the function :( This is a huge problem with godoc (I believe I contributed that bit to Gripes).

141 + _, err := context.performRequest(requestParams{
142 + Method: "GET",
143 + URL: uri,
144 + APIVersion: "2012-02-12",
145 + Result: &containers,
146 + ExpectedStatus: http.StatusOK,
147 + })

You're right - for long lists of params this style is hugely better than positional args. I really miss named parameters in Go :(

BTW I have an idea for error handling in the library. I will email the team about it so we don't cloud the MP here.

cheers

review: Approve

[lp:~jtv/gwacl/factor-storage-requests]

111. By Jeroen T. Vermeulen on 2013-05-01

Review change: forgot to call requestParams.Check().

[Jeroen T. Vermeulen (jtv)]

> [1]
>
> +func (params *requestParams) Check() {
>
> This isn't used anywhere.

Thanks for spotting that. I added the one call that was supposed to be there, right at the top of performRequest().

> [2]
>
> +func (context *StorageContext) performRequest(params requestParams) ...
>
> So, does this mean that a copy of requestParams gets passed in? Does
> that mean that copies of params' fields are made too? Body is an
> io.Reader, so does that mean it's always a pointer/reference type, and
> so only the pointer is copied? Same with Deserializer.

That's right. The io.Reader and Deserializer types are interfaces that wrap pointers, so we get to court disaster with Go's fragile type system; there isn't much we can do about that.

The requestParams object, on the other hand, is passed by value. I feel this is appropriate for an object that doesn't go through a series of stateful changes.

That may sound strange. You may think "but passing a pointer is more efficient, right?" But even if it's not particularly relevant when speaking http[s] across the internet, that's not necessarily the case, for fairly complicated reasons. I'll try to explain these not because they matter much here, but because they may put a new perspective on other situations you come across in the future.

To begin with, a smart compiler can detect that the function does not modify the parameter, and so pass it by reference if it that seems more efficient. Or it may pass parts of the object in registers, instead of writing to memory and possibly pushing data out of cache in the process. The trade-off for how to pass the parameter could shift depending on architecture, targeted CPU model, estimated cache pressure, prevalent optimization goals, changes in the compiler, profiling feedback... More than you'd care to tweak your code for every time.

The key thing is that the compiler has to know exactly what goes on with the object. If you pass an object by value, it's easy to see that no changes to the object will "flow back" to the caller. That may also allow the compiler to prove that the caller won't modify the object either, and this gives it the freedom to pass by reference in more situations. Similarly, it's hard to registerize (parts of) the object if there may be pointers to the object through which it may be modified, or through which it should be possible to observe changes. Whenever the compiler is not sure about these things (or when it can't be bothered to find out), it does the safe thing which is also the inefficient thing — with pass-by-reference, that means treating every potentially mutable item as location in memory instead of as a value that can be optimized in all sorts of ways.

Usually it's easy to see whether there may be other references to an object, but this "aliasing problem" as it's called is not generally computable.

So be kind to your compiler. Don't micro-manage it. Trust it, and give it the information it needs to do its job well. You do that by minimizing the scope of what "can" happen to an item. Copy by value where you can. In languages that have it, avoid pointer arithmetic. Try to avoid creating pointers to objects as ...

[Jeroen T. Vermeulen (jtv)]

> 75 + methods := map[string]bool{"GET": true, "PUT": true, "POST":
> true, "DELETE": true}
> 76 + if _, ok := methods[params.Method]; !ok {
>
> This could possibly be the best 2 lines of Go I've ever seen :)

Thank you!

It means “if params.Method not in {"GET", "PUT", "POST", "DELETE"}:”

Really goes to show how Go was designed to put the fun back in programming. Expressing this complex problem reasonably well kept me amused for a while.

Oh, you wanted fun while *reading* the code? Too bad. Put on some music or something.

> 81 +// performRequest issues an HTTP request to Azure.
> 82 +func (context *StorageContext) performRequest(params requestParams)
> (*http.Response, error) {
>
> Make sure you take a pointer for params here, we don't want to copy them.
> Been doing Python lately? :)
> Also you forgot to call Check().

Yes, I forgot to call Check(), but too late! Gavin spotted that in his review 9—10 hours earlier. Sometimes you do need to refresh merge proposals in your browser. :)

The copying is deliberate, harmless, and beneficial. See my explanation to Gavin above.

.

> On another note, I wish there were a way of linking the doc for params into
> the doc for performRequest. Param descriptions are nowhere near the function
> :( This is a huge problem with godoc (I believe I contributed that bit to
> Gripes).

Actually, I don't think I have that particular point there. Might be worth having a separate section about godoc, if you care to provide the material. I haven't built up much experience with godoc myself, because I haven't seen any use for it yet. From my point of view it's just been a roundabout way of reading a function's comment without seeing its body.

[Julian Edwards (julian-edwards)]

FWIW I think you are giving Go's compiler far too much benefit. Why risk it
not getting optimised when passing a pointer will definitely not copy
anything. And who's to say it won't optimise that, too?

[Jeroen T. Vermeulen (jtv)]

> FWIW I think you are giving Go's compiler far too much benefit. Why risk it
> not getting optimised when passing a pointer will definitely not copy
> anything. And who's to say it won't optimise that, too?

How much experience do you have with compiler back-ends? Compilers improve over time, and absent a very direct immediate need you're better off letting them optimize your code better in the future, than trying to clamp down on every risk of something not getting optimized now — and interfering with both future maintenance and future optimization. This is no different from when some people kept using "goto" because loops in the new structured-programming style were slower. Soon the compilers were tiling, fusing, fissioning, strip-mining, unrolling, and parallellizing the structured loops to stupendous effect... but skipping the parts with goto in them because those were icky and potentially hard to figure out.

Also, if you study compiler optimization you will find that passing a pointer definitely can cause excessive copying, and other costs, because of the aliasing problem. It just doesn't fit into a naïve, 1970s mental model of how compiled code works. Go may be in many ways a 1970s language, but much of this is now standard fare even for a young compiler. Plus there's a gcc front-end for Go, and gcc definitely has mature optimizations.

Where do the costs come from? Seemingly everywhere. Every time the compiler wants to access a variable, it needs to re-read that variable from memory — unless it can rule out aliasing, in which case it can registerize. Reloading the variable on demand every time is a form of copying, without the efficiencies of a full object copy. And if the access changes the variable, the same goes for writing the new value back to memory afterwards. The register allocator gets less freedom to make optimal choices. Trivial operations can't be optimized away if they put an object through intermediate states that may be observed through aliasing. The scheduler is much more constrained, which also eats away at the benefits you get from inlining. Escape analysis is less successful, so more objects need to be allocated on the heap that could have been allocated on the stack and/or registerized. More objects on the heap means more work for the garbage collector, more lock contention in the memory allocator, less cache locality.

You ask who's to say that the compiler won't optimise away pass-by-reference. Nothing obscures the compiler's view of what happens to an object quite like passing its address around. That's what usually creates aliases. It's harder for a compiler (and as I said, technically incomputable) to prove that aliasing is harmless in a given case, than it is to optimize away an object copy in cases where there is no aliasing in the first place. The two are not symmetrical.

Maybe it's best if I give you an example. Imagine you're the compiler. Consider this pseudocode. (If it looks silly, imagine it being the outcome of earlier optimization stages such as inlining. Imagine the computations being more interesting. Imagine much of this happening in a performance-critical loop. Any...

[Julian Edwards (julian-edwards)]

So what you're saying is that Go is missing const pointer args ;)

[Julian Edwards (julian-edwards)]

OK I'll stop trolling now!

=== modified file 'storage_base.go'
--- storage_base.go	2013-04-30 09:51:53 +0000
+++ storage_base.go	2013-05-01 05:20:37 +0000
@@ -16,6 +16,7 @@
     "crypto/hmac"
     "crypto/sha256"
     "encoding/base64"
+    "errors"
     "fmt"
     "io"
     "io/ioutil"
@@ -40,12 +41,6 @@
     "Range",
 }
 
-// Add the Authorization: header to a Request.
-func signRequest(req *http.Request, account_name, account_key string) {
-    header := composeAuthHeader(req, account_name, account_key)
-    req.Header.Set("Authorization", header)
-}
-
 // Calculate the value required for an Authorization header.
 func composeAuthHeader(req *http.Request, account_name, account_key string) string {
     signable := composeStringToSign(req, account_name)
@@ -184,12 +179,12 @@
     req.Header.Set("Date", now)
 }
 
-// Convenience function to add mandatory headers required in every request.
-func addStandardHeaders(req *http.Request, account, key, ms_version string) {
-    addVersionHeader(req, ms_version)
-    addDateHeader(req)
-    addContentHeaders(req)
-    signRequest(req, account, key)
+// signRequest adds the Authorization: header to a Request.
+// Don't make any further changes to the request before sending it, or the
+// signature will not be valid.
+func (context *StorageContext) signRequest(req *http.Request) {
+    header := composeAuthHeader(req, context.Account, context.Key)
+    req.Header.Set("Authorization", header)
 }
 
 // StorageContext keeps track of the mandatory parameters required to send a
@@ -215,6 +210,53 @@
     Deserialize([]byte) error
 }
 
+// requestParams is a Parameter Object for performRequest().
+type requestParams struct {
+    Method         string       // HTTP method, e.g. "GET" or "PUT".
+    URL            string       // Resource locator, e.g. "http://example.com/my/resource".
+    Body           io.Reader    // Optional request body.
+    APIVersion     string       // Expected Azure API version, e.g. "2012-02-12".
+    Result         Deserializer // Optional object to parse API response into.
+    ExpectedStatus HTTPStatus   // Expected response status, e.g. http.StatusOK.
+}
+
+// Check performs a basic sanity check on the request.  This will only catch
+// a few superficial problems that you can spot at compile time, to save a
+// debugging cycle for the most basic mistakes.
+func (params *requestParams) Check() {
+    const panicPrefix = "invalid request: "
+    if params.Method == "" {
+        panic(errors.New(panicPrefix + "HTTP method not specified"))
+    }
+    if params.URL == "" {
+        panic(errors.New(panicPrefix + "URL not specified"))
+    }
+    if params.APIVersion == "" {
+        panic(errors.New(panicPrefix + "API version not specified"))
+    }
+    if params.ExpectedStatus == 0 {
+        panic(errors.New(panicPrefix + "expected HTTP status not specified"))
+    }
+    methods := map[string]bool{"GET": true, "PUT": true, "POST": true, "DELETE": true}
+    if _, ok := methods[params.Method]; !ok {
+        panic(fmt.Errorf(panicPrefix+"unsupported HTTP method '%s'", params.Method))
+    }
+}
+
+// performRequest issues an HTTP request to Azure.
+func (context *StorageContext) performRequest(params requestParams) (*http.Response, error) {
+    params.Check()
+    req, err := http.NewRequest(params.Method, params.URL, params.Body)
+    if err != nil {
+        return nil, err
+    }
+    addVersionHeader(req, params.APIVersion)
+    addDateHeader(req)
+    addContentHeaders(req)
+    context.signRequest(req)
+    return context.send(req, params.Result, params.ExpectedStatus)
+}
+
 // Send a request to the storage service and process the response.
 // The "res" parameter is typically an XML struct that will deserialize the
 // raw XML into the struct data.  The http Response object is returned.
@@ -224,7 +266,7 @@
 // is an HTTPError, the request response is also returned.  In other error
 // cases, the returned response may be the one received from the server or
 // it may be nil.
-func (context *StorageContext) send(req *http.Request, res Deserializer, expected_status int) (*http.Response, error) {
+func (context *StorageContext) send(req *http.Request, res Deserializer, expected_status HTTPStatus) (*http.Response, error) {
     client := context.getClient()
     resp, err := client.Do(req)
     if err != nil {
@@ -233,7 +275,7 @@
 
     var data []byte
 
-    if resp.StatusCode != expected_status {
+    if resp.StatusCode != int(expected_status) {
         if resp.Body != nil {
             data, err = ioutil.ReadAll(resp.Body)
             if err != nil {
@@ -250,7 +292,7 @@
         return resp, nil
     }
 
-    // TODO: deserialize response headers into the "res" object.
+    // TODO: Also deserialize response headers into the "res" object.
     data, err = ioutil.ReadAll(resp.Body)
     if err != nil {
         msg := fmt.Errorf("failed to read response data: %s", err)
@@ -290,22 +332,21 @@
 // subsequent calls to get additional batches of the same result, pass the
 // NextMarker from the previous call's result.
 func (context *StorageContext) getListContainersBatch(marker string) (*ContainerEnumerationResults, error) {
-    uri := context.getAccountURL()
-    uri = addURLQueryParams(uri, "comp", "list")
+    uri := addURLQueryParams(context.getAccountURL(), "comp", "list")
     if marker != "" {
         uri = addURLQueryParams(uri, "marker", marker)
     }
-    req, err := http.NewRequest("GET", uri, nil)
-    if err != nil {
-        return nil, err
-    }
-    addStandardHeaders(req, context.Account, context.Key, "2012-02-12")
     containers := ContainerEnumerationResults{}
-    _, err = context.send(req, &containers, http.StatusOK)
+    _, err := context.performRequest(requestParams{
+        Method:         "GET",
+        URL:            uri,
+        APIVersion:     "2012-02-12",
+        Result:         &containers,
+        ExpectedStatus: http.StatusOK,
+    })
     if err != nil {
-        return nil, err
+        return nil, fmt.Errorf("request for containers list failed: %v", err)
     }
-
     return &containers, nil
 }
 
@@ -348,22 +389,25 @@
 // subsequent calls to get additional batches of the same result, pass the
 // NextMarker from the previous call's result.
 func (context *StorageContext) getListBlobsBatch(container, marker string) (*BlobEnumerationResults, error) {
-    uri := context.getContainerURL(container)
-    uri = addURLQueryParams(
-        uri,
+    uri := addURLQueryParams(
+        context.getContainerURL(container),
         "restype", "container",
         "comp", "list")
     if marker != "" {
         uri = addURLQueryParams(uri, "marker", marker)
     }
-    req, err := http.NewRequest("GET", uri, nil)
+    blobs := BlobEnumerationResults{}
+    _, err := context.performRequest(requestParams{
+        Method:         "GET",
+        URL:            uri,
+        APIVersion:     "2012-02-12",
+        Result:         &blobs,
+        ExpectedStatus: http.StatusOK,
+    })
     if err != nil {
-        return nil, err
+        return nil, fmt.Errorf("request for blobs list failed: %v", err)
     }
-    addStandardHeaders(req, context.Account, context.Key, "2012-02-12")
-    blob := &BlobEnumerationResults{}
-    _, err = context.send(req, blob, http.StatusOK)
-    return blob, err
+    return &blobs, err
 }
 
 // ListBlobs requests from the API a list of blobs in a container.
@@ -402,16 +446,17 @@
 // Send a request to the storage service to create a new container.  If the
 // request fails, error is non-nil.
 func (context *StorageContext) CreateContainer(container string) error {
-    uri := context.getContainerURL(container)
-    uri = addURLQueryParams(uri, "restype", "container")
-    req, err := http.NewRequest("PUT", uri, nil)
+    uri := addURLQueryParams(
+        context.getContainerURL(container),
+        "restype", "container")
+    _, err := context.performRequest(requestParams{
+        Method:         "PUT",
+        URL:            uri,
+        APIVersion:     "2012-02-12",
+        ExpectedStatus: http.StatusCreated,
+    })
     if err != nil {
-        return err
-    }
-    addStandardHeaders(req, context.Account, context.Key, "2012-02-12")
-    _, errmsg := context.send(req, nil, http.StatusCreated)
-    if errmsg != nil {
-        return errmsg
+        return fmt.Errorf("failed to create container %s: %v", container, err)
     }
     return nil
 }
@@ -419,16 +464,15 @@
 // Send a request to create a space to upload a blob.  Note that this does not
 // do the uploading, it just makes an empty file.
 func (context *StorageContext) PutBlob(container, filename string) error {
-    // TODO: Add blobtype header.
-    uri := context.getFileURL(container, filename)
-    req, err := http.NewRequest("PUT", uri, nil)
+    _, err := context.performRequest(requestParams{
+        Method: "PUT",
+        // TODO: Add blobtype header.
+        URL:            context.getFileURL(container, filename),
+        APIVersion:     "2012-02-12",
+        ExpectedStatus: http.StatusCreated,
+    })
     if err != nil {
-        return err
-    }
-    addStandardHeaders(req, context.Account, context.Key, "2012-02-12")
-    _, errmsg := context.send(req, nil, http.StatusCreated)
-    if errmsg != nil {
-        return errmsg
+        return fmt.Errorf("failed to create blob %s: %v", filename, err)
     }
     return nil
 }
@@ -436,23 +480,21 @@
 // Send a request to fetch the list of blocks that have been uploaded as part
 // of a block blob.
 func (context *StorageContext) GetBlockList(container, filename string) (*GetBlockList, error) {
-    uri := context.getFileURL(container, filename)
-    uri = addURLQueryParams(
-        uri,
+    uri := addURLQueryParams(
+        context.getFileURL(container, filename),
         "comp", "blocklist",
         "blocklisttype", "all")
-
-    req, err := http.NewRequest("GET", uri, nil)
-    if err != nil {
-        return nil, err
-    }
-    addStandardHeaders(req, context.Account, context.Key, "2012-02-12")
     bl := GetBlockList{}
-    _, errmsg := context.send(req, &bl, http.StatusOK)
-    if errmsg != nil {
-        return nil, errmsg
+    _, err := context.performRequest(requestParams{
+        Method:         "GET",
+        URL:            uri,
+        APIVersion:     "2012-02-12",
+        Result:         &bl,
+        ExpectedStatus: http.StatusOK,
+    })
+    if err != nil {
+        return nil, fmt.Errorf("request for block list in file %s failed: %v", filename, err)
     }
-
     return &bl, nil
 }
 
@@ -460,83 +502,72 @@
 // data block to upload.
 func (context *StorageContext) PutBlock(container, filename, id string, data io.Reader) error {
     base64_id := base64.StdEncoding.EncodeToString([]byte(id))
-    uri := context.getFileURL(container, filename)
-    uri = addURLQueryParams(
-        uri,
+    uri := addURLQueryParams(
+        context.getFileURL(container, filename),
         "comp", "block",
         "blockid", base64_id)
-    req, err := http.NewRequest("PUT", uri, data)
-    if err != nil {
-        return err
-    }
-    addStandardHeaders(req, context.Account, context.Key, "2012-02-12")
-    resp, err := context.send(req, nil, http.StatusCreated)
-    if err != nil {
-        return err
-    }
-    if resp.StatusCode != http.StatusCreated {
-        return fmt.Errorf("failed to put block: %s", resp.Status)
+    _, err := context.performRequest(requestParams{
+        Method:         "PUT",
+        URL:            uri,
+        Body:           data,
+        APIVersion:     "2012-02-12",
+        ExpectedStatus: http.StatusCreated,
+    })
+    if err != nil {
+        return fmt.Errorf("failed to put block %s for file %s: %v", id, filename, err)
     }
     return nil
 }
 
 // Send a request to piece together blocks into a list that specifies a blob.
 func (context *StorageContext) PutBlockList(container, filename string, blocklist *BlockList) error {
-    uri := context.getFileURL(container, filename)
-    uri = addURLQueryParams(uri, "comp", "blocklist")
+    uri := addURLQueryParams(
+        context.getFileURL(container, filename),
+        "comp", "blocklist")
     data, err := blocklist.Serialize()
     if err != nil {
         return err
     }
     data_reader := bytes.NewReader(data)
-    req, err := http.NewRequest("PUT", uri, data_reader)
-    if err != nil {
-        return err
-    }
-    addStandardHeaders(req, context.Account, context.Key, "2012-02-12")
-    resp, err := context.send(req, nil, http.StatusCreated)
-    if err != nil {
-        return err
-    }
-    if resp.StatusCode != http.StatusCreated {
-        return fmt.Errorf("failed to put blocklist: %s", resp.Status)
+
+    _, err = context.performRequest(requestParams{
+        Method:         "PUT",
+        URL:            uri,
+        Body:           data_reader,
+        APIVersion:     "2012-02-12",
+        ExpectedStatus: http.StatusCreated,
+    })
+    if err != nil {
+        return fmt.Errorf("failed to put blocklist for file %s: %v", filename, err)
     }
     return nil
 }
 
 // Delete the specified blob from the given container.
 func (context *StorageContext) DeleteBlob(container, filename string) error {
-    uri := context.getFileURL(container, filename)
-    req, err := http.NewRequest("DELETE", uri, nil)
-    if err != nil {
-        return err
-    }
-    addStandardHeaders(req, context.Account, context.Key, "2012-02-12")
-    resp, err := context.send(req, nil, http.StatusAccepted)
+    _, err := context.performRequest(requestParams{
+        Method:         "DELETE",
+        URL:            context.getFileURL(container, filename),
+        APIVersion:     "2012-02-12",
+        ExpectedStatus: http.StatusAccepted,
+    })
     // TODO Handle a 404 with an <Error>BlobNotFound response body silently.
     if err != nil {
-        return err
-    }
-    if resp.StatusCode != http.StatusAccepted {
-        return fmt.Errorf("failed to delete blob: %s", resp.Status)
+        return fmt.Errorf("failed to delete blob %s: %v", filename, err)
     }
     return nil
 }
 
 // Get the specified blob from the given container.
 func (context *StorageContext) GetBlob(container, filename string) (io.ReadCloser, error) {
-    uri := context.getFileURL(container, filename)
-    req, err := http.NewRequest("GET", uri, nil)
+    response, err := context.performRequest(requestParams{
+        Method:         "GET",
+        URL:            context.getFileURL(container, filename),
+        APIVersion:     "2012-02-12",
+        ExpectedStatus: http.StatusOK,
+    })
     if err != nil {
-        return nil, err
-    }
-    addStandardHeaders(req, context.Account, context.Key, "2012-02-12")
-    resp, errmsg := context.send(req, nil, http.StatusOK)
-    if errmsg != nil {
-        return nil, errmsg
-    }
-    if resp.StatusCode != http.StatusOK {
-        return nil, fmt.Errorf("failed to get blob: %s", resp.Status)
-    }
-    return resp.Body, nil
+        return nil, fmt.Errorf("failed to get blob %s: %v", filename, err)
+    }
+    return response.Body, nil
 }

=== modified file 'storage_base_test.go'
--- storage_base_test.go	2013-04-30 08:42:44 +0000
+++ storage_base_test.go	2013-05-01 05:20:37 +0000
@@ -180,7 +180,8 @@
     c.Assert(req.Header.Get("Authorization"), Equals, "")
 
     key := base64.StdEncoding.EncodeToString([]byte("dummykey"))
-    signRequest(req, "myname", key)
+    context := StorageContext{client: nil, Account: "myname", Key: key}
+    context.signRequest(req)
 
     expected := "SharedKey myname:Xf9hWQ99mM0IyEOL6rNeAUdTQlixVqiYnt2TpLCCpY0="
     c.Assert(req.Header.Get("Authorization"), Equals, expected)