Fix cancellation of downloads.

The Download class cancels asynchronously, which means callers must handle cancellation inside the completion event, and not after requesting cancellation.

OpenRA.Game/Download.cs

@@ -17,8 +17,8 @@ namespace OpenRA
 {
 	public class Download
 	{
+		readonly object syncObject = new object();
 		WebClient wc;
-		bool cancelled;
 
 		public static string FormatErrorMessage(Exception e)
 		{
@@ -28,6 +28,8 @@ namespace OpenRA
 
 			switch (ex.Status)
 			{
+				case WebExceptionStatus.RequestCanceled:
+					return "Cancelled";
 				case WebExceptionStatus.NameResolutionFailure:
 					return "DNS lookup failed";
 				case WebExceptionStatus.Timeout:
@@ -41,40 +43,42 @@ namespace OpenRA
 			}
 		}
 
-		public Download(string url, string path, Action<DownloadProgressChangedEventArgs> onProgress, Action<AsyncCompletedEventArgs, bool> onComplete)
+		public Download(string url, string path, Action<DownloadProgressChangedEventArgs> onProgress, Action<AsyncCompletedEventArgs> onComplete)
 		{
-			wc = new WebClient();
-			wc.Proxy = null;
-
-			wc.DownloadProgressChanged += (_, a) => onProgress(a);
-			wc.DownloadFileCompleted += (_, a) => onComplete(a, cancelled);
-
-			Game.OnQuit += Cancel;
-			wc.DownloadFileCompleted += (_, a) => { Game.OnQuit -= Cancel; };
-
-			wc.DownloadFileAsync(new Uri(url), path);
+			lock (syncObject)
+			{
+				wc = new WebClient { Proxy = null };
+				wc.DownloadProgressChanged += (_, a) => onProgress(a);
+				wc.DownloadFileCompleted += (_, a) => { DisposeWebClient(); onComplete(a); };
+				wc.DownloadFileAsync(new Uri(url), path);
+			}
 		}
 
-		public Download(string url, Action<DownloadProgressChangedEventArgs> onProgress, Action<DownloadDataCompletedEventArgs, bool> onComplete)
+		public Download(string url, Action<DownloadProgressChangedEventArgs> onProgress, Action<DownloadDataCompletedEventArgs> onComplete)
 		{
-			wc = new WebClient();
-			wc.Proxy = null;
-
-			wc.DownloadProgressChanged += (_, a) => onProgress(a);
-			wc.DownloadDataCompleted += (_, a) => onComplete(a, cancelled);
-
-			Game.OnQuit += Cancel;
-			wc.DownloadDataCompleted += (_, a) => { Game.OnQuit -= Cancel; };
-
-			wc.DownloadDataAsync(new Uri(url));
+			lock (syncObject)
+			{
+				wc = new WebClient { Proxy = null };
+				wc.DownloadProgressChanged += (_, a) => onProgress(a);
+				wc.DownloadDataCompleted += (_, a) => { DisposeWebClient(); onComplete(a); };
+				wc.DownloadDataAsync(new Uri(url));
+			}
 		}
 
-		public void Cancel()
+		void DisposeWebClient()
 		{
-			Game.OnQuit -= Cancel;
-			wc.CancelAsync();
-			wc.Dispose();
-			cancelled = true;
+			lock (syncObject)
+			{
+				wc.Dispose();
+				wc = null;
+			}
+		}
+
+		public void CancelAsync()
+		{
+			lock (syncObject)
+				if (wc != null)
+					wc.CancelAsync();
 		}
 	}
 }

OpenRA.Game/Map/MapCache.cs

@@ -128,11 +128,11 @@ namespace OpenRA
 
 			var url = Game.Settings.Game.MapRepository + "hash/" + string.Join(",", maps.Keys) + "/yaml";
 
-			Action<DownloadDataCompletedEventArgs, bool> onInfoComplete = (i, cancelled) =>
+			Action<DownloadDataCompletedEventArgs> onInfoComplete = i =>
 			{
-				if (cancelled || i.Error != null)
+				if (i.Error != null)
 				{
-					Log.Write("debug", "Remote map query failed with error: {0}", i.Error != null ? i.Error.Message : "cancelled");
+					Log.Write("debug", "Remote map query failed with error: {0}", Download.FormatErrorMessage(i.Error));
 					Log.Write("debug", "URL was: {0}", url);
 					foreach (var p in maps.Values)
 						p.UpdateRemoteSearch(MapStatus.Unavailable, null);

OpenRA.Game/Map/MapPreview.cs

@@ -444,13 +444,13 @@ namespace OpenRA
 					}
 
 					Action<DownloadProgressChangedEventArgs> onDownloadProgress = i => { DownloadBytes = i.BytesReceived; DownloadPercentage = i.ProgressPercentage; };
-					Action<DownloadDataCompletedEventArgs, bool> onDownloadComplete = (i, cancelled) =>
+					Action<DownloadDataCompletedEventArgs> onDownloadComplete = i =>
 					{
 						download = null;
 
-						if (cancelled || i.Error != null)
+						if (i.Error != null)
 						{
-							Log.Write("debug", "Remote map download failed with error: {0}", i.Error != null ? i.Error.Message : "cancelled");
+							Log.Write("debug", "Remote map download failed with error: {0}", Download.FormatErrorMessage(i.Error));
 							Log.Write("debug", "URL was: {0}", mapUrl);
 
 							innerData.Status = MapStatus.DownloadError;
@@ -487,7 +487,7 @@ namespace OpenRA
 			if (download == null)
 				return;
 
-			download.Cancel();
+			download.CancelAsync();
 			download = null;
 		}
 

OpenRA.Mods.Common/Widgets/Logic/Installation/DownloadPackageLogic.cs

@@ -115,17 +115,19 @@ namespace OpenRA.Mods.Common.Widgets.Logic
 				retryButton.IsVisible = () => true;
 			});
 
