[pacman-dev] [PATCH] download: major refactor to address lingering issues

[Dan McGee]

On Sun, Nov 15, 2009 at 7:23 AM, Xavier <shiningxc at gmail.com> wrote:
> On Thu, Nov 12, 2009 at 9:09 AM, Dan McGee <dan at archlinux.org> wrote:
>> Sorry for this being such a huge patch, but I believe it is necessary for
>> quite a few reasons which I will attempt to explain herein. I've been
>> mulling this over for a while, but wasn't super happy with making the
>> download interface more complex. Instead, if we carefully order things in
>> the internal download code, we can actually make the interface simpler.
>>
>> 1. FS#15657 - This involves `name.db.tar.gz.part` files being left around the
>> filesystem, and then causing all sorts of issues when someone attempts to
>> rerun the operation they canceled. We need to ensure that if we resume a
>> download, we are resuming it on exactly the same file; if we cannot be
>> almost postive of that then we need to start over.
>>
>
> That's quite different to the simple solution we had before : simply
> disable resuming for repo db, which are quite small anyway :)

Yes. However, the only way to do this was to add even more flags to
the download (and thus callback). I had a "partial_ok" flag going for
a while in one iteration of the patch, but dumped it when I realized
the issue could be solved by just checking headers correctly.

>> 2. http://www.mail-archive.com/pacman-dev@archlinux.org/msg03536.html - Here
>> we have a lighttpd bug to ruin the day. If we send both a Range: header and
>> If-Modified-Since: header across the wire in a GET request, lighttpd doesn't
>> do what we want in several cases. If the file hadn't been modified, it
>> returns a '304 Not Modified' instead of a '206 Partial Content'. We need to
>> do a stat (e.g. HEAD in HTTP terms) operation here, and the proceed
>> accordingly based off the values we get back from it.
>>
>
> These two issues look completely different.
> The link you gave was about If-Modified-Since not working correctly,
> and lighttpd not returning '304 Not Modified' because of some
> time/timezone issues.
> Besides, this should have been fixed in 1.4.24 which is in arch now.
> I guess you should report your issue to lighttpd :)

Yeah. the original message was the same problem actually. The poster
of that message didn't diagnose it correctly, and it isn't the linked
issue to lighttpd. However, the lighty issue should be solved in their
Duke Nukem Forever 1.5 release.

> >From libfetch point of view, doing a Stat is fine.
> What seemed to cause issue for some users is doing a Get right away,
> and cancel when mtimes matched.
> See 6f97842ab22eb50fdc689e8aa2e95688d015fa74

Yep; this should not negate the changes of that fix.

>> 3. The mtime stuff was rather ugly, and relied on the called function to
>> write back to a passed in reference, which isn't the greatest. Instead, use
>> the power of the filesystem to contain this info. Every file downloaded
>> internally is now carefully timestamped with the remote file time. This
>> should allow the resume logic to work. In order to guarantee this, we need
>> to implement a signal handler that catches interrupts, notifies the running
>> code, and causes it to set the mtimes on the file. It then rethrows the
>> signal so the pacman signal handler (or any frontend) works as expected.
>>
>> 4. We did a lot of funky stuff in trying to track the DB last modified time.
>> It is a lot easier to just keep the downloaded DB file around and track the
>> time on that rather than in a funky dot file. It also kills a lot of code.
>>
>
> I always find signal handling quite tricky, but yeah, the idea of
> using the file itself to track mtime rather than a dot file sounds
> good.
> Also this indeed makes resuming much safer.
>
> I was just thinking about backward compatibility (old downloaded file
> and new download code), but I think it is fine :
> - for already downloaded packages , the download code is not called
> - for partially downloaded packages, they will be discarded and the
> download will start over. But if you still have partially downloaded
> packages when upgrading pacman, they are probably old and outdated.

Yep, and even locally things seemed to work out OK. Not a show-stopper
by any means.

>> 5. For GPG verification of the databases down the road, we are going to need
>> the DB file around for at least a short bit of time anyway, so this gets us
>> closer to that.
>>
>
> Well, I do not think this would have been a problem. We probably want
> to do gpg verification right after downloading, and before extracting
> (and then removing) the file, don't we ?

But why not allow it to happen any time? Why is the removal of the
file tied in with the download/extraction loop? It is just a big mess
right now IMO.

> Anyway, I did not have big issues with the old code, but I do not have
> any with the new one either. Code review and testing did not reveal
> any issues.
> And the new one does have several advantages so it's all good.

Thanks for the review, appreciate it.