fix: Adding ConnectionError to retry mechanism

[damgad]

As described in #558 current retry mechanism does not catch ConnectionError (and ConnectionResetError which inherits from it). That pull request fixes the issue by adding the python3 specific ConnectionError into the mechanism.

[dylancaponi]

We've noticed ConnectionResetError not being retried for BigQuery, Storage, and Redis / Memorystore clients. If this PR fixes any of those we'll be very grateful! Happens about once per day on all of our deployed cloud functions.

[dylancaponi]

Also noticed gzp, base64, sys, and http_client from six.moves are imported but unused.

[dylancaponi]

@damgad Any idea how these modifications could be made "in place" until this PR is merged?

[damgad]

@dylancaponi Sure, you can always monkeypatch the _retry_request function which handles retrying. There are multiple ways of doing that (one dirtier than another ;) ) Before importing the googleapiclient write that:

import googleapiclient.http
from googleapiclient.http import LOGGER, _ssl_SSLError, _should_retry_response

def _patched_retry_request(
    http, num_retries, req_type, sleep, rand, uri, method, *args, **kwargs
):

    resp = None
    content = None
    exception = None
    for retry_num in range(num_retries + 1):
        if retry_num > 0:
            # Sleep before retrying.
            sleep_time = rand() * 2 ** retry_num
            LOGGER.warning(
                "Sleeping %.2f seconds before retry %d of %d for %s: %s %s, after %s",
                sleep_time,
                retry_num,
                num_retries,
                req_type,
                method,
                uri,
                resp.status if resp else exception,
            )
            sleep(sleep_time)

        try:
            exception = None
            resp, content = http.request(uri, method, *args, **kwargs)
        # Retry on SSL errors and socket timeout errors.
        except _ssl_SSLError as ssl_error:
            exception = ssl_error
        except socket.timeout as socket_timeout:
            # It's important that this be before socket.error as it's a subclass
            # socket.timeout has no errorcode
            exception = socket_timeout
        except ConnectionError as connection_error:
            # Needs to be before socket.error as it's a subclass of
            # OSError (socket.error)
            exception = connection_error
        except socket.error as socket_error:
            # errno's contents differ by platform, so we have to match by name.
            if socket.errno.errorcode.get(socket_error.errno) not in {
                "WSAETIMEDOUT",
                "ETIMEDOUT",
                "EPIPE",
                "ECONNABORTED",
            }:
                raise
            exception = socket_error
        except httplib2.ServerNotFoundError as server_not_found_error:
            exception = server_not_found_error

        if exception:
            if retry_num == num_retries:
                raise exception
            else:
                continue

        if not _should_retry_response(resp.status, content):
            break

    return resp, content


googleapiclient.http._retry_request = _patched_retry_request

That will just overwrite the the function from the library. You can also do it in shorter way:

import googleapiclient.http
import inspect

exec(
    inspect.getsource(
        googleapiclient.http._retry_request
    ).replace("socket.timeout", "(socket.timeout, ConnectionError)"),
    googleapiclient.http.__dict__
)

I would also suggest to make sure that you've pinned the version of google-api-python-client in your requirements, as consequences of the automatic update may be unpredictable.

[mik-laj]

@busunkim96 Any progress? This causes problems for Apache Airflow. Apache Airflow is used by the Cloud Composer service.

[busunkim96]

Thanks for the patch @damgad!

[mik-laj]

@busunkim96 Hello. Thanks for accepting this change. Do you know when it will appear in the new release?

[dylancaponi]

@damgad We've implemented the shorter monkeypatch for a few weeks. It seems to retry the call in question but not reestablish the connection. So we just get connectionRetryError 3x now instead of once per call. Wondering if I implemented something wrong or misunderstood the purpose of this PR.

[damgad]

@dylancaponi Seems that you've correctly applied the patch and it works how it should work. However, the aim of the whole retry mechanism is to overcome single occurring errors - temporary issues that may disappear with the next try.

In your case, you should either investigate why ConnectionRetryError really happens and try to fix the root cause or you can also increase the num_retries parameter to some lucky, larger value. That may help if the error cause is temporary as the retry mechanism uses exponential backoff with each consecutive try.
But, if you increase the num_retries, you may also get ConnectionRetryError even more times.

[mik-laj]

A new release has been released that includes this fix.
#868