diff --git a/.kokoro/build.sh b/.kokoro/build.sh index 69a53cf0..85db638c 100755 --- a/.kokoro/build.sh +++ b/.kokoro/build.sh @@ -41,7 +41,7 @@ python3 -m pip install --upgrade --quiet nox python3 -m nox --version # If this is a continuous build, send the test log to the FlakyBot. -# See https://github.com/googleapis/repo-automation-bots/tree/master/packages/flakybot. +# See https://github.com/googleapis/repo-automation-bots/tree/main/packages/flakybot. if [[ $KOKORO_BUILD_ARTIFACTS_SUBDIR = *"continuous"* ]]; then cleanup() { chmod +x $KOKORO_GFILE_DIR/linux_amd64/flakybot diff --git a/.kokoro/test-samples-impl.sh b/.kokoro/test-samples-impl.sh index 311a8d54..8a324c9c 100755 --- a/.kokoro/test-samples-impl.sh +++ b/.kokoro/test-samples-impl.sh @@ -80,7 +80,7 @@ for file in samples/**/requirements.txt; do EXIT=$? # If this is a periodic build, send the test log to the FlakyBot. - # See https://github.com/googleapis/repo-automation-bots/tree/master/packages/flakybot. + # See https://github.com/googleapis/repo-automation-bots/tree/main/packages/flakybot. if [[ $KOKORO_BUILD_ARTIFACTS_SUBDIR = *"periodic"* ]]; then chmod +x $KOKORO_GFILE_DIR/linux_amd64/flakybot $KOKORO_GFILE_DIR/linux_amd64/flakybot diff --git a/CHANGELOG.md b/CHANGELOG.md index 8c63e9f2..15d89bd5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,14 @@ [1]: https://pypi.org/project/google-resumable-media/#history +### [2.0.1](https://www.github.com/googleapis/google-resumable-media-python/compare/v2.0.0...v2.0.1) (2021-08-30) + + +### Bug Fixes + +* check if retry is allowed after retry wait calculation ([#258](https://www.github.com/googleapis/google-resumable-media-python/issues/258)) ([00ccf71](https://www.github.com/googleapis/google-resumable-media-python/commit/00ccf7120251d3899c8d0c2eccdf3b177b5b3742)) +* do not mark upload download instances invalid with retriable error codes ([#261](https://www.github.com/googleapis/google-resumable-media-python/issues/261)) ([a1c5f7d](https://www.github.com/googleapis/google-resumable-media-python/commit/a1c5f7d0e3ce48d8d6eb8aced31707a881f7ee96)) + ## [2.0.0](https://www.github.com/googleapis/google-resumable-media-python/compare/v2.0.0-b1...v2.0.0) (2021-08-19) @@ -218,7 +226,7 @@ might break the hypothetical usecase of downloading a blob marked with - Modify file not found test to look for the correct error message - Harden tests so they can run with debug logging statements - Add Appveyor support. ([#40](https://github.com/googleapis/google-resumable-media-python/pull/40)) -- Mark the version in `master` as `.dev1`. +- Mark the version in `main` as `.dev1`. ## 0.3.1 diff --git a/README.rst b/README.rst index a698bd8a..8feae1dd 100644 --- a/README.rst +++ b/README.rst @@ -31,4 +31,4 @@ License Apache 2.0 - See `the LICENSE`_ for more information. -.. _the LICENSE: https://github.com/googleapis/google-resumable-media-python/blob/master/LICENSE +.. _the LICENSE: https://github.com/googleapis/google-resumable-media-python/blob/main/LICENSE diff --git a/docs/conf.py b/docs/conf.py index 3896205f..c16787e3 100644 --- a/docs/conf.py +++ b/docs/conf.py @@ -64,7 +64,7 @@ # source_encoding = 'utf-8-sig' # The main toctree document. -master_doc = "index" +root_doc = "index" # General information about the project. project = u"google-resumable-media" @@ -266,7 +266,7 @@ # author, documentclass [howto, manual, or own class]). latex_documents = [ ( - master_doc, + root_doc, "google-resumable-media.tex", u"google-resumable-media Documentation", author, @@ -301,7 +301,7 @@ # (source start file, name, description, authors, manual section). man_pages = [ ( - master_doc, + root_doc, "google-resumable-media", u"google-resumable-media Documentation", [author], @@ -320,7 +320,7 @@ # dir menu entry, description, category) texinfo_documents = [ ( - master_doc, + root_doc, "google-resumable-media", u"google-resumable-media Documentation", author, diff --git a/google/_async_resumable_media/_upload.py b/google/_async_resumable_media/_upload.py index fc78b015..a25d4a3d 100644 --- a/google/_async_resumable_media/_upload.py +++ b/google/_async_resumable_media/_upload.py @@ -277,7 +277,7 @@ def _prepare_request(self, data, metadata, content_type): checksum_object = sync_helpers._get_checksum_object(self._checksum_type) - if checksum_object: + if checksum_object is not None: checksum_object.update(data) actual_checksum = sync_helpers.prepare_checksum_digest( checksum_object.digest() diff --git a/google/_async_resumable_media/requests/download.py b/google/_async_resumable_media/requests/download.py index 55ec5841..e88bac80 100644 --- a/google/_async_resumable_media/requests/download.py +++ b/google/_async_resumable_media/requests/download.py @@ -90,10 +90,7 @@ async def _write_to_stream(self, response): self._stream.write(chunk) local_checksum_object.update(chunk) - if expected_checksum is None: - return - - else: + if expected_checksum is not None: actual_checksum = sync_helpers.prepare_checksum_digest( checksum_object.digest() ) @@ -216,9 +213,7 @@ async def _write_to_stream(self, response): self._stream.write(chunk) checksum_object.update(chunk) - if expected_checksum is None: - return - else: + if expected_checksum is not None: actual_checksum = sync_helpers.prepare_checksum_digest( checksum_object.digest() ) diff --git a/google/resumable_media/_helpers.py b/google/resumable_media/_helpers.py index 6df8806e..a105d3c6 100644 --- a/google/resumable_media/_helpers.py +++ b/google/resumable_media/_helpers.py @@ -95,7 +95,8 @@ def require_status_code(response, status_codes, get_status_code, callback=do_not """ status_code = get_status_code(response) if status_code not in status_codes: - callback() + if status_code not in common.RETRYABLE: + callback() raise common.InvalidResponse( response, "Request failed with status code", @@ -192,17 +193,17 @@ def wait_and_retry(func, get_status_code, retry_strategy): else: return response - if not retry_strategy.retry_allowed(total_sleep, num_retries): - # Retries are exhausted and no acceptable response was received. - # Raise the retriable_error. - raise error - base_wait, wait_time = calculate_retry_wait( base_wait, retry_strategy.max_sleep, retry_strategy.multiplier ) - num_retries += 1 total_sleep += wait_time + + # Check if (another) retry is allowed. If retries are exhausted and + # no acceptable response was received, raise the retriable error. + if not retry_strategy.retry_allowed(total_sleep, num_retries): + raise error + time.sleep(wait_time) diff --git a/google/resumable_media/_upload.py b/google/resumable_media/_upload.py index 4e38bdd8..2855d199 100644 --- a/google/resumable_media/_upload.py +++ b/google/resumable_media/_upload.py @@ -289,7 +289,7 @@ def _prepare_request(self, data, metadata, content_type): raise TypeError("`data` must be bytes, received", type(data)) checksum_object = _helpers._get_checksum_object(self._checksum_type) - if checksum_object: + if checksum_object is not None: checksum_object.update(data) actual_checksum = _helpers.prepare_checksum_digest(checksum_object.digest()) metadata_key = _helpers._get_metadata_key(self._checksum_type) diff --git a/google/resumable_media/common.py b/google/resumable_media/common.py index d2c0be52..3e292122 100644 --- a/google/resumable_media/common.py +++ b/google/resumable_media/common.py @@ -158,9 +158,10 @@ def retry_allowed(self, total_sleep, num_retries): """Check if another retry is allowed. Args: - total_sleep (float): The amount of sleep accumulated by the caller. - num_retries (int): The number of retries already attempted by - the caller. + total_sleep (float): With another retry, the amount of sleep that + will be accumulated by the caller. + num_retries (int): With another retry, the number of retries that + will be attempted by the caller. Returns: bool: Indicating if another retry is allowed (depending on either diff --git a/google/resumable_media/requests/download.py b/google/resumable_media/requests/download.py index 40e15235..960d61a3 100644 --- a/google/resumable_media/requests/download.py +++ b/google/resumable_media/requests/download.py @@ -106,9 +106,7 @@ def _write_to_stream(self, response): self._stream.write(chunk) local_checksum_object.update(chunk) - if expected_checksum is None: - return - else: + if expected_checksum is not None: actual_checksum = _helpers.prepare_checksum_digest(checksum_object.digest()) if actual_checksum != expected_checksum: msg = _CHECKSUM_MISMATCH.format( @@ -242,9 +240,7 @@ def _write_to_stream(self, response): checksum_object.update(chunk) response._content_consumed = True - if expected_checksum is None: - return - else: + if expected_checksum is not None: actual_checksum = _helpers.prepare_checksum_digest(checksum_object.digest()) if actual_checksum != expected_checksum: diff --git a/owlbot.py b/owlbot.py index 0cf20644..e9ed0b7c 100644 --- a/owlbot.py +++ b/owlbot.py @@ -21,4 +21,16 @@ r'value: "docs-staging-v2-staging"' ) + +# Remove the replacements below once https://github.com/googleapis/synthtool/pull/1188 is merged + +# Update googleapis/repo-automation-bots repo to main in .kokoro/*.sh files +s.replace(".kokoro/*.sh", "repo-automation-bots/tree/master", "repo-automation-bots/tree/main") + +s.replace( + "docs/conf.py", + "master_doc", + "root_doc", +) + s.shell.run(["nox", "-s", "blacken"], hide_output=False) diff --git a/setup.py b/setup.py index 228071be..b1ee5de8 100644 --- a/setup.py +++ b/setup.py @@ -35,7 +35,7 @@ setuptools.setup( name='google-resumable-media', - version = "2.0.0b1", + version = "2.0.1", description='Utilities for Google Media Downloads and Resumable Uploads', author='Google Cloud Platform', author_email='googleapis-publisher@google.com', diff --git a/tests/unit/test__helpers.py b/tests/unit/test__helpers.py index dce99b04..498ca290 100644 --- a/tests/unit/test__helpers.py +++ b/tests/unit/test__helpers.py @@ -125,6 +125,28 @@ def test_failure_with_callback(self): assert error.args[3:] == status_codes callback.assert_called_once_with() + def test_retryable_failure_without_callback(self): + status_codes = (http.client.OK,) + retryable_responses = [ + _make_response(status_code) for status_code in common.RETRYABLE + ] + callback = mock.Mock(spec=[]) + for retryable_response in retryable_responses: + with pytest.raises(common.InvalidResponse) as exc_info: + _helpers.require_status_code( + retryable_response, + status_codes, + self._get_status_code, + callback=callback, + ) + + error = exc_info.value + assert error.response is retryable_response + assert len(error.args) == 4 + assert error.args[1] == retryable_response.status_code + assert error.args[3:] == status_codes + callback.assert_not_called() + class Test_calculate_retry_wait(object): @mock.patch("random.randint", return_value=125) @@ -337,7 +359,7 @@ def test_connection_import_error_failure(self, randint_mock, sleep_mock): @mock.patch("time.sleep") @mock.patch("random.randint") def test_retry_exceeds_max_cumulative(self, randint_mock, sleep_mock): - randint_mock.side_effect = [875, 0, 375, 500, 500, 250, 125, 0] + randint_mock.side_effect = [875, 0, 375, 500, 500, 250, 125] status_codes = ( http.client.SERVICE_UNAVAILABLE, @@ -346,7 +368,6 @@ def test_retry_exceeds_max_cumulative(self, randint_mock, sleep_mock): http.client.INTERNAL_SERVER_ERROR, http.client.SERVICE_UNAVAILABLE, http.client.BAD_GATEWAY, - http.client.GATEWAY_TIMEOUT, common.TOO_MANY_REQUESTS, ) responses = [_make_response(status_code) for status_code in status_codes] @@ -365,47 +386,119 @@ def raise_response(): assert ret_val.status_code == status_codes[-1] assert status_codes[-1] in common.RETRYABLE - assert func.call_count == 8 - assert func.mock_calls == [mock.call()] * 8 + assert func.call_count == 7 + assert func.mock_calls == [mock.call()] * 7 assert randint_mock.call_count == 7 assert randint_mock.mock_calls == [mock.call(0, 1000)] * 7 - assert sleep_mock.call_count == 7 + assert sleep_mock.call_count == 6 sleep_mock.assert_any_call(1.875) sleep_mock.assert_any_call(2.0) sleep_mock.assert_any_call(4.375) sleep_mock.assert_any_call(8.5) sleep_mock.assert_any_call(16.5) sleep_mock.assert_any_call(32.25) - sleep_mock.assert_any_call(64.125) + + @mock.patch("time.sleep") + @mock.patch("random.randint") + def test_retry_exceeds_max_retries(self, randint_mock, sleep_mock): + randint_mock.side_effect = [875, 0, 375, 500, 500, 250, 125] + + status_codes = ( + http.client.SERVICE_UNAVAILABLE, + http.client.GATEWAY_TIMEOUT, + common.TOO_MANY_REQUESTS, + http.client.INTERNAL_SERVER_ERROR, + http.client.SERVICE_UNAVAILABLE, + http.client.BAD_GATEWAY, + common.TOO_MANY_REQUESTS, + ) + responses = [_make_response(status_code) for status_code in status_codes] + + def raise_response(): + raise common.InvalidResponse(responses.pop(0)) + + func = mock.Mock(side_effect=raise_response) + + retry_strategy = common.RetryStrategy(max_retries=6) + try: + _helpers.wait_and_retry(func, _get_status_code, retry_strategy) + except common.InvalidResponse as e: + ret_val = e.response + + assert ret_val.status_code == status_codes[-1] + assert status_codes[-1] in common.RETRYABLE + + assert func.call_count == 7 + assert func.mock_calls == [mock.call()] * 7 + + assert randint_mock.call_count == 7 + assert randint_mock.mock_calls == [mock.call(0, 1000)] * 7 + + assert sleep_mock.call_count == 6 + sleep_mock.assert_any_call(1.875) + sleep_mock.assert_any_call(2.0) + sleep_mock.assert_any_call(4.375) + sleep_mock.assert_any_call(8.5) + sleep_mock.assert_any_call(16.5) + sleep_mock.assert_any_call(32.25) + + @mock.patch("time.sleep") + @mock.patch("random.randint") + def test_retry_zero_max_retries(self, randint_mock, sleep_mock): + randint_mock.side_effect = [875, 0, 375] + + status_codes = ( + http.client.SERVICE_UNAVAILABLE, + http.client.GATEWAY_TIMEOUT, + common.TOO_MANY_REQUESTS, + ) + responses = [_make_response(status_code) for status_code in status_codes] + + def raise_response(): + raise common.InvalidResponse(responses.pop(0)) + + func = mock.Mock(side_effect=raise_response) + + retry_strategy = common.RetryStrategy(max_retries=0) + try: + _helpers.wait_and_retry(func, _get_status_code, retry_strategy) + except common.InvalidResponse as e: + ret_val = e.response + + assert func.call_count == 1 + assert func.mock_calls == [mock.call()] * 1 + assert ret_val.status_code == status_codes[0] + + assert randint_mock.call_count == 1 + assert sleep_mock.call_count == 0 @mock.patch("time.sleep") @mock.patch("random.randint") def test_retry_exceeded_reraises_connection_error(self, randint_mock, sleep_mock): randint_mock.side_effect = [875, 0, 375, 500, 500, 250, 125] - responses = [requests.exceptions.ConnectionError] * 8 + responses = [requests.exceptions.ConnectionError] * 7 func = mock.Mock(side_effect=responses, spec=[]) retry_strategy = common.RetryStrategy(max_cumulative_retry=100.0) with pytest.raises(requests.exceptions.ConnectionError): _helpers.wait_and_retry(func, _get_status_code, retry_strategy) - assert func.call_count == 8 - assert func.mock_calls == [mock.call()] * 8 + assert func.call_count == 7 + assert func.mock_calls == [mock.call()] * 7 assert randint_mock.call_count == 7 assert randint_mock.mock_calls == [mock.call(0, 1000)] * 7 - assert sleep_mock.call_count == 7 + assert sleep_mock.call_count == 6 sleep_mock.assert_any_call(1.875) sleep_mock.assert_any_call(2.0) sleep_mock.assert_any_call(4.375) sleep_mock.assert_any_call(8.5) sleep_mock.assert_any_call(16.5) sleep_mock.assert_any_call(32.25) - sleep_mock.assert_any_call(64.125) def _make_response(status_code):