From mboxrd@z Thu Jan  1 00:00:00 1970
From: Michael Tremer <michael.tremer@ipfire.org>
To: development@lists.ipfire.org
Subject:
 Re: [PATCH] Fix bug 11567 updxlrator: don't prematurely release lock, file
Date: Sat, 30 Dec 2017 14:33:01 +0000
Message-ID: <1514644381.3685.13.camel@ipfire.org>
In-Reply-To: <3407e12c-a7f6-6f39-c2b5-8ce33e325e8e@mail.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="===============7537533358774077715=="
List-Id: <development.lists.ipfire.org>

--===============7537533358774077715==
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit

Hello,

good patch and okay with me.

(Has the same line wrapping issue like the previous one)

Best,
-Michael

On Sat, 2017-12-30 at 10:44 +0300, Justin Luth wrote:
> With Microsoft's new style of downloading updates,
> where portions of a patch are requested multiple times per second,
> it has become extremely common for downloads to reach > 100%.
> Due to an early unlinking of the "lock" file, there is a big window of
> opportunity (between the unlink and wget actually saving some data)
> for multiple download/wget threads to start, adding to the same file.
> So not only is bandwidth wasted by duplicate downloads running
> simultaneously, but the resulting file is corrupt anyway.
> 
> The problem is noticed more often by low bandwidth users
> (who need the benefits of updxlrator the most)
> because then wget's latency is even longer, creating
> a very wide window of opportunity.
> 
> Ultimately, this needs something like "flock", where the
> file is set and tested in one operation. But for now,
> settle with the current test / create lock solution, and
> just stop unnecessarily releasing the lock.
> 
> Since the file already exists as a lock when wget starts,
> wget now must ALWAYS run with --continue, which
> works fine on a zero-sized file.
> 
> Signed-off-by: Justin Luth  <jluth(a)mail.com>
> ---
>   config/updxlrator/download | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/config/updxlrator/download b/config/updxlrator/download
> index dbc722c23..afa6e6cb9 100644
> --- a/config/updxlrator/download
> +++ b/config/updxlrator/download
> @@ -30,7 +30,6 @@ my $unique=0;
>   my $mirror=1;
> 
>   my %dlinfo=();
> -my $wgetContinueFlag="";
> 
>   my $vendorid  = $ARGV[0]; if (!defined($vendorid)  || $vendorid eq '') 
> { exit; }
>   my $sourceurl = $ARGV[1]; if (!defined($sourceurl) || $sourceurl eq 
> '') { exit; }
> @@ -57,16 +56,15 @@ if($restartdl == 0)
>       # this is a new download
>       exit if (-e "$repository/download/$vendorid/$updatefile");
> 
> -    # dotzball: Why is this necessary?
> +    # hinder multiple downloads from starting simultaneously. Create 
> empty "lock" file.
> +    # TODO: Another thread may sneak in between these two commands - so 
> not fool-proof, but good enough?
>       system("touch $repository/download/$vendorid/$updatefile");
> -    $wgetContinueFlag = "-nc";
> 
>   }
>   else
>   {
>       # this is a restart of a previous (unfinished) download
>       # -> continue download
> -    $wgetContinueFlag = "-c";
>       &writelog("Continue download: $updatefile");
>   }
> 
> @@ -133,7 +131,9 @@ unless($restartdl)
>   {
>       # this is a new download
>       # -> download from scratch
> -    unlink "$repository/download/$vendorid/$updatefile";
> +
> +    #already exited earlier if the file existed, and afterwards created 
> this empty "lock", so if not empty now, another thread is already 
> downloading it.
> +    exit if ( -s "$repository/download/$vendorid/$updatefile" );
>       unlink "$repository/download/$vendorid/$updatefile.info";
>   }
> 
> @@ -147,7 +147,7 @@ $dlinfo{'REMOTESIZE'} = $remote_size;
>   $dlinfo{'STATUS'} = "1";
>   &UPDXLT::writehash("$repository/download/$vendorid/$updatefile.info", 
> \%dlinfo);
> 
> -my $cmd = "$UPDXLT::wget $login $dlrate 
> --user-agent=\"$UPDXLT::useragent\" -q -P $repository/download/$vendorid 
> $wgetContinueFlag $sourceurl";
> +my $cmd = "$UPDXLT::wget $login $dlrate 
> --user-agent=\"$UPDXLT::useragent\" -q -P $repository/download/$vendorid 
> --continue $sourceurl";
> 
>   $_ = system("$cmd");
>   $ENV{'http_proxy'} = '';

--===============7537533358774077715==
Content-Type: application/pgp-signature
Content-Transfer-Encoding: base64
Content-Disposition: attachment; filename="signature.asc"
MIME-Version: 1.0

LS0tLS1CRUdJTiBQR1AgU0lHTkFUVVJFLS0tLS0KCmlRSXpCQUFCQ2dBZEZpRUU1L3JXNWwzR0dl
Mnlwa3R4Z0hudy8yK1FDUWNGQWxwSG81MEFDZ2tRZ0hudy8yK1EKQ1FlcjhoQUF0SXhDMllZdUtz
bkNQV0NRcDd0RUliejFtVUVyS1RZV2VZMXpvZUhORVJuWk4yZDJrRXJHQ095UwpCQjV1d3FxcW5G
dUFWelpDNkFUb3dBa09zTFdGc01PajJueXdSak94aHJuWVF4clByU0lXa0V3UFVpMWMvUThnCjVj
SFd6d1hQMUt1a29MNzh0RytuVjZrY1M5Y3pMZnlHdjNqUEtjb2JWM3FYd0QyN2pYZ2srTjErSmE2
a1dFcnQKTmlYYXBsb0JHR0hTU1pQZ0o5TDVLUmdsK244KzNnS3NpR2hieDhsMlZmSTRqck1lT21R
ZXRJS1NQVjlHSEs1Sgo0ZE5RYzNYN2pTOWxHa1U0Y0ZCUUxsZ3FLMjhGdytIVWtEZldTWWdhWXJi
ZzBqaTJDRUFOMUIzbHl4dnZhQm42CmZlTFp0SDhqcjk1NGVHYUF5WGlaem0xSllnUDRRdFVzeVhU
cTd5SU4yOFh5ekd2eEtqemN6VVQ3WnBkQ1R6cEIKVzlKaVhQWTRMZ0ZMTWFNMS94dGoybHVoN1BL
QjI2NklERERFQXdzaUoyYVExYnFLSTY4Y3FqRUE2Tm5kVUZyZQoyRkdialBNWXc2aXVubVJtVzUx
UGFaZGprMmNROVFBNUMzOVJleEQvV2ZXYnJyeWpBcUlwbkVJeHh5NVlJeTlCClBwbW9pYXpGSUkx
TDJDNVBHaXJZNFhJN0dKQ24yRmYvMXA3QW1VSk03b0QxVkZ2Q3RubHVSWGpYMTdJcjlYTUsKeWpT
MHVid2U2RCtuRUZrR2ZJazJkQWlveVRmZ2dMcHV1aGVYSWNKRHpSSURPdGNxRldEN2JoVC9xT3Zh
azJLcgpyMExvcGEwbzhiYVFiMGJvZTQ0WnNOV0YxY2pmdmlKM2k2bkNTVmJBYVZEY3oyT1o1NEE9
Cj1kZ3c0Ci0tLS0tRU5EIFBHUCBTSUdOQVRVUkUtLS0tLQo=

--===============7537533358774077715==--