Skip to content

Conversation

@GarrettBeatty
Copy link
Contributor

@GarrettBeatty GarrettBeatty commented Dec 2, 2025

Fixes a race condition where previously the following could happen

// Tasks created in order: 2, 3, 4, 5...
 for (int partNum = 2; partNum <= discoveryResult.TotalParts; partNum++)
    {
      var task = CreateDownloadTaskAsync(partNum, discoveryResult.ObjectSize, wrappedCallback, internalCts.Token);
      downloadTasks.Add(task);
    }

// Inside CreateDownloadTaskAsync:
private async Task CreateDownloadTaskAsync(...)
{
    // ← RACE CONDITION WINDOW IS HERE ←
    // Part 3 could reach this line before Part 2 due to thread scheduling
    await _dataHandler.WaitForCapacityAsync(cancellationToken);
    
}

This could result in a deadlock where for example part 99 gets capacity before part 2, and since for streams it has to read in sequential order it would be a deadlock.

How

  1. The fix is to acquire capacity before creating the task. this guarantees the requests are sent in sequential order.
  2. we also had to move the logic to the background task as well is because in the case of MaxInMemoryParts = 1

If we did not move it when we make this change

1. StartDownloadsAsync() begins
2. Process Part 1 synchronously → Takes the 1 available buffer slot
3. Create tasks for Parts 2, 3, 4, etc. immediately in main thread
4. Each task calls WaitForCapacityAsync() when it starts
5. 🚫 ALL parts 2+ are blocked waiting for the 1 buffer slot
6. 🚫 Part 1 can't be consumed because StartDownloadsAsync() hasn't returned yet
7. 🚫 Consumer can't start reading because method is still blocked
8. 💥 DEADLOCK: Part 1 holds buffer, Parts 2+ wait for buffer, Consumer waits for method return


new flow

1. StartDownloadsAsync() begins  
2. Process Part 1 synchronously → Takes the 1 available buffer slot
3. Create background Task.Run() and return IMMEDIATELY
4. ✅ Consumer can start reading Part 1 right away
5. Background task sequentially:
   a. WaitForCapacityAsync() for Part 2 (blocks until Part 1 consumed)
   b. Create task for Part 2 (now has buffer space)
   c. WaitForCapacityAsync() for Part 3 (blocks until Part 2 consumed)  
   d. Create task for Part 3, etc.

Motivation and Context

#3806

Testing

  1. Wrote new unit tests which cover that requests are fired in sequential order by checking capcity
  2. reran existing unit and integ tests

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

Copy link
Contributor

Copilot AI left a 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 race condition in multipart downloads where out-of-order task scheduling could cause parts to acquire buffer capacity non-sequentially, potentially leading to deadlocks when buffer space is limited. The fix ensures capacity is acquired in sequential order (Part 2, then Part 3, etc.) before creating download tasks, and moves this logic to a background task to prevent blocking the consumer when MaxInMemoryParts=1.

Key Changes:

  • Moved capacity acquisition before task creation to guarantee sequential ordering
  • Relocated the download task creation loop into a background task to prevent consumer deadlock
  • Added proper CancellationTokenSource disposal in both success and failure paths

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
MultipartDownloadManager.cs Refactored StartDownloadsAsync to acquire capacity sequentially before task creation within a background task, added proper resource cleanup
MultipartDownloadManagerTests.cs Added comprehensive test coverage for sequential capacity acquisition, race condition prevention, and resource management

@GarrettBeatty GarrettBeatty marked this pull request as ready for review December 2, 2025 20:33
@GarrettBeatty GarrettBeatty merged commit a65b3ec into feature/transfermanager Dec 3, 2025
1 check passed
@GarrettBeatty GarrettBeatty deleted the gcbeatty/raceconditionfix branch December 3, 2025 16:14
@GarrettBeatty GarrettBeatty mentioned this pull request Dec 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants