fix: run blocking requests download in thread pool to unfreeze parallel downloads#982
Open
assafw wants to merge 1 commit into
Open
fix: run blocking requests download in thread pool to unfreeze parallel downloads#982assafw wants to merge 1 commit into
assafw wants to merge 1 commit into
Conversation
…el downloads fast_async_download was using the synchronous requests library directly on the event loop, blocking all concurrent downloads whenever a connection was active. Offload to run_in_executor so the event loop stays free, and dispatch progress callbacks via call_soon_threadsafe to keep rich's Live display thread-safe. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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
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.
Problem
fast_async_downloadwas using the synchronousrequestslibrary directly on the asyncio event loop. This caused all active concurrent downloads to freeze whenever one download was establishing a connection or reading data — becauserequests.get()is fully blocking and owns the thread until it completes.The
# noqa: ASYNC100/ASYNC101suppressions in the original code indicate this was a known lint warning that was silenced rather than addressed.The
await asyncio.sleep(0)yielded between ~1MB chunks, but the event loop was still blocked for the entire duration of each blocking read syscall between those yields.Root Cause
Fix
Split into a plain blocking function (
_do_download) and offload it to the default thread pool viarun_in_executor. This keeps the event loop free so all concurrent downloads proceed without blocking each other.The large 131KB chunk size from the original implementation is preserved — this was a deliberate fix for a separate issue where
aiohttp+aiofilesyielded to the event loop every 1KB, making downloads CPU-bound and capping total throughput at ~10MB/s.Progress callbacks are dispatched back to the event loop via
call_soon_threadsafeto keep rich'sLivedisplay thread-safe (it must only be updated from the main thread).What's preserved
requests+ large chunks for throughput (not reverted to aiohttp)max_connections/concurrencyconfig)requests_per_minuteconfig) — applied at the API layer before download starts