Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix race condition for multiple very fast downloads #3907

Merged
merged 1 commit into from Sep 16, 2023
Merged

Fix race condition for multiple very fast downloads #3907

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion Core/Net/Net.cs
View file
Open in desktop
Expand Up @@ -168,7 +168,7 @@ public static string DownloadWithProgress(Uri url, string filename = null, IUser
return targets.First().filename;
}

public static void DownloadWithProgress(ICollection<DownloadTarget> downloadTargets, IUser user = null)
public static void DownloadWithProgress(IList<DownloadTarget> downloadTargets, IUser user = null)
{
var downloader = new NetAsyncDownloader(user ?? new NullUser());
downloader.onOneCompleted += (url, filename, error, etag) =>
Expand Down
51 changes: 28 additions & 23 deletions Core/Net/NetAsyncDownloader.cs
View file
Open in desktop
Expand Up @@ -152,27 +152,30 @@ public NetAsyncDownloader(IUser user)
/// Start a new batch of downloads
/// </summary>
/// <param name="targets">The downloads to begin</param>
public void DownloadAndWait(ICollection<Net.DownloadTarget> targets)
public void DownloadAndWait(IList<Net.DownloadTarget> targets)
{
if (downloads.Count + queuedDownloads.Count > completed_downloads)
lock (dlMutex)
{
// Some downloads are still in progress, add to the current batch
foreach (Net.DownloadTarget target in targets)
if (downloads.Count + queuedDownloads.Count > completed_downloads)
{
DownloadModule(new NetAsyncDownloaderDownloadPart(target));
// Some downloads are still in progress, add to the current batch
foreach (Net.DownloadTarget target in targets)
{
DownloadModule(new NetAsyncDownloaderDownloadPart(target));
}
// Wait for completion along with original caller
// so we can handle completion tasks for the added mods
complete_or_canceled.WaitOne();
return;
}
// Wait for completion along with original caller
// so we can handle completion tasks for the added mods
complete_or_canceled.WaitOne();
return;
}

completed_downloads = 0;
// Make sure we are ready to start a fresh batch
complete_or_canceled.Reset();
completed_downloads = 0;
// Make sure we are ready to start a fresh batch
complete_or_canceled.Reset();

// Start the download!
Download(targets);
// Start the downloads!
Download(targets);
}

log.Debug("Waiting for downloads to finish...");
complete_or_canceled.WaitOne();
Expand Down Expand Up @@ -242,7 +245,8 @@ public void DownloadAndWait(ICollection<Net.DownloadTarget> targets)
}
// Otherwise just note the error and which download it came from,
// then throw them all at once later.
exceptions.Add(new KeyValuePair<int, Exception>(i, downloads[i].error));
exceptions.Add(new KeyValuePair<int, Exception>(
targets.IndexOf(downloads[i].target), downloads[i].error));
}
}
if (exceptions.Count > 0)
Expand Down Expand Up @@ -338,13 +342,14 @@ private void DownloadModule(NetAsyncDownloaderDownloadPart dl)
/// true to queue, false to start immediately
/// </returns>
private bool shouldQueue(Uri url)
// Ignore inactive downloads
=> downloads.Except(queuedDownloads)
.Any(dl => (!dl.CurrentUri.IsAbsoluteUri || dl.CurrentUri.Host == url.Host)
// Consider done if no bytes left
&& dl.bytesLeft > 0
// Consider done if already tried and failed
&& dl.error == null);
=> !url.IsFile
// Ignore inactive downloads
&& downloads.Except(queuedDownloads)
.Any(dl => (!dl.CurrentUri.IsAbsoluteUri || dl.CurrentUri.Host == url.Host)
// Consider done if no bytes left
&& dl.bytesLeft > 0
// Consider done if already tried and failed
&& dl.error == null);

