0

I'm using the following async code in a web api controller to process an XML file. Everything works as expected, however is this the correct use of the async/await approach. I'm basically extracting all images from the XML file and then saving them to disk. I wanted to try an minimise the impact of file io.

public async Task<HttpResponseMessage> PostFile()
{
    await Task.WhenAll(this.ProcessProofOfPressenceImages(address, images, surveyReference), this.ProcessSketchImages(propertyPlans, images, surveyReference), this.ProcessExteriorImages(exteriorSketch, images, surveyReference));
    //db code
}

private async Task ProcessProofOfPressenceImages(Dictionary<object, object> container, List<Dictionary<string, string>> images, string surveyReference)
{
    if(images != null)
    {
        await Task.WhenAll(this.ProcessImagesHelper(container, images, surveyReference, "propertyImage"));
    }
}

private async Task ProcessSketchImages(Dictionary<object, object> container, List<Dictionary<string, string>> images, string surveyReference)
{
    if(images != null)
    {
        await Task.WhenAll(this.ProcessImagesHelper(container, images, surveyReference, "sketchPlanImage"), this.ProcessImagesHelper(container, images, surveyReference, "sketchFrontImage"), this.ProcessImagesHelper(container, images, surveyReference, "sketchRearImage"), this.ProcessImagesHelper(container, images, surveyReference, "sketchLeftSideImage"), this.ProcessImagesHelper(container, images, surveyReference, "sketchRightSideImage"));
    }
}

private async Task ProcessExteriorImages(Dictionary<object, object> container, List<Dictionary<string, string>> images, string surveyReference)
{
    List<Task> tasks = new List<Task>();

    if(images != null)
    {
        await Task.WhenAll(this.ProcessImagesHelper(container, images, surveyReference, "image1"), this.ProcessImagesHelper(container, images, surveyReference, "image2"), this.ProcessImagesHelper(container, images, surveyReference, "image3"), this.ProcessImagesHelper(container, images, surveyReference, "image4"), this.ProcessImagesHelper(container, images, surveyReference, "image5"), this.ProcessImagesHelper(container, images, surveyReference, "image6"));
    }
}

private async Task ProcessImagesHelper(Dictionary<object, object> container, List<Dictionary<string, string>> images, string surveyReference, string image)
{
    if(container.ContainsKey(image) && !String.IsNullOrEmpty(container[image].ToString()))
    {
        using(MemoryStream memoryStream = new MemoryStream((byte[])container[image]))
        {
            string url = String.Format(@"{0}{1}{2}_{3}.jpg", EcoConfiguration.Instance.RootUrl, EcoConfiguration.Instance.SurveyImageRootUrl, surveyReference, image.SplitOnCapital("_"));

            using(FileStream fileStream = new FileStream(url, FileMode.Create, FileAccess.Write))
            {
                Dictionary<string, string> imageDetails = new Dictionary<string, string>();
                imageDetails.Add("TypeId", ((int)SurveyImageType.Exterior).ToString());
                imageDetails.Add("ImageUrl", url);
                if(container.ContainsKey(image + "Description"))
                {
                    imageDetails.Add("Description", container[image + "Description"].ToSafeString());
                }
                images.Add(imageDetails);
                await memoryStream.CopyToAsync(fileStream);
            }
        }
    }
}

Any comments/suggestions would be very welcome.

markpirvine
  • 1,485
  • 1
  • 23
  • 54

1 Answers1

5

The tricky thing about file streams is that you need to pass isAsync: true or FileOptions.Asynchronous to the constructor/factory method to get a truly asynchronous stream. If you don't do this, then the underlying file stream is actually blocking and the asynchronous methods just use the thread pool to fake asynchronous operations.

The other thing that stands out to me in your code is that you've got some unnecessary use of async. async should be used only when you need it. E.g., this method:

private async Task ProcessProofOfPressenceImages(Dictionary<object, object> container, List<Dictionary<string, string>> images, string surveyReference)
{
  if(images != null)
  {
    await Task.WhenAll(this.ProcessImagesHelper(container, images, surveyReference, "propertyImage"));
  }
}

could be written as:

private Task ProcessProofOfPressenceImages(Dictionary<object, object> container, List<Dictionary<string, string>> images, string surveyReference)
{
  if(images != null)
  {
    return Task.WhenAll(this.ProcessImagesHelper(container, images, surveyReference, "propertyImage"));
  }

  return Task.FromResult<object>(null);
}

which saves you an unnecessary state machine. The same advice would apply to ProcessSketchImages and ProcessExteriorImages.

Regarding ProcessImagesHelper, it looks pretty good, though I'm not sure why you need the MemoryStream. It would be just as easy to (asynchronously) write the byte array out to disk.

If you're interested in async performance tips, Stephen Toub has an excellent video.

Stephen Cleary
  • 437,863
  • 77
  • 675
  • 810
  • Stephen, thanks for the reply and advice - very much appreciated - all of the async methods didn't feel right! I've changed the code as suggested and removed the need for the MemoryStream and set the isAsync parameter to true. I used another article (http://stackoverflow.com/questions/1862982/c-sharp-filestream-optimal-buffer-size-for-writing-large-files) to determine an optimal buffer size for the FileStream. – markpirvine Feb 27 '13 at 15:28