diff --git a/googleapiclient/http.py b/googleapiclient/http.py index 5aad076854..433148ca2a 100644 --- a/googleapiclient/http.py +++ b/googleapiclient/http.py @@ -97,10 +97,10 @@ def _should_retry_response(resp_status, content): if resp_status == _TOO_MANY_REQUESTS: return True - # For 403 errors, we have to check for the `reason` in the response to + # For 403 and 400 errors, we have to check for the `reason` in the response to # determine if we should retry. - if resp_status == http_client.FORBIDDEN: - # If there's no details about the 403 type, don't retry. + if resp_status in [http_client.FORBIDDEN, http_client.BAD_REQUEST]: + # If there's no details about the error, don't retry. if not content: return False @@ -137,10 +137,19 @@ def _should_retry_response(resp_status, content): LOGGER.warning("Invalid JSON content from response: %s", content) return False - LOGGER.warning('Encountered 403 Forbidden with reason "%s"', reason) - - # Only retry on rate limit related failures. - if reason in ("userRateLimitExceeded", "rateLimitExceeded"): + RETRYABLE_INFO = { + http_client.FORBIDDEN: { + "reasons": ("userRateLimitExceeded", "rateLimitExceeded"), + "message": 'Encountered 403 Forbidden with reason "%s"', + }, + http_client.BAD_REQUEST: { + "reasons": ("failedPrecondition", "preconditionFailed"), + "message": 'Encountered 400 Bad Request with reason "%s"', + }, + } + info = RETRYABLE_INFO[resp_status] + LOGGER.warning(info["message"], reason) + if reason in info["reasons"]: return True # Everything else is a success or non-retriable so break. diff --git a/tests/test_discovery.py b/tests/test_discovery.py index 6912783451..9c08c43908 100644 --- a/tests/test_discovery.py +++ b/tests/test_discovery.py @@ -495,12 +495,6 @@ def test_ResourceMethodParameters_zoo_animals_patch(self): self.assertEqual(parameters.enum_params, {}) -class Discovery(unittest.TestCase): - def test_discovery_http_is_closed(self): - http = HttpMock(datafile("malformed.json"), {"status": "200"}) - service = build("plus", "v1", credentials=mock.sentinel.credentials) - http.close.assert_called_once() - class DiscoveryErrors(unittest.TestCase): def test_tests_should_be_run_with_strict_positional_enforcement(self): @@ -1549,6 +1543,32 @@ def test_file_based_cache(self): class Discovery(unittest.TestCase): + @mock.patch("google.auth.default") + @mock.patch("httplib2.Http") + def test_discovery_http_is_closed(self, mock_http_class, mock_auth_default): + """Verify that the temporary HTTP client created by build() to fetch + the discovery document is closed after initialization, preventing + connection/socket leaks (googleapis/google-api-python-client#2757). + + When no http= is passed, build() internally creates a httplib2.Http() + instance to fetch the discovery document and must close it afterwards + to avoid connection/socket leaks. + """ + mock_http = mock_http_class.return_value + mock_http.request.return_value = ( + httplib2.Response({"status": "200"}), + read_datafile("plus.json", "rb"), + ) + mock_credentials = mock.create_autospec( + google.auth.credentials.Credentials, instance=True + ) + mock_credentials.universe_domain = "googleapis.com" + mock_auth_default.return_value = (mock_credentials, "test-project") + + build("plus", "v1", static_discovery=False) + + mock_http.close.assert_called_once() + def test_method_error_checking(self): self.http = HttpMock(datafile("plus.json"), {"status": "200"}) plus = build("plus", "v1", http=self.http, static_discovery=False) diff --git a/tests/test_http.py b/tests/test_http.py index 91f2c7ae52..8575304203 100644 --- a/tests/test_http.py +++ b/tests/test_http.py @@ -880,6 +880,20 @@ def test_media_io_base_download_unknown_media_size(self): } }""" +FAILED_PRECONDITION_RESPONSE = """{ + "error": { + "errors": [ + { + "domain": "global", + "reason": "failedPrecondition", + "message": "Precondition Failed" + } + ], + "code": 400, + "message": "Precondition Failed" + } +}""" + NOT_CONFIGURED_RESPONSE = """{ "error": { @@ -1058,6 +1072,34 @@ def test_no_retry_succeeds(self): self.assertEqual(0, len(sleeptimes)) + def test_retry_400_failed_precondition(self): + num_retries = 2 + resp_seq = [ + ({"status": "400"}, FAILED_PRECONDITION_RESPONSE), + ({"status": "200"}, "{}") + ] + http = HttpMockSequence(resp_seq) + model = JsonModel() + uri = "https://www.googleapis.com/someapi/v1/collection/?foo=bar" + method = "POST" + request = HttpRequest( + http, + model.response, + uri, + method=method, + body="{}", + headers={"content-type": "application/json"}, + ) + + sleeptimes = [] + request._sleep = lambda x: sleeptimes.append(x) + request._rand = lambda: 10 + + request.execute(num_retries=num_retries) + + self.assertEqual(1, len(sleeptimes)) + self.assertEqual(10 * 2 ** 1, sleeptimes[0]) + def test_no_retry_fails_fast(self): http = HttpMockSequence([({"status": "500"}, ""), ({"status": "200"}, "{}")]) model = JsonModel()