Retry mechanism for failed downloads

Question

I'm writing a mechanism to retry any failed downloads that respond with certain http error codes, I've noticed the server I'm downloading from will unexpectedly throw these codes to me and when I manually visit the URL it appears to be fine.

It also seems that its fine when trying to download for a second or (rarely) third time. The KeyValuePair represents the key as being the download link, and the value being the location where the file is set to be saved on the device.

I simply check if the file exists in the save location to determine if it actually downloaded.

public static class DownloadUtilities
{
    public static void DownloadLinks(Dictionary<string, string> files)
    {
        Parallel.ForEach(
            files,
            new ParallelOptions { MaxDegreeOfParallelism = 20 },
            DownloadLink);
    }

    private static void DownloadLink(KeyValuePair<string, string> link, bool retrying = false)
    {
        try
        {
            using (var webClient = new WebClient())
            {
                webClient.Headers.Add("User-Agent", "Mozilla/4.0 (compatible; MSIE 8.0)");
                webClient.DownloadFile(new Uri(link.Key), link.Value);
            }
        }
        catch (WebException e)
        {
            if (retrying) { return; } // Silently exit, we're retrying.

            if (e.Status != WebExceptionStatus.ProtocolError)
            {
                throw;
            }

            if (e.Message.Contains("(504) Gateway Timeout") || e.Message.Contains("(403) Forbidden"))
            {
                if (!RetryFailedDownload(link))
                {
                    Program.FailedDownloads.Add(link.Key); // Lets settle for the fact it can't download, and add it to the failed list.
                }
            }
            else
            {
                Logger.Error("Failed to download: " + link.Key);
                Logger.Error(e.Message);
            }
        }
    }

    private static bool RetryFailedDownload(KeyValuePair<string, string> link)
    {
        for (var i = 0; i < 4; i++) // Retry mechanism for 4 trys?
        {
            DownloadLink(link, true);

            if (File.Exists(link.Value)) // It finally managed to download?
            {
                return true;
            }
        }

        return false;
    }
}

Answer 1

Provided I understand the intent of this code, I think the following may be cleaner:

public static class DownloadUtilities
{
    private const int RETRY_COUNT = 4;

    public static void DownloadLinks(Dictionary<string, string> files)
    {
        Parallel.ForEach(
            files,
            new ParallelOptions { MaxDegreeOfParallelism = 20 },
            DownloadLink);
    }

    private static void DownloadLink(KeyValuePair<string, string> link, bool retrying = false)
    {
        int retriesRemaining = retrying ? RETRY_COUNT : 1;

        while (retriesRemaining > 0 && !File.Exists(link.Value))
        {
            retriesRemaining -= 1;
            try
            {
                using (var webClient = new WebClient())
                {
                    webClient.Headers.Add("User-Agent", "Mozilla/4.0 (compatible; MSIE 8.0)");
                    webClient.DownloadFile(new Uri(link.Key), link.Value);
                }
            }
            catch (WebException e)
            {

                if (e.Status != WebExceptionStatus.ProtocolError)
                {
                    throw;
                }

                var response = ex.Response as HttpWebResponse;

                if (response.StatusCode != HttpStatusCode.GatewayTimeout || 
                    response.StatusCode != HttpStatusCode.Forbidden)
                {

                    Program.FailedDownloads.Add(link.Key);
                    Logger.Error($"Failed to download {link.Key}, unhandled response code.");
                    return;
                }

                Logger.Error($"Failed to download {link.Key}, {retriesRemaining} attempts remaining");
                Logger.Error(e.Message);
            }
        }

        if (!File.Exists(link.Value))
        {
            // Lets settle for the fact it can't download, and add it to the failed list.
            Program.FailedDownloads.Add(link.Key);
            Logger.Error($"Failed to download {link.Key}, retries expired.");
        }
    }
}

Here is a summary of the changes and their reasoning:

• Replaced addition of string literals and variables with string interpolation, which is generally regarded as better practice.
• Making use of the HttpStatusCode enum instead of matching on the response message. This shortens the code and does not require string matching.
• Moving the retry logic into the DownloadLink method, rather than extracting into an auxiliary method, since this does not add to the code and complicates behaviour.