-
Notifications
You must be signed in to change notification settings - Fork 5
fix(csharp): implement FIFO memory acquisition to prevent starvation in CloudFetch #56
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
Merged
+147
−3
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
…acquisition This PR fixes a memory deadlock issue in CloudFetch by ensuring FIFO (First-In-First-Out) ordering of memory acquisition. ## Problem Previously, memory was acquired inside DownloadFileAsync() after the download task was spawned. This caused downloads to compete for memory in an unpredictable order, potentially leading to deadlocks when memory is limited. ## Solution 1. Move memory acquisition to before spawning the download task - Memory is now acquired in the sequential main loop (line 283-285) - This ensures FIFO ordering based on when files arrive in the download queue - Removed memory acquisition from inside DownloadFileAsync() (previously line 395) 2. Increase default memory buffer from 100MB to 200MB - Reduces likelihood of memory contention - Better accommodates parallel downloads 3. Add comprehensive FIFO test - Verifies memory acquisition order - Verifies download start order (proves file 3 doesn't start before file 2) - Verifies resultQueue ordering ## Test Results - New FIFO test passes successfully - Validates proper ordering at three verification points 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Resolved conflict in CloudFetchDownloadManager.cs by accepting main branch refactoring that removed protocol-specific fields.
…ix-cloudfetch-memory-fifo
…Result interface Updated mock setup to use FileUrl, ByteCount, and ExpirationTime properties instead of the removed Link property after interface refactoring.
📊 CloudFetch Benchmark Results.NET 8.0
.NET Framework 4.7.2
🟢 Improvement | 🔴 Regression | ⚪ No change | Baseline from latest main branch run |
jadewang-db
approved these changes
Dec 8, 2025
📊 CloudFetch Benchmark Results.NET 8.0
.NET Framework 4.7.2
🟢 Improvement | 🔴 Regression | ⚪ No change | Baseline from latest main branch run |
📊 CloudFetch Benchmark Results.NET 8.0
.NET Framework 4.7.2
🟢 Improvement | 🔴 Regression | ⚪ No change | Baseline from latest main branch run |
📊 CloudFetch Benchmark Results.NET 8.0
.NET Framework 4.7.2
🟢 Improvement | 🔴 Regression | ⚪ No change | Baseline from latest main branch run |
…ix-cloudfetch-memory-fifo
msrathore-db
approved these changes
Dec 10, 2025
📊 CloudFetch Benchmark Results.NET 8.0
.NET Framework 4.7.2
🟢 Improvement | 🔴 Regression | ⚪ No change | Baseline from latest main branch run |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This is cherry pick of this PR: apache/arrow-adbc#3756
Fixes memory starvation issue in CloudFetch download pipeline where newer downloads could win memory allocation over older waiting downloads, causing indefinite delays.
Problem
The CloudFetch downloader was acquiring memory inside parallel download tasks with a polling-based wait (10ms intervals). This non-FIFO behavior caused memory starvation:
Example scenario with 200MB memory, 50MB files, 3 parallel downloads:
Solution
Move memory acquisition from inside parallel download tasks to the main sequential loop. This ensures FIFO ordering:
Additionally, increased default memory buffer from 100MB to 200MB to allow more parallel downloads without hitting memory limits.
Changes
AcquireMemoryAsyncfrom insideDownloadFileAsyncto main loop (line 278-280)DefaultMemoryBufferSizeMBfrom 100 to 200Testing
TODO
The memory buffer design have a flaw that it only captures the compressed size, but does not cover decompressed size. There are a couple of options we can take but I wanna land this first since this one actually can get blocked for customer.