Browse Source

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).
tags/debian/1.6_alpha6
David Kalnischkies 4 years ago
parent
commit
dff555d40b
6 changed files with 113 additions and 54 deletions
  1. +26
    -40
      apt-pkg/acquire-item.cc
  2. +5
    -0
      apt-pkg/acquire-item.h
  3. +25
    -9
      apt-pkg/acquire-worker.cc
  4. +4
    -4
      test/integration/framework
  5. +37
    -0
      test/integration/test-bug-869859-retry-downloads
  6. +16
    -1
      test/interactive-helper/aptwebserver.cc

+ 26
- 40
apt-pkg/acquire-item.cc View File

@@ -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 /*{{{*/


+ 5
- 0
apt-pkg/acquire-item.h View File

@@ -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 */


+ 25
- 9
apt-pkg/acquire-worker.cc View File

@@ -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();



+ 4
- 4
test/integration/framework View File

@@ -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


+ 37
- 0
test/integration/test-bug-869859-retry-downloads View File

@@ -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

+ 16
- 1
test/interactive-helper/aptwebserver.cc View File

@@ -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;


Loading…
Cancel
Save