From 923f069ee780a7204903df30ce0807a376ddf19f Mon Sep 17 00:00:00 2001 From: Paul Hebble Date: Wed, 7 Feb 2018 01:04:25 +0000 Subject: [PATCH] Clean up download error reporting --- Cmdline/Action/Install.cs | 5 ++ ConsoleUI/InstallScreen.cs | 2 + Core/Net/NetAsyncDownloader.cs | 68 ++++++++++++++++----------- Core/Net/NetAsyncModulesDownloader.cs | 66 ++++++++++---------------- Core/Types/Kraken.cs | 63 +++++++++++++++++++++++-- GUI/MainInstall.cs | 4 +- 6 files changed, 134 insertions(+), 74 deletions(-) diff --git a/Cmdline/Action/Install.cs b/Cmdline/Action/Install.cs index cb7b01030e..b7f1655f27 100644 --- a/Cmdline/Action/Install.cs +++ b/Cmdline/Action/Install.cs @@ -249,6 +249,11 @@ public int RunCommand(CKAN.KSP ksp, object raw_options) user.RaiseMessage("One or more files failed to download, stopped."); return Exit.ERROR; } + catch (ModuleDownloadErrorsKraken kraken) + { + user.RaiseMessage(kraken.ToString()); + return Exit.ERROR; + } catch (DirectoryNotFoundKraken kraken) { user.RaiseMessage("\r\n{0}", kraken.Message); diff --git a/ConsoleUI/InstallScreen.cs b/ConsoleUI/InstallScreen.cs index a8c11ab13d..c6dcc4510a 100644 --- a/ConsoleUI/InstallScreen.cs +++ b/ConsoleUI/InstallScreen.cs @@ -94,6 +94,8 @@ public override void Run(Action process = null) RaiseError("Game files reverted."); } catch (DownloadErrorsKraken ex) { RaiseError(ex.ToString()); + } catch (ModuleDownloadErrorsKraken ex) { + RaiseError(ex.ToString()); } catch (MissingCertificateKraken ex) { RaiseError(ex.ToString()); } catch (InconsistentKraken ex) { diff --git a/Core/Net/NetAsyncDownloader.cs b/Core/Net/NetAsyncDownloader.cs index ae6bc963d1..b5487265a4 100644 --- a/Core/Net/NetAsyncDownloader.cs +++ b/Core/Net/NetAsyncDownloader.cs @@ -294,20 +294,23 @@ public void DownloadAndWait(ICollection urls) } // Check to see if we've had any errors. If so, then release the kraken! - var exceptions = downloads - .Select(x => x.error) - .Where(ex => ex != null) - .ToList(); - - // Let's check if any of these are certificate errors. If so, - // we'll report that instead, as this is common (and user-fixable) - // under Linux. - if (exceptions.Any(ex => ex is WebException && - Regex.IsMatch(ex.Message, "authentication or decryption has failed"))) + List> exceptions = new List>(); + for (int i = 0; i < downloads.Count; ++i) { - throw new MissingCertificateKraken(); + if (downloads[i].error != null) + { + // Check if it's a certificate error. If so, report that instead, + // as this is common (and user-fixable) under Linux. + if (downloads[i].error is WebException + && certificatePattern.IsMatch(downloads[i].error.Message)) + { + throw new MissingCertificateKraken(); + } + // 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)); + } } - if (exceptions.Count > 0) { throw new DownloadErrorsKraken(exceptions); @@ -316,6 +319,11 @@ public void DownloadAndWait(ICollection urls) // Yay! Everything worked! } + private static readonly Regex certificatePattern = new Regex( + @"authentication or decryption has failed", + RegexOptions.Compiled + ); + /// /// /// This will also call onCompleted with all null arguments. @@ -409,32 +417,36 @@ private void FileDownloadComplete(int index, Exception error) { log.InfoFormat("Finished downloading {0}", downloads[index].url); } - completed_downloads++; // If there was an error, remember it, but we won't raise it until // all downloads are finished or cancelled. downloads[index].error = error; - if (completed_downloads == downloads.Count) + if (++completed_downloads == downloads.Count) { - log.Info("All files finished downloading"); - - // If we have a callback, then signal that we're done. + FinalizeDownloads(); + } + } - var fileUrls = new Uri[downloads.Count]; - var filePaths = new string[downloads.Count]; - var errors = new Exception[downloads.Count]; + private void FinalizeDownloads() + { + log.Info("All files finished downloading"); - for (int i = 0; i < downloads.Count; i++) - { - fileUrls[i] = downloads[i].url; - filePaths[i] = downloads[i].path; - errors[i] = downloads[i].error; - } + Uri[] fileUrls = new Uri[downloads.Count]; + string[] filePaths = new string[downloads.Count]; + Exception[] errors = new Exception[downloads.Count]; - log.Debug("Signalling completion via callback"); - triggerCompleted(fileUrls, filePaths, errors); + for (int i = 0; i < downloads.Count; ++i) + { + fileUrls[i] = downloads[i].url; + filePaths[i] = downloads[i].path; + errors[i] = downloads[i].error; } + + // If we have a callback, then signal that we're done. + log.Debug("Signalling completion via callback"); + triggerCompleted(fileUrls, filePaths, errors); } + } } diff --git a/Core/Net/NetAsyncModulesDownloader.cs b/Core/Net/NetAsyncModulesDownloader.cs index 38429e2032..eed0fdab3a 100644 --- a/Core/Net/NetAsyncModulesDownloader.cs +++ b/Core/Net/NetAsyncModulesDownloader.cs @@ -51,20 +51,28 @@ public void DownloadModules(NetModuleCache cache, IEnumerable module (_uris, paths, errors) => ModuleDownloadsComplete(cache, _uris, paths, errors); - // Start the downloads! - downloader.DownloadAndWait( - unique_downloads.Select(item => new Net.DownloadTarget( - item.Key, - // Use a temp file name - null, - item.Value.download_size, - // Send the MIME type to use for the Accept header - // The GitHub API requires this to include application/octet-stream - string.IsNullOrEmpty(item.Value.download_content_type) - ? defaultMimeType - : $"{item.Value.download_content_type};q=1.0,{defaultMimeType};q=0.9" - )).ToList() - ); + try + { + // Start the downloads! + downloader.DownloadAndWait( + unique_downloads.Select(item => new Net.DownloadTarget( + item.Key, + // Use a temp file name + null, + item.Value.download_size, + // Send the MIME type to use for the Accept header + // The GitHub API requires this to include application/octet-stream + string.IsNullOrEmpty(item.Value.download_content_type) + ? defaultMimeType + : $"{item.Value.download_content_type};q=1.0,{defaultMimeType};q=0.9" + )).ToList() + ); + } + catch (DownloadErrorsKraken kraken) + { + // Associate the errors with the affected modules + throw new ModuleDownloadErrorsKraken(this.modules, kraken); + } } /// @@ -74,33 +82,13 @@ public void DownloadModules(NetModuleCache cache, IEnumerable module /// private void ModuleDownloadsComplete(NetModuleCache cache, Uri[] urls, string[] filenames, Exception[] errors) { - if (urls != null) + if (filenames != null) { - // spawn up to 3 dialogs - int errorDialogsLeft = 3; - for (int i = 0; i < errors.Length; i++) { - if (errors[i] != null) - { - if (errorDialogsLeft > 0) - { - User.RaiseError("Failed to download \"{0}\" - error: {1}", urls[i], errors[i].Message); - errorDialogsLeft--; - } - } - else + if (errors[i] == null) { - // Even if some of our downloads failed, we want to cache the - // ones which succeeded. - - // This doesn't work :( - // for some reason the tmp files get deleted before we get here and we get a nasty exception - // not only that but then we try _to install_ the rest of the mods and then CKAN crashes - // and the user's registry gets corrupted forever - // commenting out until this is resolved - // ~ nlight - + // Cache the downloads that succeeded. try { cache.Store(modules[i], filenames[i], modules[i].StandardName()); @@ -111,14 +99,10 @@ private void ModuleDownloadsComplete(NetModuleCache cache, Uri[] urls, string[] } } } - } - if (filenames != null) - { // Finally, remove all our temp files. // We probably *could* have used Store's integrated move function above, but if we managed // to somehow get two URLs the same in our download set, that could cause right troubles! - foreach (string tmpfile in filenames) { log.DebugFormat("Cleaning up {0}", tmpfile); diff --git a/Core/Types/Kraken.cs b/Core/Types/Kraken.cs index 17f92499ff..41a695b981 100644 --- a/Core/Types/Kraken.cs +++ b/Core/Types/Kraken.cs @@ -1,4 +1,5 @@ using System; +using System.Text; using System.Collections.Generic; namespace CKAN @@ -218,18 +219,72 @@ public FileExistsKraken(string filename, string reason = null, Exception innerEx /// public class DownloadErrorsKraken : Kraken { - public List exceptions; + public readonly List> exceptions + = new List>(); - public DownloadErrorsKraken(IEnumerable errors, string reason = null, Exception innerException = null) - : base(reason, innerException) + public DownloadErrorsKraken(List> errors) : base() { - exceptions = new List(errors); + exceptions = new List>(errors); } public override string ToString() { return "Uh oh, the following things went wrong when downloading...\r\n\r\n" + String.Join("\r\n", exceptions); } + + } + + /// + /// A download errors exception that knows about modules, + /// to make the error message nicer. + /// + public class ModuleDownloadErrorsKraken : Kraken + { + /// + /// Initialize the exception. + /// + /// List of modules that we tried to download + /// Download errors from URL-level downloader + public ModuleDownloadErrorsKraken(IList modules, DownloadErrorsKraken kraken) + : base() + { + foreach (var kvp in kraken.exceptions) + { + exceptions.Add(new KeyValuePair( + modules[kvp.Key], kvp.Value + )); + } + } + + /// + /// Generate a user friendly description of this error. + /// + /// + /// One or more downloads were unsuccessful: + /// + /// Error downloading Astrogator v0.7.8: The remote server returned an error: (404) Not Found. + /// Etc. + /// + public override string ToString() + { + if (builder == null) + { + builder = new StringBuilder(); + builder.AppendLine("One or more downloads were unsuccessful:"); + builder.AppendLine(""); + foreach (KeyValuePair kvp in exceptions) + { + builder.AppendLine( + $"Error downloading {kvp.Key.ToString()}: {kvp.Value.Message}" + ); + } + } + return builder.ToString(); + } + + private readonly List> exceptions + = new List>(); + private StringBuilder builder = null; } /// diff --git a/GUI/MainInstall.cs b/GUI/MainInstall.cs index 1df7d80ee8..bae9c8c7fc 100644 --- a/GUI/MainInstall.cs +++ b/GUI/MainInstall.cs @@ -194,9 +194,11 @@ private void InstallMods(object sender, DoWorkEventArgs e) // this probably need GUI.user.RaiseMessage(kraken.ToString()); return; } - catch (DownloadErrorsKraken) + catch (ModuleDownloadErrorsKraken kraken) { // User notified in InstallList + GUI.user.RaiseMessage(kraken.ToString()); + GUI.user.RaiseError(kraken.ToString()); return; } catch (DirectoryNotFoundKraken kraken)