Multipart download refactor #4235
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request refactors the S3 multipart download functionality by consolidating the two-step download API (DiscoverDownloadStrategyAsync + StartDownloadsAsync) into a single unified method (StartDownloadAsync). This change eliminates resource leakage issues by managing HTTP slots and buffer capacity internally throughout the entire download lifecycle, rather than holding resources between separate method calls.
Key Changes
- API Consolidation: Combined
DiscoverDownloadStrategyAsyncandStartDownloadsAsyncinto a singleStartDownloadAsyncmethod that atomically performs discovery, processes Part 1, and starts background downloads for remaining parts - Naming Update: Renamed
DownloadDiscoveryResulttoDownloadResultto reflect the unified operation that includes both discovery and initial download - Test Updates: Updated all unit tests to use the new unified API, removing tests specific to the old two-method pattern and ensuring coverage of the new behavior
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| sdk/src/Services/S3/Custom/Transfer/Internal/IDownloadManager.cs | Updated interface to replace two methods with single StartDownloadAsync method; renamed DownloadDiscoveryResult to DownloadResult |
| sdk/src/Services/S3/Custom/Transfer/Internal/MultipartDownloadManager.cs | Refactored public API to StartDownloadAsync and created private helper methods PerformDiscoveryAsync and PerformDownloadsAsync for internal orchestration |
| sdk/src/Services/S3/Custom/Transfer/Internal/IPartDataHandler.cs | Updated PrepareAsync signature to accept DownloadResult instead of DownloadDiscoveryResult |
| sdk/src/Services/S3/Custom/Transfer/Internal/FilePartDataHandler.cs | Updated PrepareAsync implementation to use DownloadResult |
| sdk/src/Services/S3/Custom/Transfer/Internal/BufferedPartDataHandler.cs | Updated PrepareAsync implementation to use DownloadResult |
| sdk/src/Services/S3/Custom/Transfer/Internal/BufferedMultipartStream.cs | Simplified initialization to call single StartDownloadAsync method instead of two separate calls |
| sdk/src/Services/S3/Custom/Transfer/Internal/_async/MultipartDownloadCommand.async.cs | Updated to use unified StartDownloadAsync method with simplified logging |
| sdk/test/Services/S3/UnitTests/Custom/MultipartDownloadManagerTests.cs | Comprehensive test updates to use new API; removed obsolete tests; added inappropriate Microsoft.VisualBasic import |
| sdk/test/Services/S3/UnitTests/Custom/FilePartDataHandlerTests.cs | Updated all test instances to use DownloadResult with minor spacing issues |
| sdk/test/Services/S3/UnitTests/Custom/FilePartDataHandlerConcurrencyTests.cs | Updated to use DownloadResult |
| sdk/test/Services/S3/UnitTests/Custom/BufferedMultipartStreamTests.cs | Updated tests to mock unified API; reorganized test with incorrect region placement |
sdk/test/Services/S3/UnitTests/Custom/MultipartDownloadManagerTests.cs
Outdated
Show resolved
Hide resolved
sdk/test/Services/S3/UnitTests/Custom/FilePartDataHandlerTests.cs
Outdated
Show resolved
Hide resolved
sdk/test/Services/S3/UnitTests/Custom/FilePartDataHandlerConcurrencyTests.cs
Outdated
Show resolved
Hide resolved
sdk/src/Services/S3/Custom/Transfer/Internal/MultipartDownloadManager.cs
Show resolved
Hide resolved
sdk/src/Services/S3/Custom/Transfer/Internal/MultipartDownloadManager.cs
Show resolved
Hide resolved
sdk/test/Services/S3/UnitTests/Custom/FilePartDataHandlerTests.cs
Outdated
Show resolved
Hide resolved
GarrettBeatty
commented
Dec 12, 2025
| } | ||
|
|
||
| [TestMethod] | ||
| public async Task StartDownloadsAsync_SinglePart_DoesNotThrowOnCancellation() |
Contributor
Author
There was a problem hiding this comment.
not applicable anymore (it should throw cancellation), since everything happens in start download now
GarrettBeatty
force-pushed
the
gcbeatty/refactor2
branch
2 times, most recently
from
4a70062 to
b8d9f14
Compare
GarrettBeatty
commented
Dec 12, 2025
| #region InitializeAsync Tests - Single Part | ||
|
|
||
| [TestMethod] | ||
| public async Task InitializeAsync_SinglePart_UsesSinglePartHandler() |
Contributor
Author
There was a problem hiding this comment.
this isnt deleted, the order just got switched around by accident
GarrettBeatty
commented
Dec 12, 2025
|
|
||
|
|
||
| [TestMethod] | ||
| public async Task InitializeAsync_SinglePart_CallsStartDownloads() |
Contributor
Author
There was a problem hiding this comment.
this is still here just moved around
GarrettBeatty
commented
Dec 12, 2025
| #region InitializeAsync Tests - Multipart | ||
|
|
||
| [TestMethod] | ||
| public async Task InitializeAsync_Multipart_UsesMultipartHandler() |
Contributor
Author
There was a problem hiding this comment.
this wasnt deleted it was just moved around
GarrettBeatty
commented
Dec 12, 2025
| } | ||
|
|
||
| [TestMethod] | ||
| public async Task Discovery_HttpRequestAfterCapacityFails_ReleasesHttpSemaphore() |
Contributor
Author
There was a problem hiding this comment.
this test is still here just moved around
GarrettBeatty
force-pushed
the
gcbeatty/refactor2
branch
from
7c8cbfb to
5cf7817
Compare
update tests add back tests copilot comments add more tests add back test add back test move tests add test back
GarrettBeatty
force-pushed
the
gcbeatty/refactor2
branch
from
5cf7817 to
6fa4e5a
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Refactor multi part download manager so that instead of spreading the download across two functions
DiscoverDownloadStrategyAsync+StartDownloadsAsync, it now just uses oneStartDownloadsAsync. The reason i made this change was because before it was hard to trace the flow of the http concurrency when its split across two functions. This consolidates the logic into one function.Motivation and Context
#3806
Testing
Types of changes
Checklist
License