-
Notifications
You must be signed in to change notification settings - Fork 873
Multi part download: Stop queueing more tasks if in progress one fails #4228
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
base: gcbeatty/refactor2
Are you sure you want to change the base?
Conversation
sdk/src/Services/S3/Custom/Transfer/Internal/MultipartDownloadManager.cs
Outdated
Show resolved
Hide resolved
sdk/test/Services/S3/UnitTests/Custom/MultipartDownloadManagerTests.cs
Outdated
Show resolved
Hide resolved
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.
Pull request overview
This PR fixes a bug where multipart downloads would continue queuing new parts even after an earlier part had failed, causing unnecessary network requests and delayed error reporting. The fix adds early cancellation logic using a ContinueWith callback on each download task to immediately cancel the internal CancellationTokenSource when any part fails, preventing the loop from queuing additional parts.
Key Changes
- Adds a
ContinueWithcallback to each download task that cancels the internal token on first failure, stopping the queueing loop early - Improves exception capture to prioritize the original failure exception over cascading cancellation exceptions
- Adds comprehensive unit test that validates parts stop being queued after a failure (max 6 parts instead of all 10)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| sdk/test/Services/S3/UnitTests/Custom/MultipartDownloadManagerTests.cs | Adds new test StartDownloadsAsync_PartFailsDuringDownload_StopsQueuingNewParts to verify that when Part 3 fails, the system stops queuing additional parts instead of continuing through all parts |
| sdk/src/Services/S3/Custom/Transfer/Internal/MultipartDownloadManager.cs | Wraps the download task creation loop in try-catch for OperationCanceledException, adds ContinueWith callback to cancel internal token on task failure, improves exception handling to preserve original exception, and adds error logging to discovery methods |
sdk/test/Services/S3/UnitTests/Custom/MultipartDownloadManagerTests.cs
Outdated
Show resolved
Hide resolved
sdk/src/Services/S3/Custom/Transfer/Internal/MultipartDownloadManager.cs
Outdated
Show resolved
Hide resolved
sdk/src/Services/S3/Custom/Transfer/Internal/MultipartDownloadManager.cs
Outdated
Show resolved
Hide resolved
sdk/src/Services/S3/Custom/Transfer/Internal/MultipartDownloadManager.cs
Outdated
Show resolved
Hide resolved
d2c51c7 to
8778d34
Compare
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.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
sdk/src/Services/S3/Custom/Transfer/Internal/MultipartDownloadManager.cs
Outdated
Show resolved
Hide resolved
sdk/src/Services/S3/Custom/Transfer/Internal/MultipartDownloadManager.cs
Outdated
Show resolved
Hide resolved
sdk/test/Services/S3/UnitTests/Custom/MultipartDownloadManagerTests.cs
Outdated
Show resolved
Hide resolved
sdk/src/Services/S3/Custom/Transfer/Internal/MultipartDownloadManager.cs
Show resolved
Hide resolved
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.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
sdk/src/Services/S3/Custom/Transfer/Internal/MultipartDownloadManager.cs
Outdated
Show resolved
Hide resolved
sdk/test/Services/S3/UnitTests/Custom/MultipartDownloadManagerTests.cs
Outdated
Show resolved
Hide resolved
sdk/src/Services/S3/Custom/Transfer/Internal/MultipartDownloadManager.cs
Outdated
Show resolved
Hide resolved
sdk/src/Services/S3/Custom/Transfer/Internal/MultipartDownloadManager.cs
Outdated
Show resolved
Hide resolved
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
update tests add back tests copilot comments add more tests add back test add back test move tests add test back
remove downloadexception field code updates code updates code updates
d664bd7 to
c2594c7
Compare
Description
Previously if an error occurred when for a part, we would still queue the next parts, even though a previous part failed.
This would result in the behavior where, part 2 failed, queues part 3, part 3 fails, queues part 4, etc..
This change adds a check so that if a part fails with some exception, it will cancel the internal cancellation token to prevent other parts from getting queued.
Now the new behavior is part 2 failed, dont queue part 3.
Motivation and Context
#3806
Testing
Before my fix the unit test failed on
after my fix, the test passes.
Types of changes
Checklist
License