[storage] remove dead retry

[Stevenjin8]

Description

This PR is a little different from #25017 and the question is around the retry mechanism in

azure-sdk-for-python/sdk/storage/azure-storage-blob/azure/storage/blob/aio/_download_async.py

Lines 335 to 391 in 33acf79

	retry_active = True
	retry_total = 3
	while retry_active:
	try:
	location_mode, response = await self._clients.blob.download(
	range=range_header,
	range_get_content_md5=range_validation,
	validate_content=self._validate_content,
	data_stream_total=None,
	download_stream_current=0,
	**self._request_options)
	# Check the location we read from to ensure we use the same one
	# for subsequent requests.
	self._location_mode = location_mode
	# Parse the total file size and adjust the download size if ranges
	# were specified
	self._file_size = parse_length_from_content_range(response.properties.content_range)
	# Remove any extra encryption data size from blob size
	self._file_size = adjust_blob_size_for_encryption(self._file_size, self._encryption_data)
	if self._end_range is not None:
	# Use the length unless it is over the end of the file
	self.size = min(self._file_size, self._end_range - self._start_range + 1)
	elif self._start_range is not None:
	self.size = self._file_size - self._start_range
	else:
	self.size = self._file_size
	retry_active = False
	except HttpResponseError as error:
	if self._start_range is None and error.response.status_code == 416:
	# Get range will fail on an empty file. If the user did not
	# request a range, do a regular get request in order to get
	# any properties.
	try:
	_, response = await self._clients.blob.download(
	validate_content=self._validate_content,
	data_stream_total=0,
	download_stream_current=0,
	**self._request_options)
	retry_active = False
	except HttpResponseError as error:
	process_storage_error(error)
	# Set the download size to empty
	self.size = 0
	self._file_size = 0
	else:
	process_storage_error(error)
	except ClientPayloadError as error:
	retry_total -= 1
	if retry_total <= 0:
	raise ServiceResponseError(error, error=error)
	await asyncio.sleep(1)

which was added in #18164. Notably, this retry mechanism catches ClientPayloadError. A few months later, #20888 changed AioHttpTransportResponse.load_body to catch ClientPayloadError's and raise them as IncompleteReadError. So right now, the retry mechanism isn't doing anything (also when i remove it, all tests pass).

So far, this is looking a lot like #25017. But the crucial difference lies in where the body of the response is loaded. In #25017, the body of the response was loaded outside the pipeline in the process_content function. For the async clients, the data is loaded inside the pipeline.

The pipeline is defined in

azure-sdk-for-python/sdk/storage/azure-storage-blob/azure/storage/blob/_shared/base_client_async.py

Lines 92 to 106 in 33acf79

	QueueMessagePolicy(),
	config.headers_policy,
	config.proxy_policy,
	config.user_agent_policy,
	StorageContentValidation(),
	StorageRequestHook(**kwargs),
	self._credential_policy,
	ContentDecodePolicy(response_encoding="utf-8"),
	AsyncRedirectPolicy(**kwargs),
	StorageHosts(hosts=self._hosts, **kwargs), # type: ignore
	config.retry_policy,
	config.logging_policy,
	AsyncStorageResponseHook(**kwargs),
	DistributedTracingPolicy(**kwargs),
	HttpLoggingPolicy(**kwargs),

The AsyncStorageResponseHook is after config.retry_policy. If we look at AsyncStorageResponseHook.send, we see

azure-sdk-for-python/sdk/storage/azure-storage-blob/azure/storage/blob/_shared/policies_async.py

Line 65 in 33acf79

await response.http_response.load_body()

meaning that the body of the response is loaded later in the pipeline than the retry policy. Thus, if loading the body of the response raises a IncompleteReadError, the pipeline will retry the request. If the retry policy runs out of retries, then it will raise a ServiceResponseError.

In other words, if we modify the #18164 's retry mechanism to catch ServiceResponseErrors instead of aiohttp.ClientPayloadErrors, we would be tripling the number of retries as @jalauzon-msft/@vincenttran-msft suggested earlier.

Given that, I think the best way forward is to revert #18164 's retry mechanism because we already have a working retry mechanism and the current code shouldn't ever run.

All SDK Contribution checklist:

• The pull request does not introduce [breaking changes]
• CHANGELOG is updated for new features, bug fixes or other significant changes.
• I have read the contribution guidelines.

General Guidelines and Best Practices

• Title of the pull request is clear and informative.
• There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

• Pull request includes test coverage for the included changes.

[azure-sdk]

API change check

API changes are not detected in this pull request.

[Stevenjin8]

currently working on tests. They are only passing when playing live. I think something funky is going on with vcr

[Stevenjin8]

currently working on tests. They are only passing when playing live. I think something funky is going on with vcr

We could also mark the test as live-only.

[jalauzon-msft]

Hi @Stevenjin8, thanks for the investigation / explanation about why this retry code is no longer needed. I think you are correct in that the response streaming case does not really come into play here in async since load_body is in the response hook. What really sold it for me was the fact that in the process_content function for async we are just grabbing the repsonse body that has already been loaded:

azure-sdk-for-python/sdk/storage/azure-storage-blob/azure/storage/blob/aio/_download_async.py

Line 35 in e789c73

content = data.response.body()

So here we are really only wrapping the calls to _client.download in retry which is not needed since the normal download calls will go through our regular pipeline with retries.

For me, I think there is still a question of whether load_body will actually perform retries if response streaming is taking place. It is in the pipeline, yes, but I'm not sure the response streaming requests will actually go through the pipeline and be retried. We may end up needing to wrap load_body in retries. But this does not affect this PR so we should be good.