From 28866439aebd3ae3a65360b7c82064fdef9b3635 Mon Sep 17 00:00:00 2001 From: Paul Hebble Date: Fri, 15 Sep 2023 13:49:23 -0500 Subject: [PATCH] Fix race condition for multiple very fast downloads --- Core/Net/Net.cs | 2 +- Core/Net/NetAsyncDownloader.cs | 51 ++++---- Tests/Core/Net/NetAsyncDownloaderTests.cs | 148 ++++++++++++++++++++-- 3 files changed, 166 insertions(+), 35 deletions(-) diff --git a/Core/Net/Net.cs b/Core/Net/Net.cs index d12edf7cb6..04894d15ba 100644 --- a/Core/Net/Net.cs +++ b/Core/Net/Net.cs @@ -168,7 +168,7 @@ public static string DownloadWithProgress(Uri url, string filename = null, IUser return targets.First().filename; } - public static void DownloadWithProgress(ICollection downloadTargets, IUser user = null) + public static void DownloadWithProgress(IList downloadTargets, IUser user = null) { var downloader = new NetAsyncDownloader(user ?? new NullUser()); downloader.onOneCompleted += (url, filename, error, etag) => diff --git a/Core/Net/NetAsyncDownloader.cs b/Core/Net/NetAsyncDownloader.cs index f44ac5c055..c32f77271f 100644 --- a/Core/Net/NetAsyncDownloader.cs +++ b/Core/Net/NetAsyncDownloader.cs @@ -152,27 +152,30 @@ public NetAsyncDownloader(IUser user) /// Start a new batch of downloads /// /// The downloads to begin - public void DownloadAndWait(ICollection targets) + public void DownloadAndWait(IList 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(); @@ -242,7 +245,8 @@ public void DownloadAndWait(ICollection targets) } // Otherwise just note the error and which download it came from, // then throw them all at once later. - exceptions.Add(new KeyValuePair(i, downloads[i].error)); + exceptions.Add(new KeyValuePair( + targets.IndexOf(downloads[i].target), downloads[i].error)); } } if (exceptions.Count > 0) @@ -338,13 +342,14 @@ private void DownloadModule(NetAsyncDownloaderDownloadPart dl) /// true to queue, false to start immediately /// 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() { diff --git a/Tests/Core/Net/NetAsyncDownloaderTests.cs b/Tests/Core/Net/NetAsyncDownloaderTests.cs index 0cc12bb280..8aac621949 100644 --- a/Tests/Core/Net/NetAsyncDownloaderTests.cs +++ b/Tests/Core/Net/NetAsyncDownloaderTests.cs @@ -28,16 +28,22 @@ public void DownloadAndWait_WithValidFileUrl_SetsTargetSize(string pathWithinTes var target = new CKAN.Net.DownloadTarget(new List { 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 { @@ -46,10 +52,19 @@ 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) { @@ -59,19 +74,26 @@ public void DownloadAndWait_WithValidFileUrls_SetsTargetsSize(params string[] pa var targets = fromPaths.Select(p => new CKAN.Net.DownloadTarget(new List { new Uri(p) }, 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 { @@ -81,5 +103,109 @@ 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 => TestData.DataDir(p)).ToArray(); + var targets = fromPaths.Select(p => new CKAN.Net.DownloadTarget(new List { new Uri(p) }, + Path.GetTempFileName())) + .ToArray(); + var badIndices = fromPaths.Select((p, i) => new Tuple(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(() => + { + downloader.DownloadAndWait(targets); + }); + CollectionAssert.AreEquivalent(badIndices, exception.Exceptions.Select(kvp => kvp.Key).ToArray()); + foreach (var t in validTargets) + { + Assert.IsTrue(File.Exists(t.filename)); + } + } } }