From patchwork Sat Dec 30 18:44:53 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Justin Luth X-Patchwork-Id: 1589 Return-Path: Received: from mail01.ipfire.org (unknown [172.28.1.200]) by web02.ipfire.org (Postfix) with ESMTP id 6DB4860D8E for ; Sat, 30 Dec 2017 08:45:14 +0100 (CET) Received: from mail01.ipfire.org (localhost [IPv6:::1]) by mail01.ipfire.org (Postfix) with ESMTP id D71BFBAA; Sat, 30 Dec 2017 08:45:13 +0100 (CET) Received: from mout.gmx.com (mout.gmx.com [74.208.4.200]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "mail.gmx.com", Issuer "thawte SSL CA - G2" (verified OK)) by mail01.ipfire.org (Postfix) with ESMTPS id 3A304B83 for ; Sat, 30 Dec 2017 08:45:09 +0100 (CET) Received: from [192.168.3.1] ([41.79.25.253]) by mail.gmx.com (mrgmxus002 [74.208.5.15]) with ESMTPSA (Nemesis) id 0LqzUT-1f874c3wyx-00eaTJ for ; Sat, 30 Dec 2017 08:44:58 +0100 To: development@lists.ipfire.org From: Justin Luth Subject: [PATCH] Fix bug 11567 updxlrator: don't prematurely release lock, file Message-ID: <3407e12c-a7f6-6f39-c2b5-8ce33e325e8e@mail.com> Date: Sat, 30 Dec 2017 10:44:53 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 Content-Language: en-US X-Provags-ID: V03:K0:bL9ToP9whqSNIoNi2YtZ7L+sOwWP5Mk1VPe6DLfObzvAycPAnuN BZ/tVSjel8AHikDa1/0VUh9wdN9V7AMD3Pzc7SWA+cRMYdl42Fqw3XwCi/fX2Uu2HO495FF sFB/KDV5JyxWvlWxz9hCSU+gEZKj4ZRAOePv12Bh+2USwSsvmj2gEgyhkgfsiaZiIlXx8yr OTuyDS70GvijaiFXyjTXg== X-UI-Out-Filterresults: notjunk:1; V01:K0:YmnrNBXLX9g=:xQYVC0cf9m8+3HTqML5SKH q/zD3Lt7pcAloYGhjM08w3w7Krsn2balHYKiyhMfA6ETXmNDnbdNLdLmQ/5VbnqacMjQ9rnhs eCpklLbANtEUw00wH1lxYIlz9VrFOkZs/gOHYPvDVcO7cOGTyLGueOwLZbVVhCjLQX50o9BbG /KDMhlajjkxhuXYNGuELYl6MIB4db3/++Xad8S5w3mfVteZySJQ9Luw6lKV5DwmJceq+Pnekw tlkn2Ml0Nhbl2yB1NCHmNGbz5Lfq1BPSBte6iNb25nM8Ls2Hb4FuNhzjv8xQxh5ZV8Ul5JosY ChPEzpmVwdV3xV4hrjdGvQA8CoELbtCMu76a2rGzcNo5YYgKnpGwHrQpXuvAut6OsEaoADJyS 4JG0LDMz4MVzwgfBqRbJU06Whhr2tgWel/Xa4RAjiFhD8+ZwlLt7uiPu6Bvhh3YtmvttXxeUL +T/Ykkl33kH44NCW65rm5ghLF8uBuiwmHcfYhyt2D+BjUS6uIhOLpacV8RIpxoAXYsoCNddnB 8zuPM6WcuWPkgo99B/UfajbNvJ2aaFfJ4Qe+C++FER/JKxc8qJ56dUmh0sZ/VdPF2aTwB04Wd KG4BVpphsiKxgzG/jMpS9OcTnVRRP4TPR9t/2j9xsIzz3+e1ha254zf8UYPDYA5P+9YC4JrJF ohDouGatfc6WjmO91RJmNvvUwfAxJngrer7nL8js+WItrCnmhZ0wMt6tuzPRs3vFxQYWg3Ex8 RIqmw9mRoWL4BLawhzlSD7G+kuSHxkPC6DxPa2/1htNxUQtGZlhArVQPqn3WP6pb+7pdGS517 pSYjeORx+ngM5bpGo48GplvhdSULGqnK6aACALedI+CPAvyHZc= X-BeenThere: development@lists.ipfire.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: IPFire development talk List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: development-bounces@lists.ipfire.org Sender: "Development" 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  ---  config/updxlrator/download | 12 ++++++------  1 file changed, 6 insertions(+), 6 deletions(-)  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'} = ''; 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; }