Make downloading more robust #22

iainlane · 2016-09-20T13:37:36Z

Two things

We should catch more than just timeout errors
There's some thread unsafety when multiple threads try to download the same file - we need to synchronise this.

I'm not sure if the package index is actually accessed by multiple threads - is this bit needed?

Otherwise random things cause downloading to fail when it might work on a retry.

If multiple threads try to download the same file, then one of them will get an incomplete file since `downloadFile` will return early if the dest exists, which is does while the download is in progress.

ximion · 2016-09-20T13:43:37Z

The package-index is accessed from multiple threads, but usually when that happens it already downloaded all the stuff and has a cache of data. So it's not strictly needed.
Be aware that with more synchronized blocks, the performance will drop significantly, because this means that this part of the code will run exclusively, with no other tasks (not even trivial ones) being run in parallel.

ximion · 2016-09-20T13:47:08Z

source/backends/debian/debpkgindex.d

@@ -112,7 +114,9 @@ public:
                                            "Translation-%s.%s".format(lang, "%s"));

            try {
-                fname = downloadIfNecessary (rootDir, tmpDir, fullPath);
+                synchronized (this) {
+                    fname = downloadIfNecessary (rootDir, tmpDir, fullPath);


Can we make this to only synchronize if we actually download anything? Because otherwise this will slow down things even if the file is already downloaded and present on disk.

Meh, hardly; the code basically just returns the filename (after checking if it exists) if it's not remote or if it is downloaded already.

If you insist, then I have to look at the filename in debpkgindex.d which makes it more ugly (currently remote file handling is all taken care of in utility methods).

iainlane · 2016-09-20T14:03:51Z

On Tue, Sep 20, 2016 at 06:43:37AM -0700, Matthias Klumpp wrote:

The package-index is accessed from multiple threads, but usually when that happens it already downloaded all the stuff and has a cache of data. So it's not strictly needed.
Be aware that with more synchronized blocks, the performance will drop significantly, because this means that this part of the code will run exclusively, with no other tasks (not even trivial ones) being run in parallel.

(argh, you can't use markdown from email?!?!)

Only this synchronized block on this object will be locked out, which is exactly what is required. When downloading the file for a Package, you have to wait for it to be downloaded before you can do anything with it.

If it's so bad to have code which reduces to something like

synchronized (this) {
    if file.exists () {
        return file;
    } else { 
        return null;
    }
}

(I can't believe that the overhead is worth worrying about) in the already-downloaded case, then I could set debFname = path after downloadFile returns.

Iain Lane [ iain@orangesquash.org.uk ]
Debian Developer [ laney@debian.org ]
Ubuntu Developer [ laney@ubuntu.com ]

ximion · 2016-09-20T14:06:30Z

Jap, I forgot that synchronized (this) will only lock the object itself.
This shouldn't be too noticeable.
https://tour.dlang.org/tour/en/multithreading/synchronization-sharing

One thing still exists that I would like to know: When does it happen that the package is accessed twice so that the same file is downloaded twice in parallel? That in itself shouldn't happen...

iainlane · 2016-09-20T14:08:58Z

I think I only ever saw it for arch:all packages. Can it happen for those in particular?

ximion · 2016-09-20T14:55:42Z

Yes, this makes sense for arch:all, since those packages are in all architecture-specific files, and we process all of them in parallel - so there will be duplicates for arch:all. They get filtered out later, and we don't process them completely again, since we will detect that the metadata is the same.

But still, this makes me wonder if we should maybe filter the arch:all packages out and deal with them explicitly...
Might even be an optimization with quite some impact.

Iain Lane added 2 commits September 19, 2016 17:03

Retry on all curl errors, not just timeouts

2958838

Otherwise random things cause downloading to fail when it might work on a retry.

Synchronise downloading of files

469182f

If multiple threads try to download the same file, then one of them will get an incomplete file since `downloadFile` will return early if the dest exists, which is does while the download is in progress.

ximion reviewed Sep 20, 2016

View changes

ximion merged commit 8b9d17a into ximion:master Sep 20, 2016

iainlane deleted the more-robust-downloading branch September 20, 2016 16:11

antonio-rojas mentioned this pull request Jul 10, 2017

Segfault when generating Arch metadata #46

Closed

Make downloading more robust #22

Make downloading more robust #22

iainlane commented Sep 20, 2016

ximion commented Sep 20, 2016

ximion Sep 20, 2016

iainlane Sep 20, 2016 •

edited

iainlane commented Sep 20, 2016 •

edited

ximion commented Sep 20, 2016

iainlane commented Sep 20, 2016

ximion commented Sep 20, 2016 •

edited

Make downloading more robust #22

Make downloading more robust #22

Conversation

iainlane commented Sep 20, 2016

ximion commented Sep 20, 2016

ximion Sep 20, 2016

Choose a reason for hiding this comment

iainlane Sep 20, 2016 • edited

Choose a reason for hiding this comment

iainlane commented Sep 20, 2016 • edited

ximion commented Sep 20, 2016

iainlane commented Sep 20, 2016

ximion commented Sep 20, 2016 • edited

iainlane Sep 20, 2016 •

edited

iainlane commented Sep 20, 2016 •

edited

ximion commented Sep 20, 2016 •

edited