Better to throw an exception when things don't work out

Author: StanleyGoldman

src/GitHub.Api/Tasks/DownloadTask.cs

@@ -90,11 +90,10 @@ public static WebResponse GetResponseWithoutException(this WebRequest request)
         }
     }
 
-    class DownloadTask : TaskBase<bool>
+    class DownloadTask : TaskBase
     {
         private readonly IFileSystem fileSystem;
         private long bytes;
-        private WebRequest webRequest;
         private bool restarted;
 
         public float Progress { get; set; }
@@ -110,19 +109,19 @@ public DownloadTask(CancellationToken token, IFileSystem fileSystem, string url,
             Name = "DownloadTask";
         }
 
-        protected override bool RunWithReturn(bool success)
+        protected override void Run(bool success)
         {
-            base.RunWithReturn(success);
+            base.Run(success);
 
             RaiseOnStart();
 
-            var result = false;
             var attempts = 0;
             try
             {
+                bool result;
                 do
                 {
-                    Logger.Trace($"Download of {Url} to {Destination} Attempt {attempts + 1} of {RetryCount + 1}");
+                    Logger.Trace($"Download of {Url} Attempt {attempts + 1} of {RetryCount + 1}");
                     result = Download();
                     if (result && ValidationHash != null)
                     {
@@ -140,20 +139,23 @@ protected override bool RunWithReturn(bool success)
                             break;
                         }
                     }
-                } while (RetryCount < attempts++);
+                } while (attempts++ < RetryCount);
+
+                if (!result)
+                {
+                    throw new DownloadException("Error downloading file");
+                }
             }
             catch (Exception ex)
             {
                 Errors = ex.Message;
-                if (!RaiseFaultHandlers(ex))
+                if (!RaiseFaultHandlers(new DownloadException("Error downloading file", ex)))
                     throw;
             }
             finally
             {
-                RaiseOnEnd(result);
+                RaiseOnEnd();
             }
-
-            return result;
         }
 
         protected virtual void UpdateProgress(float progress)
@@ -178,51 +180,42 @@ public bool Download()
                 }
             }
 
-            webRequest = WebRequest.Create(Url);
-            var httpWebRequest = webRequest as HttpWebRequest;
-            if (httpWebRequest != null)
+            var expectingResume = restarted && bytes > 0;
+
+            var webRequest = (HttpWebRequest)WebRequest.Create(Url);
+
+            if (expectingResume)
             {
-                if (bytes > 0)
-                {
-                    // TODO: fix classlibs to take long overloads
-                    httpWebRequest.AddRange((int)bytes);
-                }
+                // TODO: fix classlibs to take long overloads
+                webRequest.AddRange((int)bytes);
             }
 
             webRequest.Method = "GET";
             webRequest.Timeout = 3000;
 
-            if (restarted && bytes > 0)
+            if (expectingResume)
                 Logger.Trace($"Resuming download of {Url} to {Destination}");
             else
                 Logger.Trace($"Downloading {Url} to {Destination}");
 
-            using (var webResponse = webRequest.GetResponseWithoutException())
+            using (var webResponse = (HttpWebResponse) webRequest.GetResponseWithoutException())
             {
-                if (webResponse == null)
-                    return false;
+                var httpStatusCode = webResponse.StatusCode;
+                Logger.Trace($"Downloading {Url} StatusCode:{(int)webResponse.StatusCode}");
 
-                if (restarted && bytes > 0)
+                if (expectingResume && httpStatusCode == HttpStatusCode.RequestedRangeNotSatisfiable)
                 {
-                    var httpWebResponse = webResponse as HttpWebResponse;
-                    if (httpWebResponse != null)
-                    {
-                        var httpStatusCode = httpWebResponse.StatusCode;
-                        if (httpStatusCode == HttpStatusCode.RequestedRangeNotSatisfiable)
-                        {
-                            UpdateProgress(1);
-                            return true;
-                        }
+                    UpdateProgress(1);
+                    return true;
+                }
 
-                        if (!(httpStatusCode == HttpStatusCode.OK || httpStatusCode == HttpStatusCode.PartialContent))
-                        {
-                            return false;
-                        }
-                    }
+                if (!(httpStatusCode == HttpStatusCode.OK || httpStatusCode == HttpStatusCode.PartialContent))
+                {
+                    return false;
                 }
 
                 var responseLength = webResponse.ContentLength;
-                if (restarted && bytes > 0)
+                if (expectingResume)
                 {
                     UpdateProgress(bytes / (float)responseLength);
                 }
@@ -249,6 +242,15 @@ public bool Download()
         protected int RetryCount { get; }
     }
 
+    class DownloadException : Exception
+    {
+        public DownloadException(string message) : base(message)
+        { }
+
+        public DownloadException(string message, Exception innerException) : base(message, innerException)
+        { }
+    }
+
     class DownloadTextTask : TaskBase<string>
     {
         public float Progress { get; set; }

src/tests/IntegrationTests/Download/DownloadTaskTests.cs

@@ -26,9 +26,7 @@ public async Task TestDownloadTask()
             var downloadHalfPath = TestBasePath.Combine("5MB-split.zip");
 
             var downloadTask = new DownloadTask(CancellationToken.None, fileSystem, TestDownload, downloadPath);
-            var downloadResult = await downloadTask.StartAwait();
-
-            downloadResult.Should().BeTrue();
+            await downloadTask.StartAwait();
 
             var downloadPathBytes = fileSystem.ReadAllBytes(downloadPath);
             Logger.Trace("File size {0} bytes", downloadPathBytes.Length);
@@ -45,9 +43,7 @@ public async Task TestDownloadTask()
             fileSystem.WriteAllBytes(downloadHalfPath, cutDownloadPathBytes);
 
             downloadTask = new DownloadTask(CancellationToken.None, fileSystem, TestDownload, downloadHalfPath, TestDownloadMD5, 1);
-            downloadResult = await downloadTask.StartAwait();
-
-            downloadResult.Should().BeTrue();
+            await downloadTask.StartAwait();
 
             var downloadHalfPathBytes = fileSystem.ReadAllBytes(downloadHalfPath);
             Logger.Trace("File size {0} Bytes", downloadHalfPathBytes.Length);
@@ -56,6 +52,36 @@ public async Task TestDownloadTask()
             md5Sum.Should().Be(TestDownloadMD5.ToUpperInvariant());
         }
 
+        [Test]
+        public void TestDownloadFailure()
+        {
+            InitializeTaskManager();
+
+            var fileSystem = new FileSystem();
+
+            var downloadPath = TestBasePath.Combine("5MB.zip");
+
+            var taskFailed = false;
+            Exception exceptionThrown = null;
+
+            var autoResetEvent = new AutoResetEvent(false);
+
+            var downloadTask = new DownloadTask(CancellationToken.None, fileSystem, "http://www.unknown.com/5MB.gz", downloadPath, null, 1)
+                .Finally((b, exception) => {
+                    taskFailed = !b;
+                    exceptionThrown = exception;
+                    autoResetEvent.Set();
+                });
+
+            downloadTask.Start();
+
+            autoResetEvent.WaitOne();
+
+            taskFailed.Should().BeTrue();
+            exceptionThrown.Should().NotBeNull();
+        }
+
+
         [Test]
         public void TestDownloadTextTask()
         {