Optimize Part Stream for Multi part Download #4162

GarrettBeatty · 2025-11-24T19:34:11Z

Previously in #4130, we would buffer every part, including the first part. This was not optimal because the first part can technically be streamed directly to the user because it will always be in order. Similarly, there is another case where if a part finishes downloading and is the next expected part, we can stream that right to the user instead of buffering.

This change adds support so that in those cases the part is streamed directly to the user instead of buffering in memory.

Changes

Add support for direct streaming
Also fix potential resource leak in existing code. I discovered these issues while working on this PR and i figure they are small enough so i included them here

Motivation and Context

#3806

Testing

Unit Tests which validate no additional array pools are created for this scenario and the part is streamed to the user directly
Existing integration tests pass

Re-ran performance tests and got similar results

Total bytes per run: 5,368,709,120

Run:1 Secs:2.661879 Gb/s:16.135098
Run:2 Secs:1.455994 Gb/s:29.498529
Run:3 Secs:1.204284 Gb/s:35.664076
Run:4 Secs:1.119768 Gb/s:38.355867
Run:5 Secs:1.057126 Gb/s:40.628725
Run:6 Secs:1.055039 Gb/s:40.709082
Run:7 Secs:1.060168 Gb/s:40.512150
Run:8 Secs:1.053934 Gb/s:40.751759
Run:9 Secs:1.062503 Gb/s:40.423092
Run:10 Secs:1.094352 Gb/s:39.246664

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist

My code follows the code style of this project
My change requires a change to the documentation
I have updated the documentation accordingly
I have read the README document
I have added tests to cover my changes
All new and existing tests passed

License

I confirm that this pull request can be released under the Apache 2 license

sdk/src/Services/S3/Custom/Transfer/Internal/BufferedPartDataHandler.cs

Copilot

Pull request overview

This PR optimizes the S3 multipart download manager to reduce memory usage by streaming responses directly to the consumer when parts arrive in sequential order, while falling back to buffering for out-of-order parts. The optimization is transparent to callers and maintains backward compatibility.

Key Changes:

Introduced StreamingDataSource class to stream GetObjectResponse directly without buffering
Modified BufferedPartDataHandler to intelligently choose between streaming (in-order parts) and buffering (out-of-order parts)
Added IPartBufferManager.AddBufferAsync(IPartDataSource) overload to support both streaming and buffered data sources
Updated response disposal logic to handle ownership transfer for streaming vs immediate disposal for buffering

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
generator/.DevConfigs/9d07dc1e-d82d-4f94-8700-c7b57f872045.json	Dev config with patch version bump and changelog (contains spelling error)
sdk/src/Services/S3/Custom/Transfer/Internal/StreamingDataSource.cs	New class implementing IPartDataSource for direct streaming from GetObjectResponse without buffering
sdk/src/Services/S3/Custom/Transfer/Internal/BufferedPartDataHandler.cs	Enhanced to decide between streaming vs buffering based on part arrival order
sdk/src/Services/S3/Custom/Transfer/Internal/IPartBufferManager.cs	Added AddBufferAsync(IPartDataSource) method to support both streaming and buffered sources
sdk/src/Services/S3/Custom/Transfer/Internal/PartBufferManager.cs	Implemented new AddBufferAsync overload to handle IPartDataSource types
sdk/src/Services/S3/Custom/Transfer/Internal/MultipartDownloadManager.cs	Updated response disposal logic to account for ownership transfer in streaming path
sdk/test/Services/S3/UnitTests/Custom/StreamingDataSourceTests.cs	Comprehensive unit tests for new StreamingDataSource class (708 lines)
sdk/test/Services/S3/UnitTests/Custom/BufferedPartDataHandlerTests.cs	Updated tests to cover streaming vs buffering decision logic and mixed scenarios
sdk/test/Services/S3/UnitTests/Custom/PartBufferManagerTests.cs	Added integration tests for StreamingDataSource with PartBufferManager

sdk/src/Services/S3/Custom/Transfer/Internal/BufferedPartDataHandler.cs

generator/.DevConfigs/9d07dc1e-d82d-4f94-8700-c7b57f872045.json

sdk/src/Services/S3/Custom/Transfer/Internal/MultipartDownloadManager.cs

