Storage Blob Async downloader ignores exceptions from chunks

[ian-young-sydney]

Package: azure-storage-blob
Version: 12.4.0
System: ubuntu-bionic-core-cloudimg-amd64 (container)
Python: 3.6

Describe the bug
I'm executing this code to simply download a file:

            file_client = file_system.get_file_client(full_path)
            with open(local_file, "wb") as my_file:
                download = await file_client.download_file()
                await download.readinto(my_file)

I'm expecting this to throw any exception is there is a problem during this process. 99.9% of the time it's perfectly fine, but after running the service using this code for days, I found that I'm getting the following exception:

ERROR:Task exception was never retrieved

future: <Task finished coro=<_AsyncChunkDownloader.process_chunk() done, defined at /usr/local/lib/python3.6/dist-packages/azure/storage/blob/aio/_download_async.py:53> exception=ResourceModifiedError('The condition specified using HTTP conditional header(s) is not met.\nRequestId:XXX\nTime:2020-10-06T03:09:34.2866006Z\nErrorCode:ConditionNotMet\nError:None',)>
	
Traceback (most recent call last):
  File "/usr/local/lib/python3.6/dist-packages/azure/storage/blob/aio/_download_async.py", line 101, in _download_chunk
    **self.request_options
  File "/usr/local/lib/python3.6/dist-packages/azure/storage/blob/_generated/aio/operations_async/_blob_operations_async.py", line 180, in download
    raise models.StorageErrorException(response, self._deserialize)
azure.storage.blob._generated.models._models_py3.StorageErrorException: Operation returned an invalid status 'The condition specified using HTTP conditional header(s) is not met.'

During handling of the above exception, another exception occurred:
	
Traceback (most recent call last):
  File "/usr/local/lib/python3.6/dist-packages/azure/storage/blob/aio/_download_async.py", line 55, in process_chunk
    chunk_data = await self._download_chunk(chunk_start, chunk_end - 1)
  File "/usr/local/lib/python3.6/dist-packages/azure/storage/blob/aio/_download_async.py", line 104, in _download_chunk
    process_storage_error(error)
  File "/usr/local/lib/python3.6/dist-packages/azure/storage/blob/_shared/response_handlers.py", line 147, in process_storage_error	
    raise error
azure.core.exceptions.ResourceModifiedError: The condition specified using HTTP conditional header(s) is not met.
Time:2020-10-06T03:09:34.2866006Z
ErrorCode:ConditionNotMet
Error:None

The problem appears to be in this code: "azure/storage/blob/aio/_download_async.py"

async def readinto(self, stream):
    ...
        while running_futures:
            # Wait for some download to finish before adding a new one
            _done, running_futures = await asyncio.wait(              <============== HERE
                running_futures, return_when=asyncio.FIRST_COMPLETED)
            try:
                next_chunk = next(dl_tasks)
            except StopIteration:
                break
            else:
                running_futures.add(asyncio.ensure_future(downloader.process_chunk(next_chunk)))

        if running_futures:
            # Wait for the remaining downloads to finish
            await asyncio.wait(running_futures)                              <============= HERE
    ...

For each of these calls to asyncio.wait() it should be check for exceptions like this:

    done, pending = await asyncio.wait(tasks)
    for future in done:
        try:
            result = future.result()   # To raise exceptions and get results
        except Exception as e:
            exceptions.append(e)

The result() function needs to be called on each future to ensure that any exceptions are received and dealt with properly... It might be simpler using asyncio.gather() on the "done" tasks, but both should work

See the asyncio Futures documentation here:
https://docs.python.org/3/library/asyncio-future.html
and similar issues:
https://stackoverflow.com/questions/44048536/python3-get-result-from-async-method

To Reproduce
Sadly you have to run it for many hours to reproduce. You can manually raise an exception into the code to reproduce the problem

Expected behavior
I would have expected the exception to be raised so that I can capture and retry appropriately.

Additional context
None

[amishra-dev]

@ian-young-sydney
Hi Ian,
This is good work, thanks. Since you have done the investigation in details, do you want to send us a PR? I am happy to get it reviewed and merged.
Let me know if you can else I will assign someone to do the work.

Thanks
Ashish

[ian-young-sydney]

Hi Ashish,

I don't really have the time to create a fix for this. Also not sure what tests need to be updated for this as I think you need to ensure these chunk exceptions are caught. I think it's best to assign someone to make the fix.

Thanks,
Ian.

[amishra-dev]

@ian-young-sydney
No worries, i will get the work scheduled at our end. We do appreciate the time you took to log the issues. Feel free to share any other feedback you have about the SDK with me.

Thanks,
Ashish

[ian-young-sydney]

@amishra-dev Is there any word on when this will be fixed? As I see it, this is a critical bug in a critical library. Downloading a file from Blob Storage is a pretty fundamental thing to be able to do. Right now I have no guarantee that the file I've downloaded is valid or not.

[tasherif-msft]

Hi @ian-young-sydney! I will investigate this bug and will give you an update/PR as soon as possible. We're very sorry for the delay.

[ian-young-sydney]

Thanks @tasherif-msft . Let us know when there are any updates.

[tasherif-msft]

Hi @ian-young-sydney, the fix has been merged and will be available in our next release :)
thank you so much for your patience!
Let me know if anything else comes up.

[ian-young-sydney]

Thanks @tasherif-msft . I can see the beta release out there and the code change looks fairly straight forward and low risk. I'm going to give that a test run. I think it will be obvious if the change is stable, but will take a couple of days of running to reproduce a similar exception. I'll let you know if I hit any problems.