private void triggerCompleted()
{
Expand Down
155 changes: 143 additions & 12 deletions Tests/Core/Net/NetAsyncDownloaderTests.cs
View file
Open in desktop
Expand Up @@ -28,16 +28,22 @@ public void DownloadAndWait_WithValidFileUrl_SetsTargetSize(string pathWithinTes
var target = new CKAN.Net.DownloadTarget(new List<Uri> { new Uri(fromPath) },
Path.GetTempFileName());
var targets = new CKAN.Net.DownloadTarget[] { target };
var realSize = new FileInfo(fromPath).Length;
var origSize = new FileInfo(fromPath).Length;

// Act
try
{
downloader.DownloadAndWait(targets);
var realSize = new FileInfo(target.filename).Length;

// Assert
Assert.IsTrue(File.Exists(target.filename));
Assert.AreEqual(realSize, target.size);
Assert.Multiple(() =>
{
Assert.IsTrue(File.Exists(target.filename));
Assert.AreEqual(origSize, realSize, "Size on disk should match original");
Assert.AreEqual(realSize, target.size, "Target size should match size on disk");
Assert.AreEqual(origSize, target.size, "Target size should match original");
});
}
finally
{
Expand All @@ -46,32 +52,48 @@ public void DownloadAndWait_WithValidFileUrl_SetsTargetSize(string pathWithinTes
}

[Test,
TestCase("DogeCoinFlag-1.01.zip",
TestCase("gh221.zip",
"ModuleManager-2.5.1.zip",
"ZipWithUnicodeChars.zip",
"DogeCoinPlugin.zip",
"DogeCoinFlag-1.01-corrupt.zip",
"CKAN-meta-testkan.zip",
"ZipWithBadChars.zip",
"DogeCoinFlag-1.01-no-dir-entries.zip",
"DogeTokenFlag-1.01.zip",
"DogeCoinFlag-1.01.zip",
"DogeCoinFlag-1.01-LZMA.zip",
"DogeCoinFlag-1.01-avc.zip",
"DogeCoinFlag-extra-files.zip",
"DogeCoinFlag-1.01-corrupt.zip"),
"DogeCoinFlag-extra-files.zip"),
]
public void DownloadAndWait_WithValidFileUrls_SetsTargetsSize(params string[] pathsWithinTestData)
{
// Arrange
var downloader = new NetAsyncDownloader(new NullUser());
var fromPaths = pathsWithinTestData.Select(p => TestData.DataDir(p)).ToArray();
var targets = fromPaths.Select(p => new CKAN.Net.DownloadTarget(new List<Uri> { new Uri(p) },
Path.GetTempFileName()))
Path.GetTempFileName()))
.ToArray();
var realSizes = fromPaths.Select(p => new FileInfo(p).Length).ToArray();
var origSizes = fromPaths.Select(p => new FileInfo(p).Length).ToArray();

// Act
try
{
downloader.DownloadAndWait(targets);
var realSizes = targets.Select(t => new FileInfo(t.filename).Length).ToArray();
var targetSizes = targets.Select(t => t.size).ToArray();

// Assert
foreach (var t in targets)
Assert.Multiple(() =>
{
Assert.IsTrue(File.Exists(t.filename));
}
CollectionAssert.AreEquivalent(realSizes, targets.Select(t => t.size).ToArray());
foreach (var t in targets)
{
Assert.IsTrue(File.Exists(t.filename));
}
CollectionAssert.AreEquivalent(origSizes, realSizes, "Sizes on disk should match originals");
CollectionAssert.AreEquivalent(realSizes, targetSizes, "Target sizes should match sizes on disk");
CollectionAssert.AreEquivalent(origSizes, targetSizes, "Target sizes should match originals");
});
}
finally
{
Expand All @@ -81,5 +103,114 @@ public void DownloadAndWait_WithValidFileUrls_SetsTargetsSize(params string[] pa
}
}
}

