-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Blob][Fileshare] Ensured that download fails if blob/file modified mid download #19276
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
@@ -162,6 +163,9 @@ def _download_chunk(self, chunk_start, chunk_end): | |||
download_stream_current=self.progress_total, | |||
**self.request_options | |||
) | |||
if response.properties.etag != self.etag: | |||
raise ValueError("The file has been modified while downloading.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check with @annatisch if this is right exception type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In dotnet we throw this and provide more data than just message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed with @xiafu-msft and we are now throwing ResourceModifiedError
as per blob natively does in this case.
…id download (Azure#19276) * Ensured that download fails if blob/file modified mid download * fixed linter * ResourceModifiedError
remove global config (Azure#19276)
Blobs:
Currently in blobs we lock on etag, and we try to let the user override this by passing
If-Match:'*'
but the logic was incorrect which just led to the ability to modify a blob during the second chunked call, otherwise it would still fail. I have removed this extra logic completely.Fileshare:
Added locking on etag - previously there was no etag locking.