Merge #3635 Many improvements for failed downloads

KSP-CKAN · Aug 19, 2022 · 6bc1dbf · 6bc1dbf
2 parents 1d4b745 + 30d0376
commit 6bc1dbf
Show file tree

Hide file tree

Showing 28 changed files with 768 additions and 307 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -13,6 +13,7 @@ All notable changes to this project will be documented in this file.
 - [CLI] Create a system menu entry for command prompt (#3622 by: HebaruSan; reviewed: techman83)
 - [Multiple] Internationalize Core, CmdLine, ConsoleUI, and AutoUpdater (#3482 by: HebaruSan; reviewed: techman83)
 - [Multiple] Check free space before downloading (#3631 by: HebaruSan; reviewed: techman83)
+- [Multiple] Many improvements for failed downloads (#3635 by: HebaruSan; reviewed: techman83)
 
 ## Bugfixes
 

diff --git a/Cmdline/Main.cs b/Cmdline/Main.cs
@@ -288,7 +288,10 @@ private static int Gui(GameInstanceManager manager, GuiOptions options, string[]
             // TODO: Sometimes when the GUI exits, we get a System.ArgumentException,
             // but trying to catch it here doesn't seem to help. Dunno why.
 
-            GUI.GUI.Main_(args, manager, options.ShowConsole);
+            // GUI expects its first param to be an identifier, don't confuse it
+            GUI.GUI.Main_(args.Except(new string[] {"--verbose", "--debug", "--show-console"})
+                              .ToArray(),
+                          manager, options.ShowConsole);
 
             return Exit.OK;
         }

diff --git a/ConsoleUI/InstallScreen.cs b/ConsoleUI/InstallScreen.cs
@@ -1,6 +1,7 @@
 using System;
 using System.Transactions;
 using System.Collections.Generic;
+
 using CKAN.ConsoleUI.Toolkit;
 
 namespace CKAN.ConsoleUI {

diff --git a/Core/HelpURLs.cs b/Core/HelpURLs.cs
@@ -17,6 +17,7 @@ public static class HelpURLs
         public const string Filters = "https://github.com/KSP-CKAN/CKAN/pull/3458";
         public const string Labels = "https://github.com/KSP-CKAN/CKAN/pull/2936";
         public const string PlayTime = "https://github.com/KSP-CKAN/CKAN/pull/3543";
+        public const string DownloadsFailed = "https://github.com/KSP-CKAN/CKAN/pull/3635";
 
         public const string Discord = "https://discord.gg/Mb4nXQD";
     }

diff --git a/Core/ModuleInstaller.cs b/Core/ModuleInstaller.cs
@@ -4,11 +4,13 @@
 using System.Linq;
 using System.Text.RegularExpressions;
 using System.Transactions;
+
 using ICSharpCode.SharpZipLib.Core;
 using ICSharpCode.SharpZipLib.Zip;
 using log4net;
 using ChinhDo.Transactions.FileManager;
 using Autofac;
+
 using CKAN.Extensions;
 using CKAN.Versioning;
 using CKAN.Configuration;

diff --git a/Core/Net/NetAsyncDownloader.cs b/Core/Net/NetAsyncDownloader.cs
@@ -105,6 +105,7 @@ private void ResetAgent()
         /// </summary>
         public event Action<Net.DownloadTarget, long, long> Progress;
 
+        private readonly object dlMutex = new object();
         private List<NetAsyncDownloaderDownloadPart> downloads = new List<NetAsyncDownloaderDownloadPart>();
         private List<Net.DownloadTarget> queuedDownloads = new List<Net.DownloadTarget>();
         private int completed_downloads;
@@ -143,11 +144,13 @@ private void DownloadModule(Net.DownloadTarget target)
         {
             if (shouldQueue(target))
             {
+                log.DebugFormat("Enqueuing download of {0}", target.url);
                 // Throttled host already downloading, we will get back to this later
                 queuedDownloads.Add(target);
             }
             else
             {
+                log.DebugFormat("Beginning download of {0}", target.url);
                 // We need a new variable for our closure/lambda, hence index = 1+prev max
                 int index = downloads.Count;
 
@@ -167,7 +170,7 @@ private void DownloadModule(Net.DownloadTarget target)
 
                 // And schedule a notification if we're done (or if something goes wrong)
                 dl.Done += (sender, args) =>
-                    FileDownloadComplete(index, args.Error);
+                    FileDownloadComplete(index, args.Error, args.Cancelled);
 
                 // Start the download!
                 dl.Download(dl.target.url, dl.path);
@@ -187,7 +190,9 @@ private bool shouldQueue(Net.DownloadTarget target)
         {
             return downloads.Any(dl =>
                     dl.target.url.Host == target.url.Host
-                    && dl.bytesLeft > 0);
+                    && dl.bytesLeft > 0
+                    // Consider done if already tried and failed
+                    && dl.error == null);
         }
 
         /// <summary>
@@ -196,7 +201,7 @@ private bool shouldQueue(Net.DownloadTarget target)
         /// <param name="urls">The downloads to begin</param>
         public void DownloadAndWait(ICollection<Net.DownloadTarget> urls)
         {
-            if (downloads.Count > completed_downloads)
+            if (downloads.Count + queuedDownloads.Count > completed_downloads)
             {
                 // Some downloads are still in progress, add to the current batch
                 foreach (Net.DownloadTarget target in urls)
@@ -230,6 +235,8 @@ public void DownloadAndWait(ICollection<Net.DownloadTarget> urls)
             // If the user cancelled our progress, then signal that.
             if (old_download_canceled)
             {
+                // Ditch anything we haven't started
+                queuedDownloads.Clear();
                 // Abort all our traditional downloads, if there are any.
                 foreach (var download in downloads.ToList())
                 {
@@ -378,51 +385,55 @@ private void FileProgressReport(int index, int percent, long bytesDownloaded, lo
         /// This method gets called back by `WebClient` when a download is completed.
         /// It in turncalls the onCompleted hook when *all* downloads are finished.
         /// </summary>
-        private void FileDownloadComplete(int index, Exception error)
+        private void FileDownloadComplete(int index, Exception error, bool canceled)
         {
-            if (error != null)
+            // Make sure the threads don't trip on one another
+            lock (dlMutex)
             {
-                log.InfoFormat("Error downloading {0}: {1}", downloads[index].target.url, error);
-
-                // Check whether we were already downloading the fallback url
-                if (!downloads[index].triedFallback && downloads[index].target.fallbackUrl != null)
+                if (error != null)
                 {
-                    log.InfoFormat("Trying fallback URL: {0}", downloads[index].target.fallbackUrl);
-                    // Encode spaces to avoid confusing URL parsers
-                    User.RaiseMessage(Properties.Resources.NetAsyncDownloaderTryingFallback,
-                        downloads[index].target.url.ToString().Replace(" ", "%20"),
-                        downloads[index].target.fallbackUrl.ToString().Replace(" ", "%20")
-                    );
-                    // Try the fallbackUrl
-                    downloads[index].triedFallback = true;
-                    downloads[index].Download(downloads[index].target.fallbackUrl, downloads[index].path);
-                    // Short circuit the completion process so the fallback can run
-                    return;
+                    log.InfoFormat("Error downloading {0}: {1}", downloads[index].target.url, error);
+
+                    // Check whether we were already downloading the fallback url
+                    if (!canceled && !downloads[index].triedFallback && downloads[index].target.fallbackUrl != null)
+                    {
+                        log.InfoFormat("Trying fallback URL: {0}", downloads[index].target.fallbackUrl);
+                        // Encode spaces to avoid confusing URL parsers
+                        User.RaiseMessage(Properties.Resources.NetAsyncDownloaderTryingFallback,
+                            downloads[index].target.url.ToString().Replace(" ", "%20"),
+                            downloads[index].target.fallbackUrl.ToString().Replace(" ", "%20")
+                        );
+                        // Try the fallbackUrl
+                        downloads[index].triedFallback = true;
+                        downloads[index].Download(downloads[index].target.fallbackUrl, downloads[index].path);
+                        // Short circuit the completion process so the fallback can run
+                        return;
+                    }
+                    else
+                    {
+                        downloads[index].error = error;
+                    }
                 }
                 else
                 {
-                    downloads[index].error = error;
+                    log.InfoFormat("Finished downloading {0}", downloads[index].target.url);
                 }
-            }
-            else
-            {
-                log.InfoFormat("Finished downloading {0}", downloads[index].target.url);
-            }
 
-            var next = queuedDownloads.FirstOrDefault(dl =>
-                dl.url.Host == downloads[index].target.url.Host);
-            if (next != null)
-            {
-                // Start this host's next queued download
-                queuedDownloads.Remove(next);
-                DownloadModule(next);
-            }
+                var next = queuedDownloads.FirstOrDefault(dl =>
+                    dl.url.Host == downloads[index].target.url.Host);
+                if (next != null)
+                {
+                    // Start this host's next queued download
+                    queuedDownloads.Remove(next);
+                    DownloadModule(next);
+                }
 
-            onOneCompleted.Invoke(downloads[index].target.url, downloads[index].path, downloads[index].error);
+                onOneCompleted.Invoke(downloads[index].target.url, downloads[index].path, downloads[index].error);
 
-            if (++completed_downloads >= downloads.Count + queuedDownloads.Count)
-            {
-                triggerCompleted();
+                if (++completed_downloads >= downloads.Count + queuedDownloads.Count)
+                {
+                    triggerCompleted();
+                }
             }
         }
     }

diff --git a/Core/Net/NetAsyncModulesDownloader.cs b/Core/Net/NetAsyncModulesDownloader.cs
@@ -91,15 +91,16 @@ public void DownloadModules(IEnumerable<CkanModule> modules)
                             : $"{item.Value.download_content_type};q=1.0,{defaultMimeType};q=0.9"
                     )).ToList()
                 );
-                if (AllComplete != null)
-                {
-                    AllComplete();
-                }
+                this.modules.Clear();
+                AllComplete?.Invoke();
             }
             catch (DownloadErrorsKraken kraken)
             {
                 // Associate the errors with the affected modules
-                throw new ModuleDownloadErrorsKraken(this.modules, kraken);
+                var exc = new ModuleDownloadErrorsKraken(this.modules.ToList(), kraken);
+                // Clear this.modules because we're done with these
+                this.modules.Clear();
+                throw exc;
             }
         }
 

diff --git a/Core/Registry/AvailableModule.cs b/Core/Registry/AvailableModule.cs
@@ -97,7 +97,6 @@ public CkanModule Latest(
             IEnumerable<CkanModule> toInstall    = null
         )
         {
-            log.DebugFormat("Our dictionary has {0} keys", module_version.Keys.Count);
             IEnumerable<CkanModule> modules = module_version.Values.Reverse();
             if (relationship != null)
             {
@@ -128,6 +127,7 @@ private static bool DependsAndConflictsOK(CkanModule module, IEnumerable<CkanMod
                     // If 'others' matches an identifier, it must also match the versions, else fail
                     if (rel.ContainsAny(others.Select(m => m.identifier)) && !rel.MatchesAny(others, null, null))
                     {
+                        log.DebugFormat("Unsatisfied dependency {0}, rejecting", rel);
                         return false;
                     }
                 }
@@ -139,8 +139,9 @@ private static bool DependsAndConflictsOK(CkanModule module, IEnumerable<CkanMod
                 foreach (RelationshipDescriptor rel in module.conflicts)
                 {
                     // If any of the conflicts are present, fail
-                    if (rel.MatchesAny(othersMinusSelf, null, null))
+                    if (rel.MatchesAny(othersMinusSelf, null, null, out CkanModule matched))
                     {
+                        log.DebugFormat("Found conflict with {0}, rejecting", matched);
                         return false;
                     }
                 }
@@ -155,6 +156,7 @@ private static bool DependsAndConflictsOK(CkanModule module, IEnumerable<CkanMod
                     {
                         if (rel.MatchesAny(selfArray, null, null))
                         {
+                            log.DebugFormat("Found reverse conflict with {0}, rejecting", other);
                             return false;
                         }
                     }

diff --git a/Core/Registry/IRegistryQuerier.cs b/Core/Registry/IRegistryQuerier.cs
@@ -79,7 +79,7 @@ List<CkanModule> LatestAvailableWithProvides(
         /// Finds and returns all modules that could not exist without the listed modules installed, including themselves.
         /// </summary>
         IEnumerable<string> FindReverseDependencies(
-            IEnumerable<string> modulesToRemove, IEnumerable<CkanModule> modulesToInstall = null
+            IEnumerable<string> modulesToRemove, IEnumerable<CkanModule> modulesToInstall = null, Func<RelationshipDescriptor, bool> satisfiedFilter = null
         );
 
         /// <summary>

diff --git a/Core/Registry/Registry.cs b/Core/Registry/Registry.cs
@@ -1094,11 +1094,12 @@ internal static IEnumerable<string> FindReverseDependencies(
             IEnumerable<CkanModule> modulesToInstall,
             IEnumerable<CkanModule> origInstalled,
             IEnumerable<string> dlls,
-            IDictionary<string, ModuleVersion> dlc
+            IDictionary<string, ModuleVersion> dlc,
+            Func<RelationshipDescriptor, bool> satisfiedFilter = null
         )
         {
             modulesToRemove = modulesToRemove.Memoize();
-            origInstalled    = origInstalled.Memoize();
+            origInstalled   = origInstalled.Memoize();
             var dllSet = dlls.ToHashSet();
             // The empty list has no reverse dependencies
             // (Don't remove broken modules if we're only installing)
@@ -1112,7 +1113,8 @@ IDictionary<string, ModuleVersion> dlc
                 while (true)
                 {
                     // Make our hypothetical install, and remove the listed modules from it.
-                    HashSet<CkanModule> hypothetical = new HashSet<CkanModule>(origInstalled); // Clone because we alter hypothetical.
+                    // Clone because we alter hypothetical.
+                    HashSet<CkanModule> hypothetical = new HashSet<CkanModule>(origInstalled);
                     if (modulesToInstall != null)
                     {
                         // Pretend the mods we are going to install are already installed, so that dependencies that will be
@@ -1124,34 +1126,38 @@ IDictionary<string, ModuleVersion> dlc
                     log.DebugFormat("Started with {0}, removing {1}, and keeping {2}; our dlls are {3}", string.Join(", ", origInstalled), string.Join(", ", modulesToRemove), string.Join(", ", hypothetical), string.Join(", ", dllSet));
 
                     // Find what would break with this configuration.
-                    var broken = SanityChecker.FindUnsatisfiedDepends(hypothetical, dllSet, dlc)
-                        .Select(x => x.Key.identifier).ToHashSet();
+                    var brokenDeps = SanityChecker.FindUnsatisfiedDepends(hypothetical, dllSet, dlc);
+                    if (satisfiedFilter != null)
+                    {
+                        brokenDeps.RemoveAll(kvp => satisfiedFilter(kvp.Value));
+                    }
+                    var brokenIdents = brokenDeps.Select(x => x.Key.identifier).ToHashSet();
 
                     if (modulesToInstall != null)
                     {
                         // Make sure to only report modules as broken if they are actually currently installed.
                         // This is mainly to remove the modulesToInstall again which we added
                         // earlier to the hypothetical list.
-                        broken.IntersectWith(origInstalled.Select(m => m.identifier));
+                        brokenIdents.IntersectWith(origInstalled.Select(m => m.identifier));
                     }
                     // Lazily return each newly found rev dep
-                    foreach (string newFound in broken.Except(modulesToRemove))
+                    foreach (string newFound in brokenIdents.Except(modulesToRemove))
                     {
                         yield return newFound;
                     }
 
                     // If nothing else would break, it's just the list of modules we're removing.
                     HashSet<string> to_remove = new HashSet<string>(modulesToRemove);
 
-                    if (to_remove.IsSupersetOf(broken))
+                    if (to_remove.IsSupersetOf(brokenIdents))
                     {
-                        log.DebugFormat("{0} is a superset of {1}, work done", string.Join(", ", to_remove), string.Join(", ", broken));
+                        log.DebugFormat("{0} is a superset of {1}, work done", string.Join(", ", to_remove), string.Join(", ", brokenIdents));
                         break;
                     }
 
                     // Otherwise, remove our broken modules as well, and recurse.
-                    broken.UnionWith(to_remove);
-                    modulesToRemove = broken;
+                    brokenIdents.UnionWith(to_remove);
+                    modulesToRemove = brokenIdents;
                 }
             }
         }
@@ -1161,11 +1167,12 @@ IDictionary<string, ModuleVersion> dlc
         /// </summary>
         public IEnumerable<string> FindReverseDependencies(
             IEnumerable<string> modulesToRemove,
-            IEnumerable<CkanModule> modulesToInstall = null
+            IEnumerable<CkanModule> modulesToInstall = null,
+            Func<RelationshipDescriptor, bool> satisfiedFilter = null
         )
         {
             var installed = new HashSet<CkanModule>(installed_modules.Values.Select(x => x.Module));
-            return FindReverseDependencies(modulesToRemove, modulesToInstall, installed, new HashSet<string>(installed_dlls.Keys), InstalledDlc);
+            return FindReverseDependencies(modulesToRemove, modulesToInstall, installed, new HashSet<string>(installed_dlls.Keys), InstalledDlc, satisfiedFilter);
         }
 
         /// <summary>

diff --git a/Core/Types/CkanModule.cs b/Core/Types/CkanModule.cs
@@ -544,9 +544,10 @@ internal static bool UniConflicts(CkanModule mod1, CkanModule mod2)
         /// </summary>
         public bool IsCompatibleKSP(GameVersionCriteria version)
         {
-            log.DebugFormat("Testing if {0} is compatible with game versions {1}", this, version.ToString());
-
-            return _comparator.Compatible(version, this);
+            var compat = _comparator.Compatible(version, this);
+            log.DebugFormat("Checking compat of {0} with game versions {1}: {2}",
+                this, version.ToString(), compat ? "Compatible": "Incompatible");
+            return compat;
         }
 
         /// <summary>