[Test,
// Only one bad URL
TestCase("DoesNotExist.zip"),
// First URL is bad
TestCase("DoesNotExist.zip",
"gh221.zip",
"ModuleManager-2.5.1.zip",
"ZipWithUnicodeChars.zip",
"DogeCoinPlugin.zip",
"DogeCoinFlag-1.01-corrupt.zip",
"CKAN-meta-testkan.zip",
"ZipWithBadChars.zip",
"DogeCoinFlag-1.01-no-dir-entries.zip",
"DogeTokenFlag-1.01.zip",
"DogeCoinFlag-1.01.zip",
"DogeCoinFlag-1.01-LZMA.zip",
"DogeCoinFlag-1.01-avc.zip",
"DogeCoinFlag-extra-files.zip"),
// Last URL is bad
TestCase("gh221.zip",
"ModuleManager-2.5.1.zip",
"ZipWithUnicodeChars.zip",
"DogeCoinPlugin.zip",
"DogeCoinFlag-1.01-corrupt.zip",
"CKAN-meta-testkan.zip",
"ZipWithBadChars.zip",
"DogeCoinFlag-1.01-no-dir-entries.zip",
"DoesNotExist.zip",
"DogeTokenFlag-1.01.zip",
"DogeCoinFlag-1.01.zip",
"DogeCoinFlag-1.01-LZMA.zip",
"DogeCoinFlag-1.01-avc.zip",
"DogeCoinFlag-extra-files.zip"),
// A URL in the middle is bad
TestCase("gh221.zip",
"ModuleManager-2.5.1.zip",
"ZipWithUnicodeChars.zip",
"DogeCoinPlugin.zip",
"DogeCoinFlag-1.01-corrupt.zip",
"CKAN-meta-testkan.zip",
"ZipWithBadChars.zip",
"DogeCoinFlag-1.01-no-dir-entries.zip",
"DogeTokenFlag-1.01.zip",
"DogeCoinFlag-1.01.zip",
"DogeCoinFlag-1.01-LZMA.zip",
"DogeCoinFlag-1.01-avc.zip",
"DogeCoinFlag-extra-files.zip",
"DoesNotExist.zip"),
// Every other URL is bad
TestCase("DoesNotExist.zip",
"gh221.zip",
"DoesNotExist.zip",
"ModuleManager-2.5.1.zip",
"DoesNotExist.zip",
"ZipWithUnicodeChars.zip",
"DoesNotExist.zip",
"DogeCoinPlugin.zip",
"DoesNotExist.zip",
"DogeCoinFlag-1.01-corrupt.zip",
"DoesNotExist.zip",
"CKAN-meta-testkan.zip",
"DoesNotExist.zip",
"ZipWithBadChars.zip",
"DoesNotExist.zip",
"DogeCoinFlag-1.01-no-dir-entries.zip",
"DoesNotExist.zip",
"DogeTokenFlag-1.01.zip",
"DoesNotExist.zip",
"DogeCoinFlag-1.01.zip",
"DoesNotExist.zip",
"DogeCoinFlag-1.01-LZMA.zip",
"DoesNotExist.zip",
"DogeCoinFlag-1.01-avc.zip",
"DoesNotExist.zip",
"DogeCoinFlag-extra-files.zip",
"DoesNotExist.zip"),
]
public void DownloadAndWait_WithSomeInvalidUrls_ThrowsDownloadErrorsKraken(
params string[] pathsWithinTestData)
{
// Arrange
var downloader = new NetAsyncDownloader(new NullUser());
var fromPaths = pathsWithinTestData.Select(p => Path.GetFullPath(TestData.DataDir(p))).ToArray();
var targets = fromPaths.Select(p => new CKAN.Net.DownloadTarget(new List<Uri> { new Uri(p) },
Path.GetTempFileName()))
.ToArray();
var badIndices = fromPaths.Select((p, i) => new Tuple<int, bool>(i, File.Exists(p)))
.Where(tuple => !tuple.Item2)
.Select(tuple => tuple.Item1)
.ToArray();
var validTargets = targets.Where((t, i) => !badIndices.Contains(i));

// Act / Assert
var exception = Assert.Throws<DownloadErrorsKraken>(() =>
{
downloader.DownloadAndWait(targets);
});
CollectionAssert.AreEquivalent(badIndices, exception.Exceptions.Select(kvp => kvp.Key).ToArray());
foreach (var kvp in exception.Exceptions)
{
var baseExc = kvp.Value.GetBaseException() as FileNotFoundException;
Assert.AreEqual(fromPaths[kvp.Key], baseExc.FileName);
}
foreach (var t in validTargets)
{
Assert.IsTrue(File.Exists(t.filename));
}
}
}
}