Refactor HTTP download logic #13383
Open
Refactor HTTP download logic #13383
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
BatchDownloader is a wrapper class over Downloader that is effectively a glorified for loop. It doesn't need to exist.
The Downloader code is hard to follow as it passes a bunch of variables across a variety of functions and methods. To deal with this mess, encapsulate the state needed to download a single link into a separate _FileDownload class and use that instead.
The value used for conditional range requests should be updated when the download is restarted (not *resumed*). The unit tests testing conditional range requests were broken, using code 206 for the non-matching responses. This violates the HTTP spec and fools the resumption logic into thinking the conditional range request was fulfilled (when it wasn't!). The test cases were fixed to use code 200 as expected and extended to ensure the content key is updated across a chain of non-matching repsonses (which is borderline impossible, but hey it did catch my eye so why not fix it).
ichard26
added
type: refactor
uranusjr
approved these changes
May 19, 2025
|
@gmargaritis if you have time, I'd appreciate a look-over for this PR. |
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.
I was never really happy with how the download logic turned out after PR #12991. This is my second attempt at refactoring the downloading logic to be easier to read/follow. The main change is refactoring the
Downloaderclass to use a separate dataclass to store state for individual downloads. This greatly reduces the number of names being passed around in the download logic.It's easiest this to review by reviewing commit-by-commit.