Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have been looking everywhere to find good real world examples of the new Async and Await features in .net 4.5. I have come up with the following code to download a list of files and limit the number of concurrent downloads. I would appreciate any best practices or ways to improve/optimize this code.

We are calling the below code using the following statement.

await this.asyncDownloadManager.DownloadFiles(this.applicationShellViewModel.StartupAudioFiles, this.applicationShellViewModel.SecurityCookie, securityCookieDomain).ConfigureAwait(false);

We are then using events to add the downloaded files to an observablecollection (new thread safe version in .net 4.5) on the ViewModel.

public class AsyncDownloadManager
    {
        public event EventHandler<DownloadedEventArgs> FileDownloaded;

        public async Task DownloadFiles(string[] fileIds, string securityCookieString, string securityCookieDomain)
          {
            List<Task> allTasks = new List<Task>();
            //Limits Concurrent Downloads 
            SemaphoreSlim throttler = new SemaphoreSlim(initialCount: Properties.Settings.Default.maxConcurrentDownloads);

            var urls = CreateUrls(fileIds);

            foreach (var url in urls)   
            {  
                await throttler.WaitAsync();
                allTasks.Add(Task.Run(async () => 
                {
                    try
                    {
                        HttpClientHandler httpClientHandler = new HttpClientHandler();
                        if (!string.IsNullOrEmpty(securityCookieString))
                        {
                            Cookie securityCookie;
                            securityCookie = new Cookie(FormsAuthentication.FormsCookieName, securityCookieString);
                            securityCookie.Domain = securityCookieDomain;
                            httpClientHandler.CookieContainer.Add(securityCookie);    
                        }                     

                        await DownloadFile(url, httpClientHandler).ConfigureAwait(false);
                    }
                    finally
                    {
                        throttler.Release();
                    }
                }));
            }
            await Task.WhenAll(allTasks).ConfigureAwait(false);
        }

        async Task DownloadFile(string url, HttpClientHandler clientHandler)
        {
            HttpClient client = new HttpClient(clientHandler);
            DownloadedFile downloadedFile = new DownloadedFile();

            try
            {
                HttpResponseMessage responseMessage = await client.GetAsync(url).ConfigureAwait(false);
                var byteArray = await responseMessage.Content.ReadAsByteArrayAsync().ConfigureAwait(false);

                if (responseMessage.Content.Headers.ContentDisposition != null)
                {
                    downloadedFile.FileName = Path.Combine(Properties.Settings.Default.workingDirectory, responseMessage.Content.Headers.ContentDisposition.FileName);
                }
                else
                {
                    return;
                }

                if (!Directory.Exists(Properties.Settings.Default.workingDirectory))   
                {
                    Directory.CreateDirectory(Properties.Settings.Default.workingDirectory);
                }
                using (FileStream filestream = new FileStream(downloadedFile.FileName, FileMode.Create, FileAccess.Write, FileShare.None, bufferSize: 4096, useAsync: true))
                {
                    await filestream.WriteAsync(byteArray, 0, byteArray.Length);
                }
            }
            catch(Exception ex)
            {    
                return; 
            }
            OnFileDownloaded(downloadedFile);
        }

        private void OnFileDownloaded(DownloadedFile downloadedFile)
        {    
            if (this.FileDownloaded != null)
            {
                this.FileDownloaded(this, new DownloadedEventArgs(downloadedFile));
            }
        }    

    public class DownloadedEventArgs : EventArgs
    {
        public DownloadedEventArgs(DownloadedFile downloadedFile)
        {   
            DownloadedFile = downloadedFile;
        }

        public DownloadedFile DownloadedFile { get; set; }
    }
  1. What is the affect of embedding Async / Await in other Async / Await methods? (Write filestream to disk inside an Async / Await method.
  2. Is the ConfigureAwait(False) used properly or should we use it more liberally?
  3. Should a httpclient be used for each separate task or should they share a singe one? 4. Are events a good way to "send" the downloaded file reference to the viewmodel?
  4. Is the await Task.WhenAll(allTasks).ConfigureAwait(false); the best way to run all tasks concurrently limited by the SemaphoreSlim throttle.
share|improve this question

2 Answers 2

up vote 9 down vote accepted

First, some smaller things (in terms of the size of code changed):

  1. I think you should use var more, especially when it's clear what type the object has, because you're just creating it.
  2. You shouldn't read the response as a single byte array. Instead, you should use Streams where possible (it certainly is possible in your case), because they are more efficient.
  3. Don't just ignore unknown exceptions. If there are some exceptions which you want to ignore (e.g. when a website returns 404), specify them explicitly.

The big thing I would change about your code is how you throttle your code and how you return the results. While throttling your code using Semaphore will work fine, I think you should let some library for asynchronous handling of collections do that for you. Such libraries include Rx and TPL Dataflow. Doing that will also let you avoid using an event, because your method will return “asynchronous collection” instead (IObservable<T> in Rx, ISourceBlock<T> in TPL Dataflow).

With TPL Dataflow, your code could look something like:

public ISourceBlock<DownloadedFile> DownloadFiles(string[] fileIds, string securityCookieString, string securityCookieDomain)
{
    var urls = CreateUrls(fileIds);

    // we have to use TransformManyBlock here, because we want to be able to return 0 or 1 items
    var block = new TransformManyBlock<string, DownloadedFile>(
        async url =>
        {
            var httpClientHandler = new HttpClientHandler();
            if (!string.IsNullOrEmpty(securityCookieString))
            {
                var securityCookie = new Cookie(FormsAuthentication.FormsCookieName, securityCookieString);
                securityCookie.Domain = securityCookieDomain;
                httpClientHandler.CookieContainer.Add(securityCookie);
            }

            return await DownloadFile(url, httpClientHandler);
        }, new ExecutionDataflowBlockOptions { MaxDegreeOfParallelism = Properties.Settings.Default.maxConcurrentDownloads });

    foreach (var url in urls)
        block.Post(url);

    block.Complete();

    return block;
}

private static async Task<DownloadedFile[]> DownloadFile(string url, HttpClientHandler clientHandler)
{
    var client = new HttpClient(clientHandler);
    var downloadedFile = new DownloadedFile();

    try
    {
        HttpResponseMessage responseMessage = await client.GetAsync(url);

        if (responseMessage.Content.Headers.ContentDisposition == null)
            return new DownloadedFile[0];

        downloadedFile.FileName = Path.Combine(
            Properties.Settings.Default.workingDirectory, responseMessage.Content.Headers.ContentDisposition.FileName);

        if (!Directory.Exists(Properties.Settings.Default.workingDirectory))
        {
            Directory.CreateDirectory(Properties.Settings.Default.workingDirectory);
        }

        using (var httpStream = await responseMessage.Content.ReadAsStreamAsync())
        using (var filestream = new FileStream(
            downloadedFile.FileName, FileMode.Create, FileAccess.Write, FileShare.None, bufferSize: 4096, useAsync: true))
        {
            await httpStream.CopyToAsync(filestream, 4096);
        }
    }
    // TODO: improve
    catch (Exception ex)
    {
        return new DownloadedFile[0];
    }

    return new[] { downloadedFile };
}
share|improve this answer
    
Thanks for the code. I tend to not use var when posting samples. I was thinking that stream was a much better way to consume than the bytearray, but I was worried about the async nature. I see how your code would work perfectly dealing with the streams. Finally - it seems like I need to really research Rx and TPL more. I had thought that they were somewhat obsolete with Async/Await, but it looks like I was mistaken. New releases for both. You would recommend TPL for this scenario? –  Blane Bunderson Nov 20 '12 at 1:03
    
TPL is certainly not obsolete in any way by async, party because async is actually built on top of TPL (the Task type) and partly because they are doing different things (e.g. there is no direct async alternative to Parallel.ForEach()). But I was talking specifically about TPL Dataflow, which is new in .Net 4.5 and was written to take advantage of async. And Rx is not obsolete either, its uses are mostly different than plain async (though there is some overlap). –  svick Nov 20 '12 at 1:15
1  
Any reason you are not using this GetAsync overload with HttpCompletionOption.ResponseHeadersRead and then consume the response with ReadAsStreamAsync()? –  G. Stoynev Dec 18 '13 at 16:43
    
@G.Stoynev I just copied that code from the question. I don't actually know HttpClient that well. –  svick Dec 18 '13 at 17:23

Sorry, I did not look thoroughly through your code, but regarding "good real world examples of the new Async and Await features":

Writing simple windows 8 store (ex-"Metro-style") application helped me greatly to understand and get used to async/await pattern.

Basically, in their new API everything that can be time-consuming (disk IO, network IO, etc) is now async. You just can't call it synchronously.

I think this is aimed to force us writing responsive apps which do not block UI thread. For example, you need to download a file and save it to disk, and display busy indicator in UI. In your ViewModel you then say something like:

void DoTheJob()
{
  IsBusy=true;
  var text = await DownloadText();
  ProgressText="Saving...";
  await SaveText(text);
  IsBusy=false;
}

I'd say this is a good real world example (my "dictionary" app uses this approach to load translations from disk).

This is a really cool way to deal with UI, in contrast to old BackgroundWorkers/ThreadPool and Dispatcher.BeginInvoke hassle.

share|improve this answer
4  
Note that for your code to compile, you would need to add the async modifier to your method. Also, it would be preferable if you changed its return type to Task, so that your method could be awaited in turn. –  svick Nov 13 '12 at 20:40

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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