Skip to content

Commit

Permalink
Merge #3907 Fix race condition for multiple very fast downloads
Browse files Browse the repository at this point in the history
  • Loading branch information
@HebaruSan
HebaruSan committed Sep 16, 2023
2 parents 5588442 + 625a557 commit c38c7a2
Show file tree
Hide file tree
Showing 4 changed files with 173 additions and 37 deletions.
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ All notable changes to this project will be documented in this file.
- [Multiple] Show recommendations of full changeset with opt-out (#3892 by: HebaruSan; reviewed: techman83)
- [Multiple] Dutch translation and icon duplication guardrails (#3897 by: HebaruSan; reviewed: techman83)
- [GUI] Shorten toolbar button labels (#3903 by: HebaruSan; reviewed: techman83)
- [Multiple] Refactor repository and available module handling (#3904 by: HebaruSan; reviewed: techman83)
- [Multiple] Refactor repository and available module handling (#3904, #3907 by: HebaruSan; reviewed: techman83)

### Bugfixes

Expand Down
2 changes: 1 addition & 1 deletion Core/Net/Net.cs
View file
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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));
}
}
}
}

0 comments on commit c38c7a2

Please sign in to comment.