implement Acquire::Retries support for all items

Moving the Retry-implementation from individual items to the worker
implementation not only gives every file retry capability instead of
just a selected few but also avoids needing to implement it in each item
(incorrectly).

6 files changed, 113 insertions, 54 deletions

diff --git a/apt-pkg/acquire-item.cc b/apt-pkg/acquire-item.cc
index b3eb75d16..8c11337ac 100644
--- a/apt-pkg/acquire-item.cc
+++ b/apt-pkg/acquire-item.cc
@@ -684,6 +684,11 @@ class pkgAcquire::Item::Private
 {
 public:
    std::vector<std::string> PastRedirections;
+   unsigned int Retries;
+
+   Private() : Retries(_config->FindI("Acquire::Retries", 0))
+   {
+   }
 };
 APT_IGNORE_DEPRECATED_PUSH
 pkgAcquire::Item::Item(pkgAcquire * const owner) :
@@ -707,6 +712,11 @@ std::string pkgAcquire::Item::Custom600Headers() const			/*{{{*/
    return std::string();
 }
 									/*}}}*/
+unsigned int &pkgAcquire::Item::ModifyRetries() /*{{{*/
+{
+   return d->Retries;
+}
+									/*}}}*/
 std::string pkgAcquire::Item::ShortDesc() const				/*{{{*/
 {
    return DescURI();
@@ -778,6 +788,13 @@ void pkgAcquire::Item::Failed(string const &Message,pkgAcquire::MethodConfig con
       Dequeue();
    }
 
+   FailMessage(Message);
+
+   if (QueueCounter > 1)
+      Status = StatIdle;
+}
+void pkgAcquire::Item::FailMessage(string const &Message)
+{
    string const FailReason = LookupTag(Message, "FailReason");
    enum { MAXIMUM_SIZE_EXCEEDED, HASHSUM_MISMATCH, WEAK_HASHSUMS, REDIRECTION_LOOP, OTHER } failreason = OTHER;
    if ( FailReason == "MaximumSizeExceeded")
@@ -851,9 +868,6 @@ void pkgAcquire::Item::Failed(string const &Message,pkgAcquire::MethodConfig con
       ReportMirrorFailureToCentral(*this, FailReason, ErrorText);
    else
       ReportMirrorFailureToCentral(*this, ErrorText, ErrorText);
-
-   if (QueueCounter > 1)
-      Status = StatIdle;
 }
 									/*}}}*/
 // Acquire::Item::Start - Item has begun to download			/*{{{*/
@@ -899,7 +913,7 @@ void pkgAcquire::Item::Done(string const &/*Message*/, HashStringList const &Has
       }
    }
    Status = StatDone;
-   ErrorText = string();
+   ErrorText.clear();
    Owner->Dequeue(this);
 }
 									/*}}}*/
