Copy a stream to a file using Universal .NET

Question

I have answered to a question on Stackoverflow where Andy struggled to cancel a Stream copy to file operation using the Universal .NET Framework:

Apparently, cancelling the cts CancelOperation token had not the expected behavior.

await httpResponseContent.Content.WriteToStreamAsync(streamToWrite.AsOutputStream()).AsTask(cts, progressDownload); 

So I suggested to do it another way using a chunks-based stream reading and writing. So I provided this code which has been tested and accepted:

async Task Download(Uri uriToWork, CancellationToken cts, IProgress<int> progress) {
    using(HttpClient httpClient = new HttpClient()) {

        var chunkSize = 1024;
        var buffer = new byte[chunkSize];
        int count = 0;
        string fileToWrite = Path.GetTempFileName();

        using(var inputStream = await httpClient.GetInputStreamAsync(uriToWork)) {
            using(var streamToRead = inputStream.AsStreamForRead()) {
                using(Stream streamToWrite = File.OpenWrite(fileToWrite)) {
                    int size;
                    while((size = await streamToRead.ReadAsync(buffer, 0, chunkSize, cts).ConfigureAwait(false)) > 0) {
                        count += size;
                        await Dispatcher.RunAsync(CoreDispatcherPriority.Normal, () => progress.Report(count));
                        // progress.Report(count);
                        await streamToWrite.WriteAsync(buffer, 0, size, cts).ConfigureAwait(false);
                    }
                }
            }
        }
    }
}

Although it works, I am not that familiar with the Universal .NET and I am not sure this code is optimal. A casual .NET solution would be shorter.

I am very sensitive about code quality and canonicity and I wonder whether some parts of this code are redundant and can be removed, and I would like it to match the most "Universal .NET" oriented as possible.

Answer 1

1. I will first focus on your nested using statements, as that can be very problematic. As you can read on the help page of CA2202 (CA2202: Do not dispose objects multiple times) the suggested way to resolve this is the following:

   To resolve this issue, use a try/finally block instead of the outer using statement. In the finally block, make sure that the stream resource is not null.

   So let's map this to your code. You nest four different using statements, which is extremely problematic. Not only can this result in failures, it makes the code unnecessarily hard to read and comprehend. Refactor it to make use of the try-finally pattern. At most use one using statement

2. Additionaly I am unsure about your use of Dispatcher.RunAsync. Is there a specific reason for it? You are inside of an async method, meaning you don't have to wrap every method call awkwardly like that. Instead just call the commented-out line of code (and you should remove commented-out code in general).

3. The use of the var keyword seems strange to me. Why do you use it in this statement:

   using(var streamToRead = inputStream.AsStreamForRead())
   

   but not in this:

   using(Stream streamToWrite = File.OpenWrite(fileToWrite))
   

   I would generally discourage use of the var keyword if you don't explicitly state the type you define in a declaration. That means this line is okay:

   var buffer = new byte[chunkSize];
   

   But you should refactor the rest of your code to explicitly define the types of the variables.

4. Is there a specific reason to define some variables towards the top of the method and others at the place of use? Also why are they not defined at the very beginning of your method? You should decide on one approach. I prefer defining them right before they are used. This would e.g. mean moving int count = 0 down:

   int count = 0;
   int size;
   while((size = await streamToRead.ReadAsync(buffer, 0, chunkSize, cts).ConfigureAwait(false)) > 0)