diff --git a/doc/download.xml b/doc/download.xml index def4273..cf8548c 100644 --- a/doc/download.xml +++ b/doc/download.xml @@ -68,6 +68,8 @@ The following components are supported. that is a local filename, and the function writes the downloaded contents to this file; the returned record does not have a result component in this case. +

+ If the download fails then this file is not left behind. verifyCert diff --git a/lib/download.gi b/lib/download.gi index 1e01b2e..e051763 100644 --- a/lib/download.gi +++ b/lib/download.gi @@ -125,10 +125,16 @@ Add( Download_Methods, rec( res:= ValueGlobal( "SingleHTTPRequest" )( domain, port, "GET", uri, rec( Host:= authority ), false, false ); fi; - if res.statuscode = 0 then - return rec( success:= false, - error:= res.status ); - elif res.statuscode >= 400 then + if res.statuscode = 0 or res.statuscode >= 400 then + # 'SingleHTTPRequest' has already written the response body, for example + # an error page, to the target file; remove it. + if IsBound( opt.target ) and IsString( opt.target ) and + IsExistingFile( opt.target ) then + RemoveFile( opt.target ); + fi; + if res.statuscode = 0 then + return rec( success:= false, error:= res.status ); + fi; return rec( success:= false, error:= Concatenation( "HTTP error code ", String( res.statuscode ) ) ); @@ -219,6 +225,11 @@ Add( Download_Methods, rec( code:= Process( DirectoryCurrent(), exec, InputTextNone(), outstream, args ); CloseStream( outstream ); if code <> 0 then + # curl may have created the target file; remove it, as wget's method does + if IsBound( opt.target ) and IsString( opt.target ) and + IsExistingFile( opt.target ) then + RemoveFile( opt.target ); + fi; return rec( success:= false, error:= Concatenation( "Process returned ", String( code ) ) ); elif not ( IsBound( opt.target ) and IsString( opt.target ) ) then @@ -270,6 +281,13 @@ InstallMethod( Download, if res.success = true then return res; fi; + # A failed method may have left a partial or bogus target file behind. + # Remove it here, so that the guarantee holds for every method, + # including ones added to 'Download_Methods' from outside. + if IsBound( opt.target ) and IsString( opt.target ) and + IsExistingFile( opt.target ) then + RemoveFile( opt.target ); + fi; Info( InfoUtils, 2, "Download method ", r.name, " failed with\n", "#I ", res.error ); Add( errors, Concatenation( r.name, ": ", res.error ) ); diff --git a/tst/download.tst b/tst/download.tst index e52cc6e..f94824e 100644 --- a/tst/download.tst +++ b/tst/download.tst @@ -139,6 +139,26 @@ gap> res1:= Download( url, rec( maxTime:= 5 ) );; gap> res1.success = true; true +## A failed download must not leave the target file behind, +## whichever method was used. +gap> file:= Filename( DirectoryTemporary(), "target" );; +gap> for r in meths do +> res1:= r.download( Concatenation( baseurl, "/missing" ), +> rec( target:= file ) ); +> if res1.success = true then +> Print( "unexpected success for ", r.name, "\n" ); +> elif IsExistingFile( file ) then +> Print( "leftover target file after ", r.name, "\n" ); +> RemoveFile( file ); +> fi; +> od; +gap> res1:= Download( Concatenation( baseurl, "/missing" ), +> rec( target:= file ) );; +gap> res1.success = false; +true +gap> IsExistingFile( file ); +false + ## test errors and redirects gap> res1:= Download( Concatenation( baseurl, "/missing" ) );; gap> res1.success = false;