From 742f67eaede80d2f9b3631d8697ebd63b8f95427 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Thu, 7 Apr 2016 17:48:17 +0200 Subject: [PATCH] don't ask server if we have entire file in partial/ MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit We have this situation in cases were parts of the transaction are refused (e.g. in a hashsum mismatch) and rerun the update (e.g. in the hope that we get a mirror which is synced this time). Previously we would ask the server with an if-range and in the best case recieve a 416 in response (less featureful server might end up giving us the entire file again or we get the wrong file this time giving us a hashsum mismatch…), which is a waste of time if we know already by checking the hashsums that we got the complete and correct file. --- methods/server.cc | 78 +++++++++++++------ test/integration/framework | 16 +++- test/integration/test-apt-update-transactions | 3 +- test/integration/test-pdiff-usage | 15 +++- 4 files changed, 81 insertions(+), 31 deletions(-) diff --git a/methods/server.cc b/methods/server.cc index 322b8d94c..b323ef4f3 100644 --- a/methods/server.cc +++ b/methods/server.cc @@ -419,36 +419,66 @@ void ServerMethod::SigTerm(int) depth. */ bool ServerMethod::Fetch(FetchItem *) { - if (Server == 0) + if (Server == nullptr || QueueBack == nullptr) return true; - // Queue the requests - int Depth = -1; - for (FetchItem *I = Queue; I != 0 && Depth < (signed)PipelineDepth; - I = I->Next, Depth++) - { - if (Depth >= 0) - { - // If pipelining is disabled, we only queue 1 request - if (Server->Pipeline == false) - break; - // if we have no hashes, do at most one such request - // as we can't fixup pipeling misbehaviors otherwise - else if (I->ExpectedHashes.usable() == false) - break; - } - + // If pipelining is disabled, we only queue 1 request + auto const AllowedDepth = Server->Pipeline ? PipelineDepth : 0; + // how deep is our pipeline currently? + decltype(PipelineDepth) CurrentDepth = 0; + for (FetchItem const *I = Queue; I != QueueBack; I = I->Next) + ++CurrentDepth; + + do { // Make sure we stick with the same server - if (Server->Comp(I->Uri) == false) + if (Server->Comp(QueueBack->Uri) == false) + break; + + bool const UsableHashes = QueueBack->ExpectedHashes.usable(); + // if we have no hashes, do at most one such request + // as we can't fixup pipeling misbehaviors otherwise + if (CurrentDepth != 0 && UsableHashes == false) break; - if (QueueBack == I) + + if (UsableHashes && FileExists(QueueBack->DestFile)) { - QueueBack = I->Next; - SendReq(I); - continue; + FileFd partial(QueueBack->DestFile, FileFd::ReadOnly); + Hashes wehave(QueueBack->ExpectedHashes); + if (QueueBack->ExpectedHashes.FileSize() == partial.FileSize()) + { + if (wehave.AddFD(partial) && + wehave.GetHashStringList() == QueueBack->ExpectedHashes) + { + FetchResult Res; + Res.Filename = QueueBack->DestFile; + Res.ResumePoint = QueueBack->ExpectedHashes.FileSize(); + URIStart(Res); + // move item to the start of the queue as URIDone will + // always dequeued the first item in the queue + if (Queue != QueueBack) + { + FetchItem *Prev = Queue; + for (; Prev->Next != QueueBack; Prev = Prev->Next) + /* look for the previous queue item */; + Prev->Next = QueueBack->Next; + QueueBack->Next = Queue; + Queue = QueueBack; + QueueBack = Prev->Next; + } + Res.TakeHashes(wehave); + URIDone(Res); + continue; + } + else + RemoveFile("Fetch-Partial", QueueBack->DestFile); + } } - } - + auto const Tmp = QueueBack; + QueueBack = QueueBack->Next; + SendReq(Tmp); + ++CurrentDepth; + } while (CurrentDepth <= AllowedDepth && QueueBack != nullptr); + return true; } /*}}}*/ diff --git a/test/integration/framework b/test/integration/framework index 213169a98..2a78e6194 100644 --- a/test/integration/framework +++ b/test/integration/framework @@ -1505,6 +1505,14 @@ testmarkedmanual() { msggroup } +catfile() { + if [ "${1##*.}" = 'deb' ]; then + stat >&2 "$1" || true + file >&2 "$1" || true + else + cat >&2 "$1" || true + fi +} msgfailoutput() { msgreportheader 'msgfailoutput' local MSG="$1" @@ -1514,7 +1522,7 @@ msgfailoutput() { echo >&2 while [ -n "$2" ]; do shift; done echo "#### Complete file: $1 ####" - cat >&2 "$1" || true + catfile "$1" echo '#### grep output ####' elif [ "$1" = 'test' ]; then echo >&2 @@ -1529,7 +1537,7 @@ msgfailoutput() { ls >&2 "$2" || true elif test -e "$2"; then echo "#### Complete file: $2 ####" - cat >&2 "$2" || true + catfile "$2" fi fi } @@ -1543,12 +1551,12 @@ msgfailoutput() { echo >&2 while [ -n "$2" ]; do echo "#### Complete file: $2 ####" - cat >&2 "$2" || true + catfile "$2" shift done echo '#### cmp output ####' fi - cat >&2 "$OUTPUT" + catfile "$OUTPUT" msgfail "$MSG" } diff --git a/test/integration/test-apt-update-transactions b/test/integration/test-apt-update-transactions index 884838038..d8154b283 100755 --- a/test/integration/test-apt-update-transactions +++ b/test/integration/test-apt-update-transactions @@ -44,7 +44,8 @@ testrun() { signreleasefiles onehashbroken() { - testfailure aptget update + rm -rf rootdir/var/lib/apt/lists/partial + testfailure aptget update -o Debug::pkgAcquire::Worker=1 -o Debug::Acquire::http=1 # each file generates two messages with this string testequal '2' grep --count 'Hash Sum mismatch' rootdir/tmp/testfailure.output testfileequal "$1" "$(listcurrentlistsdirectory)" diff --git a/test/integration/test-pdiff-usage b/test/integration/test-pdiff-usage index f219b9193..9c7946083 100755 --- a/test/integration/test-pdiff-usage +++ b/test/integration/test-pdiff-usage @@ -30,7 +30,7 @@ echo 'hacked' > aptarchive/hacked-i386 compressfile aptarchive/hacked-i386 wasmergeused() { - testsuccess apt update "$@" -o Debug::pkgAcquire::Worker=1 -o Debug::Acquire::http=1 -o Debug::pkgAcquire=1 + testsuccess apt update "$@" msgtest 'No intermediate patch files' 'still exist' local EDS="$(find rootdir/var/lib/apt/lists -name '*.ed' -o -name '*.ed.*')" @@ -224,6 +224,14 @@ SHA256-Download: testsuccessequal "$(cat "${PKGFILE}") " aptcache show apt oldstuff + # we reuse the entire state of the previous test here + msgmsg "Testcase: good files from previous fails are picked up from partial: $*" + wasmergeused "$@" + testfailure grep '^GET /Packages.diff/Index HTTP/1.1' rootdir/tmp/testsuccess.output + testnopackage oldstuff + testsuccessequal "$(cat Packages-future) +" aptcache show apt newstuff futurestuff + # we reuse the archive state of the previous test here msgmsg "Testcase: downloading a patch fails, but successful fallback: $*" rm -rf rootdir/var/lib/apt/lists @@ -294,7 +302,7 @@ SHA256-Download: $(sha256sum "${PATCHFILE}.gz" | cut -d' ' -f 1) $(stat -c%s "${PATCHFILE}.gz")000 $(basename "${PATCHFILE}.gz")" > "$PATCHINDEX" generatereleasefiles '+1hour' signreleasefiles - testsuccess apt update -o Debug::pkgAcquire::Diffs=1 "$@" + testsuccess apt update "$@" cp -f rootdir/tmp/testsuccess.output rootdir/tmp/aptupdate.output testsuccess grep 'bytes (Limit is' rootdir/tmp/aptupdate.output testnopackage oldstuff @@ -303,6 +311,9 @@ SHA256-Download: } echo 'Debug::pkgAcquire::Diffs "true"; Debug::Acquire::Transaction "true"; +Debug::pkgAcquire::Worker "true"; +Debug::Acquire::http "true"; +Debug::pkgAcquire "true"; Debug::pkgAcquire::rred "true";' > rootdir/etc/apt/apt.conf.d/rreddebug.conf testcase() { -- 2.45.2