-			Action<AsyncCompletedEventArgs, bool> onDownloadComplete = (i, cancelled) =>
+			Action<AsyncCompletedEventArgs> onDownloadComplete = i =>
 			{
-				if (i.Error != null)
+				if (i.Cancelled)
 				{
-					onError(Download.FormatErrorMessage(i.Error));
+					deleteTempFile();
+					Game.RunAfterTick(Ui.CloseWindow);
 					return;
 				}
 
-				if (cancelled)
+				if (i.Error != null)
 				{
-					onError("Download cancelled");
+					deleteTempFile();
+					onError(Download.FormatErrorMessage(i.Error));
 					return;
 				}
 
@@ -185,30 +187,30 @@ namespace OpenRA.Mods.Common.Widgets.Logic
 
 				downloadHost = new Uri(url).Host;
 				var dl = new Download(url, file, onDownloadProgress, onDownloadComplete);
-				cancelButton.OnClick = () => { dl.Cancel(); deleteTempFile(); Ui.CloseWindow(); };
-				retryButton.OnClick = () => { dl.Cancel(); ShowDownloadDialog(); };
+				cancelButton.OnClick = dl.CancelAsync;
+				retryButton.OnClick = ShowDownloadDialog;
 			};
 
 			if (download.MirrorList != null)
 			{
 				Log.Write("install", "Fetching mirrors from " + download.MirrorList);
 
-				Action<DownloadDataCompletedEventArgs, bool> onFetchMirrorsComplete = (i, cancelled) =>
+				Action<DownloadDataCompletedEventArgs> onFetchMirrorsComplete = i =>
 				{
 					progressBar.Indeterminate = true;
 
+					if (i.Cancelled)
+					{
+						Game.RunAfterTick(Ui.CloseWindow);
+						return;
+					}
+
 					if (i.Error != null)
 					{
 						onError(Download.FormatErrorMessage(i.Error));
 						return;
 					}
 
-					if (cancelled)
-					{
-						onError("Download cancelled");
-						return;
-					}
-
 					try
 					{
 						var data = Encoding.UTF8.GetString(i.Result);
@@ -224,8 +226,8 @@ namespace OpenRA.Mods.Common.Widgets.Logic
 				};
 
 				var updateMirrors = new Download(download.MirrorList, onDownloadProgress, onFetchMirrorsComplete);
-				cancelButton.OnClick = () => { updateMirrors.Cancel(); Ui.CloseWindow(); };
-				retryButton.OnClick = () => { updateMirrors.Cancel(); ShowDownloadDialog(); };
+				cancelButton.OnClick = updateMirrors.CancelAsync;
+				retryButton.OnClick = ShowDownloadDialog;
 			}
 			else
 				downloadUrl(download.URL);

OpenRA.Mods.Common/Widgets/Logic/MainMenuLogic.cs

@@ -294,8 +294,8 @@ namespace OpenRA.Mods.Common.Widgets.Logic
 								.JoinWith("&");
 
 						new Download(newsURL, cacheFile, e => { },
-							(e, c) => NewsDownloadComplete(e, cacheFile, currentNews,
-							() => newsButton.AttachPanel(newsPanel)));
+							e => NewsDownloadComplete(e, cacheFile, currentNews,
+								() => newsButton.AttachPanel(newsPanel)));
 					}
 
 					newsButton.OnClick = () => newsButton.AttachPanel(newsPanel);

OpenRA.Mods.Common/Widgets/Logic/MultiplayerLogic.cs

@@ -296,11 +296,11 @@ namespace OpenRA.Mods.Common.Widgets.Logic
 
 			searchStatus = SearchStatus.Fetching;
 
-			Action<DownloadDataCompletedEventArgs, bool> onComplete = (i, cancelled) =>
+			Action<DownloadDataCompletedEventArgs> onComplete = i =>
 			{
 				currentQuery = null;
 
-				if (i.Error != null || cancelled)
+				if (i.Error != null)
 				{
 					RefreshServerListInner(null);
 					return;