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

asyncasynchronous-programmingcconcurrencymultithreading

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;
            }            
        }

Best Answer

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;
}
Related Topic