http response from google may not always has 'x-goog-hash' header

[teddybearz]

The http response from google may not always has 'x-goog-hash' header. When it happens, the node process will exit.

Need fix like:

diff --git a/node_modules/gcloud/lib/storage/file.js b/node_modules/gcloud/lib/storage/file.js
index 8e1a26e..4aaec72 100644
--- a/node_modules/gcloud/lib/storage/file.js
+++ b/node_modules/gcloud/lib/storage/file.js
@@ -361,10 +361,12 @@
             var md5Fail = true;

             var hashes = {};
-            res.headers['x-goog-hash'].split(',').forEach(function(hash) {
-              var hashType = hash.split('=')[0];
-              hashes[hashType] = hash.substr(hash.indexOf('=') + 1);
-            });
+            if (res.headers && res.headers['x-goog-hash']) {
+              res.headers['x-goog-hash'].split(',').forEach(function(hash) {
+                var hashType = hash.split('=')[0];
+                hashes[hashType] = hash.substr(hash.indexOf('=') + 1);
+              });
+            }

             var remoteMd5 = hashes.md5;
             var remoteCrc = hashes.crc32c && hashes.crc32c.substr(4);

[stephenplusplus]

Did you run into a situation where it wasn't returned?

Google Cloud Storage stores MD5 hashes for all non-composite objects. CRC32Cs are available for all objects.

• https://cloud.google.com/storage/docs/reference-headers#xgooghash

[ryanseys]

Seems like a reasonable request for a sanity check.

[stephenplusplus]

Still would like to know if this can come up. Would affect more of our code, if we can't always trust that a CRC32C hash is available.

[teddybearz]

It happens reliably if you try to fetch an object right before the upload of the object is done.

[ryanseys]

Why would you try to fetch an object before it was uploaded? Can you provide a snippet of code that causes this issue?

[teddybearz]

It was not intentionally. The bug was triggered because of in my old code, I registered on the "finish" event instead of "complete" event. A normal writeablestream will emit 'finish' when the data has been flushed to the underlying system, apparently 'finish' and 'complete' have different semantic in your lib. 'finish' event doesn't mean the object is committed. There is no document about this 'complete" event and I have to dig into the lib code to find out that.

Nevertheless, the lib's robustness should not entirely depend on the well-behave of the backend service. A not-well-behaved service should not cause the lib user's process exits.

[stephenplusplus]

Regarding finish vs complete, it's just kind of hard to work with Node streams. You'll see various libraries using different terms for different reasons. We use complete to be consistent with the request library, which is a familiar term for many developers. That at least makes the transition a little less painful for most.

Regarding the header, it should still always be there. I don't think we need to defensively program against that, since that the code pasted in your initial post executes after we've received a response from an upload. I'm still uncertain how our library executed that code before the response came back. Code that you used showing that happening may help me understand better.

[teddybearz]

To reproduce that you just need to fetch the object right after get the 'finish' event during a stream upload.

fsStream.pipe(bucket.file(blobname).createWriteStream({ metadata: metadata} )).
.on('error', function(err) {
handleError(err, 'write');
})
.on('finish', function(ex) {
if (!fdCbCalled) {
fdCbCalled = true;
future.return();
}
});
future.wait();
fetchBlob(blobName);

50% of the time you will get a response without the 'x-goog-hash' header the lib code expects.

[stephenplusplus]

We merged a PR that will hopefully help with explaining we use complete and not finish. I believe that was one part of this issue, and the other was: in parallel, upload a file and read from it. @ryanseys did some testing and observed the API responds with a 404 error, however, our code doesn't account for this and continues on as if it's a successful response.

Specifically, we need to implement util.handleResp here to help determine the quality of the API response.

Thanks for catching this @teddybearz and for tracking down the problem @ryanseys! PR incoming :)