sdk/src/Services/S3/Custom/Transfer/Internal/BufferedPartDataHandler.cs

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

sdk/src/Services/S3/Custom/Transfer/Internal/BufferedPartDataHandler.cs

generator/.DevConfigs/9d07dc1e-d82d-4f94-8700-c7b57f872045.json

sdk/src/Services/S3/Custom/Transfer/Internal/BufferedPartDataHandler.cs

GarrettBeatty · 2025-11-24T21:02:14Z

sdk/src/Services/S3/Custom/Transfer/Internal/BufferedPartDataHandler.cs

+        /// </list>
+        /// <para><strong>Response Ownership:</strong></para>
+        /// <para>
+        /// This method takes ownership of the response and is responsible for disposing it in ALL cases,


the reason for this is because previously MultipartDownloadManager disposed of the response in the finally block. but with this change of adding StreamingDataSource, if we kept the dispose in the MultipartDownloadManager, the response would've been dispoed by the time StreamingDataSource has to read it back to the user. Therefore we need to have StreamingDataSource keep ownership of the response

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

sdk/src/Services/S3/Custom/Transfer/Internal/BufferedPartDataHandler.cs

GarrettBeatty · 2025-11-24T21:29:00Z

sdk/src/Services/S3/Custom/Transfer/Internal/MultipartDownloadManager.cs

                {
-                    discoveryResult.InitialResponse.WriteObjectProgressEvent -= wrappedCallback;
+                    // Always detach the event handler to prevent memory leak
+                    // This runs whether ProcessPartAsync succeeds or throws


this is not related to the change at all, but one potential issue i found anyway, so fixing it here

GarrettBeatty · 2025-11-24T21:34:35Z

sdk/src/Services/S3/Custom/Transfer/Internal/BufferedMultipartStream.cs

+                    // This is critical because GetObjectResponse holds unmanaged resources that
+                    // won't be cleaned up by GC - must be explicitly disposed to return HTTP
+                    // connection to the pool and close network streams
+                    _discoveryResult?.InitialResponse?.Dispose();


unrelated to the optimizing stream for multi part download, but i discovered this potential resource leak while working on this

GarrettBeatty · 2025-11-25T18:28:50Z

sdk/src/Services/S3/Custom/Transfer/Internal/BufferedPartDataHandler.cs

        {
-            Logger.DebugFormat("BufferedPartDataHandler: [Part {0}] Starting to buffer part from response stream - ContentLength={1}",
-                partNumber, response.ContentLength);
+            if (partNumber == _partBufferManager.NextExpectedPartNumber)


this "felt" kind of weird putting this logic here but i felt it was fine for now

philasmar · 2025-11-28T15:36:10Z

generator/.DevConfigs/9d07dc1e-d82d-4f94-8700-c7b57f872045.json

+      "serviceName": "S3",
+      "type": "patch",
+      "changeLogMessages": [
+        "Optimized multipart download manager to stream responses directly where applicable."


isn't this an optimization for a feature we haven't yet released? Maybe we don't need a changelog in this case?

yeah you are right i can remove this dev config

remove dev config

GarrettBeatty commented Nov 24, 2025

View reviewed changes

sdk/src/Services/S3/Custom/Transfer/Internal/BufferedPartDataHandler.cs Show resolved Hide resolved

GarrettBeatty force-pushed the gcbeatty/optimization branch from 4e0bd93 to 34dad4c Compare November 24, 2025 19:47

GarrettBeatty requested a review from Copilot November 24, 2025 19:48

Copilot started reviewing on behalf of GarrettBeatty November 24, 2025 19:50 View session

Copilot finished reviewing on behalf of GarrettBeatty November 24, 2025 19:53

Copilot AI reviewed Nov 24, 2025

View reviewed changes

GarrettBeatty force-pushed the gcbeatty/optimization branch 2 times, most recently from e75f10a to 6a42d97 Compare November 24, 2025 20:38

GarrettBeatty requested a review from Copilot November 24, 2025 20:40

GarrettBeatty changed the title ~~Optimize part streaming~~ Optimize Part Stream for Multi part Download Nov 24, 2025

Copilot started reviewing on behalf of GarrettBeatty November 24, 2025 20:42 View session

GarrettBeatty force-pushed the gcbeatty/optimization branch from 6a42d97 to 28dbce5 Compare November 24, 2025 20:44

Copilot finished reviewing on behalf of GarrettBeatty November 24, 2025 20:45

Copilot AI reviewed Nov 24, 2025

View reviewed changes

sdk/src/Services/S3/Custom/Transfer/Internal/BufferedPartDataHandler.cs Outdated Show resolved Hide resolved

generator/.DevConfigs/9d07dc1e-d82d-4f94-8700-c7b57f872045.json Outdated Show resolved Hide resolved

GarrettBeatty commented Nov 24, 2025

View reviewed changes

sdk/src/Services/S3/Custom/Transfer/Internal/BufferedPartDataHandler.cs Outdated Show resolved Hide resolved

GarrettBeatty force-pushed the gcbeatty/optimization branch from 28dbce5 to 6e40599 Compare November 24, 2025 20:58

GarrettBeatty requested a review from Copilot November 24, 2025 20:58

Copilot started reviewing on behalf of GarrettBeatty November 24, 2025 21:00 View session

GarrettBeatty commented Nov 24, 2025

View reviewed changes

Copilot finished reviewing on behalf of GarrettBeatty November 24, 2025 21:03

Copilot AI reviewed Nov 24, 2025

View reviewed changes

sdk/src/Services/S3/Custom/Transfer/Internal/BufferedPartDataHandler.cs Show resolved Hide resolved

sdk/src/Services/S3/Custom/Transfer/Internal/BufferedPartDataHandler.cs Show resolved Hide resolved

GarrettBeatty force-pushed the gcbeatty/optimization branch 2 times, most recently from 12b92d1 to e6e17e3 Compare November 24, 2025 21:24

GarrettBeatty commented Nov 24, 2025

View reviewed changes

GarrettBeatty force-pushed the gcbeatty/optimization branch from e6e17e3 to 432b08c Compare November 24, 2025 21:33

GarrettBeatty commented Nov 24, 2025

View reviewed changes

GarrettBeatty requested review from normj and philasmar November 24, 2025 21:34

GarrettBeatty marked this pull request as ready for review November 24, 2025 21:34

GarrettBeatty force-pushed the gcbeatty/directoryresponse branch from a522b17 to 135eacc Compare November 25, 2025 14:56

GarrettBeatty force-pushed the gcbeatty/optimization branch from 432b08c to d268c0f Compare November 25, 2025 14:57

GarrettBeatty commented Nov 25, 2025

View reviewed changes

philasmar reviewed Nov 28, 2025

View reviewed changes

GarrettBeatty requested a review from philasmar November 28, 2025 18:23

philasmar approved these changes Dec 1, 2025

View reviewed changes

normj approved these changes Dec 1, 2025

View reviewed changes

GarrettBeatty force-pushed the gcbeatty/directoryresponse branch from 135eacc to 105d5c6 Compare December 1, 2025 22:05

Base automatically changed from gcbeatty/directoryresponse to feature/transfermanager December 1, 2025 22:07

Optimize part streaming

6d72f9a

remove dev config

GarrettBeatty force-pushed the gcbeatty/optimization branch from 493f981 to 6d72f9a Compare December 1, 2025 22:33

GarrettBeatty merged commit dd150cb into feature/transfermanager Dec 1, 2025
1 check passed

GarrettBeatty deleted the gcbeatty/optimization branch December 1, 2025 22:33

Optimize Part Stream for Multi part Download #4162

Optimize Part Stream for Multi part Download #4162

Uh oh!

Conversation

GarrettBeatty commented Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Motivation and Context

Testing

Types of changes

Checklist

License

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

GarrettBeatty Nov 24, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

GarrettBeatty Nov 24, 2025

Choose a reason for hiding this comment

Uh oh!

GarrettBeatty Nov 24, 2025

Choose a reason for hiding this comment

Uh oh!

GarrettBeatty Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

philasmar Nov 28, 2025

Choose a reason for hiding this comment

Uh oh!

GarrettBeatty Nov 28, 2025

Choose a reason for hiding this comment

Uh oh!

GarrettBeatty Nov 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

GarrettBeatty commented Nov 24, 2025 •

edited

Loading