From b90b9b936d17d59050263467e8e4590bf56508f0 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Thu, 19 Nov 2020 14:03:00 -0400 Subject: [PATCH] don't rely on exception for http 416 Fix a bug that could make resuming a download from the web fail when the entire content of the file is actually already present locally. What a mess that Request can throw exceptions or not, depending on how it's configured. Makes it very hard if you need to handle some specific http status codes in a function like this! Implementing everything two ways did not seem appealing, if possible at all, so I decided to override the Request if it did come configured to throw exception on non-2xx http status. Other exceptions, like from http-client-restricted, or due to a redirect to a non-http url, still get thrown. This commit was sponsored by Luke Shumaker on Patreon. --- CHANGELOG | 3 ++ Utility/Url.hs | 43 +++++++++++-------- ..._58___Requested_Range_Not_Satisfiable.mdwn | 2 + ..._44bc0678a9978b62ab133d64d3614610._comment | 30 +++++++++++++ 4 files changed, 60 insertions(+), 18 deletions(-) create mode 100644 doc/bugs/download_failed__58___Requested_Range_Not_Satisfiable/comment_1_44bc0678a9978b62ab133d64d3614610._comment diff --git a/CHANGELOG b/CHANGELOG index fc0db5e3d0..e1521c31f1 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -15,6 +15,9 @@ git-annex (8.20201117) UNRELEASED; urgency=medium * Fix build on Windows. * Prevent windows assistant from trying (and failing) to upgrade itself, which has never been supported on windows. + * Fix a bug that could make resuming a download from the web fail + when the entire content of the file is actually already present + locally. -- Joey Hess Mon, 16 Nov 2020 09:38:32 -0400 diff --git a/Utility/Url.hs b/Utility/Url.hs index eacac2f122..7ef821ca05 100644 --- a/Utility/Url.hs +++ b/Utility/Url.hs @@ -452,7 +452,12 @@ download' nocurlerror meterupdate url file uo = {- Download a perhaps large file using conduit, with auto-resume - of incomplete downloads. - - - Does not catch exceptions. + - A Request can be configured to throw exceptions for non-2xx http + - status codes, or not. That configuration is overridden by this, + - and if it is unable to download, it throws an exception containing + - a user-visible explanation of the problem. (However, exceptions + - thrown for reasons other than http status codes will still be thrown + - as usual.) -} downloadConduit :: MeterUpdate -> Request -> FilePath -> UrlOptions -> IO () downloadConduit meterupdate req file uo = @@ -480,28 +485,29 @@ downloadConduit meterupdate req file uo = filter ((/= hAcceptEncoding) . fst) (requestHeaders req) , decompress = const False + -- Avoid throwing exceptions non-2xx http status codes, + -- since we rely on parsing the Response to handle + -- several such codes. + , checkResponse = \_ _ -> return () } -- Resume download from where a previous download was interrupted, -- when supported by the http server. The server may also opt to -- send the whole file rather than resuming. - resumedownload sz = catchJust - (matchStatusCodeHeadersException (alreadydownloaded sz)) - dl - (const noop) - where - dl = join $ runResourceT $ do - let req'' = req' { requestHeaders = resumeFromHeader sz : requestHeaders req } - liftIO $ debugM "url" (show req'') - resp <- http req'' (httpManager uo) - if responseStatus resp == partialContent206 + resumedownload sz = join $ runResourceT $ do + let req'' = req' { requestHeaders = resumeFromHeader sz : requestHeaders req } + liftIO $ debugM "url" (show req'') + resp <- http req'' (httpManager uo) + if responseStatus resp == partialContent206 + then do + store (toBytesProcessed sz) AppendMode resp + return (return ()) + else if responseStatus resp == ok200 then do - store (toBytesProcessed sz) AppendMode resp + store zeroBytesProcessed WriteMode resp return (return ()) - else if responseStatus resp == ok200 - then do - store zeroBytesProcessed WriteMode resp - return (return ()) + else if alreadydownloaded sz resp + then return (return ()) else do rf <- extractFromResourceT (respfailure resp) if responseStatus resp == unauthorized401 @@ -510,8 +516,9 @@ downloadConduit meterupdate req file uo = Just ba -> retryauthed ba else return $ giveup rf - alreadydownloaded sz s h = s == requestedRangeNotSatisfiable416 - && case lookup hContentRange h of + alreadydownloaded sz resp + | responseStatus resp /= requestedRangeNotSatisfiable416 = False + | otherwise = case lookup hContentRange (responseHeaders resp) of -- This could be improved by fixing -- https://github.com/aristidb/http-types/issues/87 Just crh -> crh == B8.fromString ("bytes */" ++ show sz) diff --git a/doc/bugs/download_failed__58___Requested_Range_Not_Satisfiable.mdwn b/doc/bugs/download_failed__58___Requested_Range_Not_Satisfiable.mdwn index 188691639f..6cb41f9439 100644 --- a/doc/bugs/download_failed__58___Requested_Range_Not_Satisfiable.mdwn +++ b/doc/bugs/download_failed__58___Requested_Range_Not_Satisfiable.mdwn @@ -41,3 +41,5 @@ Stock git-annex on Fedora 32: ### Have you had any luck using git-annex before? (Sometimes we get tired of reading bug reports all day and a lil' positive end note does wonders) Yes! git-annex has been working great for me so far, and is powering the bioinformatics chat podcast (https://bioinformatics.chat/). Thanks! + +> [[fixed|done]] --[[Joey]] diff --git a/doc/bugs/download_failed__58___Requested_Range_Not_Satisfiable/comment_1_44bc0678a9978b62ab133d64d3614610._comment b/doc/bugs/download_failed__58___Requested_Range_Not_Satisfiable/comment_1_44bc0678a9978b62ab133d64d3614610._comment new file mode 100644 index 0000000000..006eec9bdc --- /dev/null +++ b/doc/bugs/download_failed__58___Requested_Range_Not_Satisfiable/comment_1_44bc0678a9978b62ab133d64d3614610._comment @@ -0,0 +1,30 @@ +[[!comment format=mdwn + username="joey" + subject="""comment 1""" + date="2020-11-19T16:56:45Z" + content=""" +To reproduce this, interrupt git-annex after it downloads the whole file, +but before it moves it from the download location into the annex. (Or, +let it get the file, then move the object back to the temp object location.) + +This is a tricky case, because if the total file size is not +known when resuming the download, how can it detect if it's got it all +already? And git-annex does not always know the total file size, eg when +git-annex addurl --relaxed is used, and then git-annex get is later used +to download the content. + +What git-annex already tried to do to detect this is, +when it got a 416 it looks for a Content-Range header "bytes */$size" +where $size is the same as the size of the file on disk. + +That relied on the http library throwing an exception for the 416. +Thing is, http may or may not throw exceptions for non-2xx +responses, depending on the input Request. IMHO that is a very bad design, +it leads to this kind of bug, rather than making it evident with the data +types what is going on. + +Currently downloadConduit takes a Request, and assumes it throws exceptions +for 416, but not for 401. Both can't be right. + +Ok, fixed this mess.. +"""]] -- 2.30.2