6
\$\begingroup\$

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;
    }
}
\$\endgroup\$
  • 3
    \$\begingroup\$ Very ambiguous method. Can you clarify the specifics? You seem to care only about the first thrown exception's message, completely ignoring the rest. Sometimes you're throwing without even attempting 4 times, sometimes the function logs some stuff and other times it throws exceptions. Is there a reason you are only doing Program.FailedDownloads.Add(link.Key); without logging and vice-versa? It would be very useful if you can provide some more information on how you expect this function to work. \$\endgroup\$ – Denis Apr 24 '18 at 22:41
3
\$\begingroup\$

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.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.