@@ -3217,8 +3231,6 @@ pkgAcqArchive::pkgAcqArchive(pkgAcquire * const Owner,pkgSourceList * const Sour
                StoreFilename(StoreFilename), Vf(Version.FileList()),
 	       Trusted(false)
 {
-   Retries = _config->FindI("Acquire::Retries",0);
-
    if (Version.Arch() == 0)
    {
       _error->Error(_("I wasn't able to locate a file for the %s package. "
@@ -3453,17 +3465,6 @@ void pkgAcqArchive::Failed(string const &Message,pkgAcquire::MethodConfig const
    Status = StatIdle;
    if (QueueNext() == false)
    {
-      // This is the retry counter
-      if (Retries != 0 &&
-	  Cnf->LocalOnly == false &&
-	  StringToBool(LookupTag(Message,"Transient-Failure"),false) == true)
-      {
-	 Retries--;
-	 Vf = Version.FileList();
-	 if (QueueNext() == true)
-	    return;
-      }
-
       StoreFilename = string();
       Status = StatError;
    }
@@ -3754,14 +3755,12 @@ pkgAcqChangelog::~pkgAcqChangelog()					/*{{{*/
 									/*}}}*/
 
 // AcqFile::pkgAcqFile - Constructor					/*{{{*/
-pkgAcqFile::pkgAcqFile(pkgAcquire * const Owner,string const &URI, HashStringList const &Hashes,
-		       unsigned long long const Size,string const &Dsc,string const &ShortDesc,
+APT_IGNORE_DEPRECATED_PUSH
+pkgAcqFile::pkgAcqFile(pkgAcquire *const Owner, string const &URI, HashStringList const &Hashes,
+		       unsigned long long const Size, string const &Dsc, string const &ShortDesc,
 		       const string &DestDir, const string &DestFilename,
-                       bool const IsIndexFile) :
-                       Item(Owner), d(NULL), IsIndexFile(IsIndexFile), ExpectedHashes(Hashes)
+		       bool const IsIndexFile) : Item(Owner), d(NULL), Retries(0), IsIndexFile(IsIndexFile), ExpectedHashes(Hashes)
 {
-   Retries = _config->FindI("Acquire::Retries",0);
-
    if(!DestFilename.empty())
       DestFile = DestFilename;
    else if(!DestDir.empty())
@@ -3791,6 +3790,7 @@ pkgAcqFile::pkgAcqFile(pkgAcquire * const Owner,string const &URI, HashStringLis
 
    QueueURI(Desc);
 }
+APT_IGNORE_DEPRECATED_POP
 									/*}}}*/
 // AcqFile::Done - Item downloaded OK					/*{{{*/
 void pkgAcqFile::Done(string const &Message,HashStringList const &CalcHashes,
@@ -3840,24 +3840,10 @@ void pkgAcqFile::Done(string const &Message,HashStringList const &CalcHashes,
    }
 }
 									/*}}}*/
-// AcqFile::Failed - Failure handler					/*{{{*/
-// ---------------------------------------------------------------------
-/* Here we try other sources */
-void pkgAcqFile::Failed(string const &Message, pkgAcquire::MethodConfig const * const Cnf)
+void pkgAcqFile::Failed(string const &Message, pkgAcquire::MethodConfig const *const Cnf) /*{{{*/
 {
-   Item::Failed(Message,Cnf);
-
-   // This is the retry counter
-   if (Retries != 0 &&
-       Cnf->LocalOnly == false &&
-       StringToBool(LookupTag(Message,"Transient-Failure"),false) == true)
-   {
-      --Retries;
-      QueueURI(Desc);
-      Status = StatIdle;
-      return;
-   }
-
+   // FIXME: Remove this pointless overload on next ABI break
+   Item::Failed(Message, Cnf);
 }
 									/*}}}*/
 string pkgAcqFile::Custom600Headers() const				/*{{{*/
diff --git a/apt-pkg/acquire-item.h b/apt-pkg/acquire-item.h
index a5c7d848a..7705f3ccb 100644
--- a/apt-pkg/acquire-item.h
+++ b/apt-pkg/acquire-item.h
@@ -174,6 +174,7 @@ class pkgAcquire::Item : public WeakPointable				/*{{{*/
     *  \sa pkgAcqMethod
     */
    virtual void Failed(std::string const &Message,pkgAcquire::MethodConfig const * const Cnf);
+   APT_HIDDEN void FailMessage(std::string const &Message);
 
    /** \brief Invoked by the acquire worker to check if the successfully
     * fetched object is also the objected we wanted to have.
@@ -238,6 +239,8 @@ class pkgAcquire::Item : public WeakPointable				/*{{{*/
     *  no trailing newline.
     */
    virtual std::string Custom600Headers() const;
+   // Retries should really be a member of the Item, but can't be for ABI reasons
+   APT_HIDDEN unsigned int &ModifyRetries();
 
    /** \brief A "descriptive" URI-like string.
     *
@@ -994,6 +997,7 @@ class pkgAcqArchive : public pkgAcquire::Item
     *
     *  Set from Acquire::Retries.
     */
+   APT_DEPRECATED_MSG("Unused member. See pkgAcqItem::Retries.")
    unsigned int Retries;
 
    /** \brief \b true if this version file is being downloaded from a
@@ -1171,6 +1175,7 @@ class pkgAcqFile : public pkgAcquire::Item
    /** \brief How many times to retry the download, set from
     *  Acquire::Retries.
     */
+   APT_DEPRECATED_MSG("Unused member. See pkgAcqItem::Retries.")
    unsigned int Retries;
 
    /** \brief Should this file be considered a index file */
diff --git a/apt-pkg/acquire-worker.cc b/apt-pkg/acquire-worker.cc
index 39158aed7..a763d9242 100644
--- a/apt-pkg/acquire-worker.cc
+++ b/apt-pkg/acquire-worker.cc
@@ -506,6 +506,9 @@ bool pkgAcquire::Worker::RunMessages()
 	    Itm = nullptr;
 
 	    bool errTransient = false, errAuthErr = false;
+	    if (StringToBool(LookupTag(Message, "Transient-Failure"), false) == true)
+	       errTransient = true;
+	    else
 	    {
 	       std::string const failReason = LookupTag(Message, "FailReason");
 	       {
@@ -522,15 +525,28 @@ bool pkgAcquire::Worker::RunMessages()
 
 	    for (auto const Owner: ItmOwners)
 	    {
-	       if (errAuthErr && Owner->GetExpectedHashes().empty() == false)
-		  Owner->Status = pkgAcquire::Item::StatAuthError;
-	       else if (errTransient)
-		  Owner->Status = pkgAcquire::Item::StatTransientNetworkError;
-	       auto SavedDesc = Owner->GetItemDesc();
-	       if (isDoomedItem(Owner) == false)
-		  Owner->Failed(Message,Config);
-	       if (Log != nullptr)
-		  Log->Fail(SavedDesc);
+	       if (errTransient == true && Config->LocalOnly == false && Owner->ModifyRetries() != 0)
+	       {
+		  --Owner->ModifyRetries();
+		  Owner->FailMessage(Message);
+		  auto SavedDesc = Owner->GetItemDesc();
+		  if (Log != nullptr)
+		     Log->Fail(SavedDesc);
+		  if (isDoomedItem(Owner) == false)
+		     OwnerQ->Owner->Enqueue(SavedDesc);
+	       }
+	       else
+	       {
+		  if (errAuthErr && Owner->GetExpectedHashes().empty() == false)
+		     Owner->Status = pkgAcquire::Item::StatAuthError;
+		  else if (errTransient)
+		     Owner->Status = pkgAcquire::Item::StatTransientNetworkError;
+		  auto SavedDesc = Owner->GetItemDesc();
+		  if (isDoomedItem(Owner) == false)
+		     Owner->Failed(Message, Config);
+		  if (Log != nullptr)
+		     Log->Fail(SavedDesc);
+	       }
 	    }
 	    ItemDone();
 
diff --git a/test/integration/framework b/test/integration/framework
index bf6a8155e..bff6c0e65 100644
--- a/test/integration/framework
+++ b/test/integration/framework
@@ -1273,8 +1273,8 @@ webserverconfig() {
 		NOCHECK=true
 		shift
 	fi
-	local DOWNLOG='rootdir/tmp/download-testfile.log'
-	local STATUS='downloaded/webserverconfig.status'
+	local DOWNLOG="${TMPWORKINGDIRECTORY}/rootdir/tmp/download-testfile.log"
+	local STATUS="${TMPWORKINGDIRECTORY}/downloaded/webserverconfig.status"
 	rm -f "$STATUS" "$DOWNLOG"
 	# very very basic URI encoding
 	local URI
@@ -1937,8 +1937,8 @@ testaccessrights() {
 
 testwebserverlaststatuscode() {
 	msggroup 'testwebserverlaststatuscode'
-	local DOWNLOG='rootdir/tmp/webserverstatus-testfile.log'
-	local STATUS='downloaded/webserverstatus-statusfile.log'
+	local DOWNLOG="${TMPWORKINGDIRECTORY}/rootdir/tmp/webserverstatus-testfile.log"
+	local STATUS="${TMPWORKINGDIRECTORY}/downloaded/webserverstatus-statusfile.log"
 	rm -f "$DOWNLOG" "$STATUS"
 	msgtest 'Test last status code from the webserver was' "$1"
 	if downloadfile "http://localhost:${APTHTTPPORT}/_config/find/aptwebserver::last-status-code" "$STATUS" > "$DOWNLOG" && [ "$(cat "$STATUS")" = "$1" ]; then
diff --git a/test/integration/test-bug-869859-retry-downloads b/test/integration/test-bug-869859-retry-downloads
new file mode 100755
index 000000000..a62429a53
--- /dev/null
+++ b/test/integration/test-bug-869859-retry-downloads
@@ -0,0 +1,37 @@
+#!/bin/sh
+set -e
+
+TESTDIR="$(readlink -f "$(dirname "$0")")"
+. "$TESTDIR/framework"
+
+setupenvironment
+configarchitecture 'amd64'
+
+buildsimplenativepackage 'testpkg' 'all' '1' 'stable'
+
+setupaptarchive --no-update
+changetowebserver
+testsuccess apt update
+
+cd downloaded
+testsuccess apt download testpkg
+testsuccess test -f testpkg_1_all.deb
+rm -f testpkg_1_all.deb
+
+msgmsg 'Fail after too many retries'
+webserverconfig 'aptwebserver::failrequest' '429'
+webserverconfig 'aptwebserver::failrequest::pool/testpkg_1_all.deb' '99'
+testfailure apt download testpkg -o acquire::retries=3
+testfailure test -f testpkg_1_all.deb
+
+msgmsg 'Success in the third try'
+webserverconfig 'aptwebserver::failrequest::pool/testpkg_1_all.deb' '2'
+testsuccess apt download testpkg -o acquire::retries=3
+testsuccess test -f testpkg_1_all.deb
+rm -f testpkg_1_all.deb
+
+msgmsg 'Do not try everything again, hard failures keep hard failures'
+webserverconfig 'aptwebserver::failrequest' '404'
+webserverconfig 'aptwebserver::failrequest::pool/testpkg_1_all.deb' '2'
+testfailure apt download testpkg -o acquire::retries=3
+testfailure test -f testpkg_1_all.deb
diff --git a/test/interactive-helper/aptwebserver.cc b/test/interactive-helper/aptwebserver.cc
index 11f9b4b4f..4bc344178 100644
--- a/test/interactive-helper/aptwebserver.cc
+++ b/test/interactive-helper/aptwebserver.cc
@@ -82,7 +82,10 @@ static std::string httpcodeToStr(int const httpcode)			/*{{{*/
       case 504: return _config->Find("aptwebserver::httpcode::504", "504 Gateway Time-out");
       case 505: return _config->Find("aptwebserver::httpcode::505", "505 HTTP Version not supported");
    }
-   return "";
+   std::string codeconf, code;
+   strprintf(codeconf, "aptwebserver::httpcode::%i", httpcode);
+   strprintf(code, "%i Unknown HTTP code", httpcode);
+   return _config->Find(codeconf, code);
 }
 									/*}}}*/
 static bool chunkedTransferEncoding(std::list<std::string> const &headers) {
@@ -696,6 +699,18 @@ static void * handleClient(int const client, size_t const id)		/*{{{*/
 	    }
 	 }
 
+	 // automatic retry can be tested with this
+	 {
+	    int failrequests = _config->FindI("aptwebserver::failrequest::" + filename, 0);
+	    if (failrequests != 0)
+	    {
+	       --failrequests;
+	       _config->Set(("aptwebserver::failrequest::" + filename).c_str(), failrequests);
+	       sendError(log, client, _config->FindI("aptwebserver::failrequest", 400), *m, sendContent, "Server is configured to fail this file.", headers);
+	       continue;
+	    }
+	 }
+
 	 // deal with the request
 	 unsigned int const httpsport = _config->FindI("aptwebserver::port::https", 4433);
 	 std::string hosthttpsport;