Download: never leave the target file behind after a failure - #103
Open
fingolfin wants to merge 1 commit into
Open
Download: never leave the target file behind after a failure#103fingolfin wants to merge 1 commit into
fingolfin wants to merge 1 commit into
Conversation
The wget method removed a partial target file, the curl method did not, and the SingleHTTPRequest method wrote the response body -- an error page, say -- to the target and then reported failure. So whether the file existed after a failed download, and what was in it, depended on which method happened to be available. Fixed in curl and SingleHTTPRequest, and enforced in 'Download' as well so it holds for methods added to 'Download_Methods' from elsewhere. Documented. The new test drives each available method directly; going through 'Download' alone hides the bug whenever a well-behaved method runs last. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #103 +/- ##
==========================================
- Coverage 86.24% 86.23% -0.02%
==========================================
Files 32 32
Lines 1905 1918 +13
==========================================
+ Hits 1643 1654 +11
- Misses 262 264 +2
🚀 New features to boost your workflow:
|
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.
When
opt.targetis set and the download fails, the three methods disagree:via wgetremoves the partial target file;via curlleaves it;via SingleHTTPRequestwrites the response body -- a 404 error page, say --to the target and then returns
success := false.So whether the file exists after a failed download, and what is in it, depends
on which method happened to be available. A caller cannot write correct
cleanup code against that, and one that tests
IsExistingFile( target )todecide whether it has the data will accept an error page.
tst/download.tstalready recorded the problem, in a comment:This fixes
via curlandvia SingleHTTPRequestto matchvia wget, andadditionally removes the target in
Downloaditself after any method reportsfailure, so the guarantee also covers methods added to
Download_Methodsfromoutside the package. Documented under the
targetoption.On the test
The test drives each available method directly rather than going through
Download. That is deliberate: throughDownloadthe bug hides whenever awell-behaved method runs after a leaking one. On my machine wget runs after
SingleHTTPRequestand cleans up, so aDownload-level test passed evenwithout the fix. Driven directly, it reports
gap --packagedirs $PWD tst/testall.g: 0 failures in 22 files.One question
via wgetraisesError( "Download cannot remove unwanted file ", ... )ifits own
RemoveFilefails. I left that alone -- it is loud rather than leaky-- but it now makes wget the only method that can turn a failed download into
a break loop. Happy to make it consistent with the others if you prefer.
Found while writing https://github.com/gap-packages/ArtifactManager, which
deletes the target before every attempt to work around this.
CC @ThomasBreuer