2

Method is downloading file to local file system from HTTP server. On high load some downloaded files are empty. Files accessed right after download.

 private static Task<FileHolderModel> DownloadFile(string downloadUrl)
   {
       return Client.GetAsync(downloadUrl, HttpCompletionOption.ResponseHeadersRead).ContinueWith(task =>
       {
           using (var response = task.Result)
           {
               var originalFileName = ReadFileName(response);
               var fullName = Path.Combine(Helper.CreateTempFolder(), $"{Guid.NewGuid()}{Path.GetExtension(originalFileName)}");
               using (var fileStream = File.Open(fullName, FileMode.Create))
               {
                   response.Content.CopyToAsync(fileStream).Wait();
                   fileStream.Flush(true);
               }
               return new FileHolderModel(fullName, originalFileName);
           }
       });
   }

What could cause empty files?

Jonas
  • 4,683
  • 4
  • 45
  • 81
  • Can you show the code that calls this method? Are you using `await` ? – Darren Wainwright Jan 14 '19 at 16:20
  • 7
    You're using `.Wait()` here which is going to give you all sorts of problems. Why are you even using `ContinueWith` at all? Just await the result of the `GetAsync` operation and use it later. – DavidG Jan 14 '19 at 16:20
  • 1
    Is there a specific reason why you don't `await` CopyToAsync? – Fildor Jan 14 '19 at 16:23
  • I don't use await just because I can. Why I should add additional complexity to my code when everything can be done in more simple way. The question is why I can't use plain TPL? Would be great that someone could pinpoint the problem instead of stigmatize .Wait() :) – Jonas Jan 14 '19 at 17:54
  • Why `HttpCompletionOption.ResponseHeadersRead`? I suspect that's your problem. – glenebob Jan 14 '19 at 20:50
  • If you're dead set on using synchronous code, why not use `CopyTo()`? It's a lot more readable than `CopyToAsync().Wait()`, and does the exact same thing as far as you're concerned. – glenebob Jan 14 '19 at 20:55
  • @glenebob he is reading file name from header content disposition, that's why he is using HttpCompletionOption.ResponseHeadersRead – Tomas Jan 14 '19 at 22:00
  • Thanks @Tomas, it pays to read documentation :) – glenebob Jan 14 '19 at 23:34
  • 1
    @Jonas, you are not checking the status code of the HTTP response. If the server sends back an error status with an empty payload, that could result in the condition you observe. – glenebob Jan 14 '19 at 23:36

1 Answers1

3

You're in a bit of a mess here, likely because you're using ContinueWith and also .Wait() inside there. This function would be much simpler and readable if you use await and completely remove the ContinueWith block. For example:

private static async Task<FileHolderModel> DownloadFile(string downloadUrl)
             //^^^^^ make it an async method
{
    var response = await Client.GetAsync(downloadUrl, HttpCompletionOption.ResponseHeadersRead);
                 //^^^^^ await this call
    var originalFileName = ReadFileName(response);
    var fullName = Path.Combine(/* snip */);

    using (var fileStream = File.Open(fullName, FileMode.Create))
    {
        await response.Content.CopyToAsync(fileStream);
      //^^^^^ await this call too
        fileStream.Flush(true); //This is probably not needed
    }

    return new FileHolderModel(fullName, originalFileName);    
}
DavidG
  • 113,891
  • 12
  • 217
  • 223
  • Could you please explain why I can't use Wait() on task? And in general why I should not use TPL? – Jonas Jan 14 '19 at 16:43
  • @Jonas https://stackoverflow.com/questions/24623120/await-on-a-completed-task-same-as-task-result is a good start. – DavidG Jan 14 '19 at 16:44
  • I red all info and still can't find what is wrong with my code. I am observing unobserved exceptions (there was no exceptions) and ContinueWith should make sure that task is finished. What else can be wrong? – Jonas Jan 14 '19 at 17:14
  • While the suggested changes in this answer are quite sound and I agree with the way you've changed the code, this answer does not appear to answer the question of why some files are empty. – glenebob Jan 14 '19 at 23:34