Add retries in case of a socket hangup on HTTP

[inlined]

Fixes #2254

Should hopefully catch the issue customers are seeing in Google Cloud Functions where google APIs get into a permanent failed state.

[stephenplusplus]

Thanks for this! I can't think of a reason why we shouldn't retry on this type of error, but before we merge, I just want to make sure we're consistent between our different API request transport layers (we have 3):

• common.Service (REST): (covered in this PR)
• commonGrpc.Service (gRPC): We would want to find a similar solution in this function: https://github.com/GoogleCloudPlatform/google-cloud-node/blob/40abb0b0fc3868900cd342c84d13069c128cc5a5/packages/common-grpc/src/service.js#L602 -- AFAIK, gRPC itself does not do any retries.
• GAX: @jmuk @landrito - does GAX do any retries for this error?

And one more thought, since we're re-thinking this-- are there any other similar errors we should retry on?

[s2young]

Is there any way to npm install @google-cloud/storage at this commit? I cannot figure out how to get this into my firebase functions project. Any help would be greatly appreciated! I've tried every recommended syntax on the google machine and nothing is working.

[stephenplusplus]

Sorry @s2young, there isn't a way to use this with npm due to our mono-repo for multiple packages.

cc @swcloud @lukesneeringer -- any thoughts on if we should retry on this error, and if there are any others?

[s2young]

This module is pretty much unusable on Firebase Functions without some change - at least with respect to interacting with Cloud Storage. I can confirm that after the timeout period the only way to get my functions working is to redeploy them. Thanks!

[s2young]

Sorry to pester, but this issue is REALLY a problem for me. What about a way to programmatically reconnect? A setting I can pass in when instantiating the @google-cloud/storage object? Anything?!

[stephenplusplus]

The solution for this goes a bit deeper into a dependency module, retry-request. That is the module that the changes in this PR affect. It will take a custom "shouldRetryRequest" function, pass into it the HTTP response object, and use the returned value to determine if it should retry the request.

There is a bit of complexity to how this is solved. Currently, retry-request immediately errors out on ones such as "ECONNRESET" or other low-level HTTP/DNS errors (here). It won't even give the user the chance to retry on that. If we want to support retrying from these types of errors, that gives us 2 options:

1. Return the error to the shouldRetryRequest function, allowing the user to specify they want retries on ECONNRESET errors, or
2. Automatically retry using an exponential backoff strategy on these low-level errors, considering it a "probably what you want" best practice. These retries would not count against the maxRetries value the user provides.

Any thoughts are welcome.

[stephenplusplus]

cc @callmehiphop

[s2young]

My personal vote is for number 2 (and soon). I can continue developing the solution I'm working on, but cannot launch anything until this is resolved. Thanks!

[s2young]

With respect to option 1, why would a user not want to retry? The user is interfacing with a method that is supposed to upload/setMetadata/getMetadata/etc. What about going with option 2, and rejecting the promise after a user-configurable timeout? A timeout option could give the user flexibility enough to move on with their program, while having the nature of the failure in the rejection.

[stephenplusplus]

I prefer that option as well. I put out a release with this change. If you un- and re-install @google-cloud/storage, you should have the new code. Can you let me know if it works?

[stephenplusplus]

I'm going to consider this resolved after the fix mentioned above. Just let me know if it's not working, and we'll figure out a new way to attack this. Thanks @inlined and @s2young for reporting the issue and your patience while we figure(d) this out.

[s2young]

So far so good! Thanks so much!

[s2young]

Ok, maybe I spoke too soon. It's definitely better but I'm still seeing the ECONNRESET. My scenario is the following:

1. Client application uploads files to Firebase Storage.
2. Firebase Function listens for changes to Firebase Storage, downloads and resizes images where needed, and updates Firebase real-time db with some data about the uploaded files.
3. Client monitors changes to Firebase Db to figure out when all the processing is complete.

In my testing I'm uploading 16 images, and sometimes it works for all 16. But equally often, I'll only get 14 or 15 out of 16 properly updated in the Db.

[stephenplusplus]

Thanks for letting me know. The change to retry-request allows 3 total failures. Upping that number might be a naive solution, so I'm going to start with what was described in #2254, and modify as necessary to try to reproduce and hopefully become more aware of the underlying issue.

[stephenplusplus]

Carried the conversation over to #2254.