Implementing HTTP retries for the SDK

[hiranya911]

Implementing HTTP retries for the SDK. This will:

1. Retry once for all low-level errors
2. Retry up to 4 times for HTTP 500 and 503 errors (with exponential backoff)
3. Respect the HTTP Retry-After header when available

Resolves #237

[rockwotj]

These changes LGTM, but I'm not sold we should always retry 500s. These are not always completely safe AFAIK in the RTDB. They are mostly and in the majority of cases, so I'm torn. Ideally people mostly only see 503s these days

[hiranya911]

According to https://cloud.google.com/apis/design/errors#handling_errors clients should retry on 500 and 503 errors. I do understand that RTDB APIs are slightly different from other cloud REST APIs. But I suppose we can try this out in the wild, and if it becomes a problem use a different retry config just for the RTDB client (the API as implemented here supports it).

[rockwotj]

That sounds good to me @hiranya911

[ghost]

I am not finding raise_on_status as an argument to Retry object defined in version 2.21.0 of requests module (most recent public version). Perhaps you are getting some other version? This is causing an error during import of firebase_admin.admin on my setup.

[hiranya911]

retry.Retry is a class defined in urllib3. It has supported raise_on_status since v1.15 (released in 2016): urllib3/urllib3@9f48d26

You need to upgrade urllib3 to something more recent.

[ghost]

Your code reaches into a copy of urllib3 that's bundled with requests. It's not possible to upgrade that independently, AFAIK. I am using the newest requests (2.21.0) and it contains this issue.

[hiranya911]

This has been developed and tested on latest requests. I double checked just now to make sure:

>>> import requests
>>> requests.__version__
'2.21.0'
>>> import firebase_admin
>>> firebase_admin.__version__
'2.15.1'
>>> from firebase_admin import db
>>>
>>> from requests.packages.urllib3.util import retry
>>> print(retry.Retry.__doc__)
 Retry configuration.

    Each retry attempt will create a new Retry object with updated values, so
    they can be safely reused.
...
    :param bool raise_on_status: Similar meaning to ``raise_on_redirect``:
        whether we should raise an exception, or return a response,
        if status falls in ``status_forcelist`` range and retries have
        been exhausted.
...

requests does not ship urllib3 with it (as far as I can tell). It just re-exports whatever it can find already installed in the system: https://github.com/requests/requests/blob/v2.21.0/requests/packages.py

[ghost]

requests has urllib3>=1.21.1,<1.25 as a requirement (in setup.py), and that range of versions will do what we want. However, my build process is using pip to construct zipfile modules (because appengine limitations) and when pip is finished with requests, the packages subdirectory inside has an obsolete version of urllib3 captured into the zip. I've since learned that is coming from the virtualenv being used for building, even though I've added urllib3 to my requirements.txt, and pip finds and downloads urllib3 1.24, but that is not what gets captured into the zipfile holding requests!

If your code used from urllib3.util import retry and specified urllib3 as a dependancy, then all would be well. Since we might hope that requests will cleanup and remove the packages hack someday, maybe that would be a net improvement regardless?

Thanks for your help today.

[hiranya911]

That sounds like a reasonable change to make. Although I'm a little concerned about declaring a dependency on urllib3. We had to stop declaring requests as a dependency to work around a bug related to Firestore and pip. I wonder if adding urllib3 to our setup file will cause issues. But I can certainly experiment with that option and see if it's possible.