Correct usage of async/await and Task.Run()

Question

I am developing an application that will read excel files from disk and then process tests based on the data in the files. In order to keep the user interface from locking up when loading files and running tests, I have implemented the async/await with Task.Run() to handle the loads and tests running. Right now I only have one test and three input files, but I want to make sure that this application will scale as the number of files & tests increase. My other concerns are not taking up too much processing power by having too many threads and being able to report the progress of the file loads and tests. Have I correctly implemented async/await/Task.Run in order to satisfy these concerns?

private async void ProcessTestsAsync()
 {
 try
 {
 //TestFactory testFact = new TestFactory();
 if (WorkingDirectory != null && (from f in Files where f.Location == "" select f).FirstOrDefault() == null)
 {
 // Load Files
 var loadResults = await LoadFilesAsync();
 // Run Tests
 if (loadResults)
 {
 var testResults = await RunTestsAsync();
 if (testResults)
 {
 MessageBox.Show("Success");
 }
 else
 {
 MessageBox.Show("Failure");
 }
 }
 else
 {
 MessageBox.Show("File Load Failed. Please check your files and try again.");
 }
 }
 else
 {
 MessageBox.Show("One or more files has not been selected. Please choose any missing files and try again.");
 }
 }
 catch (Exception err)
 {
 MessageBox.Show("Tests failed. Please try again. Error: " + err.Message);
 }
 }
private async Task<bool> RunTestsAsync()
 {
 try
 {
 using (var sem = new SemaphoreSlim(MAX_THREADS)) // Limit the number of threads that can run at a time
 {
 TestFactory testFact = new TestFactory();
 var tasks = new List<Task>();
 foreach (Model.Test test in IncludedTests)
 {
 await sem.WaitAsync();
 tasks.Add(Task.Run(() =>
 {
 SortedList<Enums.FileType, DataTable> sources = new SortedList<Enums.FileType, DataTable>();
 foreach (Enums.FileType fileType in test.ExpectedSources)
 {
 sources.Add(fileType, _files[fileType]);
 }
 test.Progress = 25;
 TestBase t = testFact.getTest(test.Type);
 if (t.SetWorkingDirectory(WorkingDirectory)) 
 {
 test.Progress = 50;
 if (t.SetSources(sources))
 {
 test.Progress = 75;
 if (t.RunTest())
 {
 test.Progress = 100;
 }
 } 
 }
 else
 {
 MessageBox.Show("Test Failed.");
 }
 })); 
 }
 await Task.WhenAll(tasks);
 }
 return true;
 }
 catch (Exception)
 {
 return false;
 }
 }
 private async Task<bool> LoadFilesAsync()
 {
 try
 {
 _files.Clear();
 using (var sem = new SemaphoreSlim(MAX_THREADS))
 {
 var tasks = new List<Task>();
 foreach (var file in Files)
 {
 await sem.WaitAsync(); // Limit the number of threads that can run at a time
 tasks.Add(Task.Run(() =>
 {
 file.FileLoadStatus = Enums.FileLoadStatus.InProgress;
 _files.Add(file.Type, file.Source.LoadRecords(file));
 file.FileLoadStatus = Enums.FileLoadStatus.Completed;
 }));
 }
 await Task.WhenAll(tasks);
 }
 return true;
 }
 catch (Exception)
 {
 return false;
 } 
 }

• Becuzz: This question is probably a better fit on CodeReview.SE rather than here.
• Tim Hutchison: @Becuzz I agree - I realized that after I had already asked the question and received an answer. I would have no problems with migrating this over

Answer 1

At first glance, I would say the async/await code looks OK. However, there are a few things to consider.

• Don't change global or class state in a task. Have the task be a true function (i.e. does not use or alter global or class state). Example in LoadFilesAsync()
• When the code to run is small enough, the task overhead is worse than just running the code in place (example in LoadFilesAsync()).
• I assume this is temporary, but MessageBox.ShowMessage() isn't a good idea deep in your tasks (example in RunTestsAsync() when the handling class also does the same thing)

In your LoadFilesAsync() method I would restructure it like this:

private async Task<IEnumerable<MyFile>> LoadFilesAsync()
{
 // Multiple tasks clearing state can step on each other's toes.
 // It's better to return the set of files and then have the receiver
 // code merge them or do work with it directly.
 List<MyFile> files = new List<MyFile>();
 foreach (var file in Files)
 {
 file.FileLoadStatus = Enums.FileLoadStatus.InProgress;
 // Just await the load here.
 // Since I don't know the type of your _files object
 // this code won't compile since List.Add() only takes one argument
 files.Add(file.Type, await file.Source.LoadRecords(file));
 file.FileLoadStatus = Enums.FileLoadStatus.Completed;
 }
 return files;
}

• Tim Hutchison: Will I need to use something like var loadResults = await Task.Run(LoadFilesAsync); to keep the application from locking if I remove the individual Task.Run()'s from LoadFilesAsync?
• Berin Loritsch: If LoadFilesAsync() is marked as async then no, you do not. Basically, you want the files to be loaded sequentially since that rarely takes a long time, but you don't want your task to be blocking while the read happens.
• Berin Loritsch: The bigger issue, of course, is that you were calling _file.Clear() in async code, which can be called in parallel. That means your files being loaded might not be as robust as you wanted.
• Jaxidian: Berin is absolutely correct with regards to MessageBox.ShowMessage usage. In these situations, I like to pass in a logger object of some sort, preferably with log levels (or at least a boolean to indicate something is verbose that can be controlled with a feature flag in configuration) so I can leave in these debug messages yet be able to enable/disable them at will without having to actually change code and without putting in bad-practice code.