C# Tasks for Asynchronous Operations

Question

I currently use the following code to asynchronously create a cimsession to a remote host, query the remote host, return multiple values and finally updated some textboxes on my WPF window. Just wanting to know if anyone can see any glaring issues (I'm new to all things async and c# :) ) or a better was of doing it:

using System.Threading;
using System.Threading.Tasks;
using Microsoft.Management.Infrastructure;

//creating cimsession to remote host
CimSession session = await Task.Run(() =>
{
     return CimSession.Create(computerHostName);
});

//getting username, bootupstate, manufacturer and model 
var GI1 = await Task.Run(() =>
{
    var results = new List<string>();
    IEnumerable<CimInstance> queryResults = session.QueryInstances(nameSpace, WQL, "SELECT Username, BootUpState, Manufacturer, Model FROM Win32_ComputerSystem");
    foreach(CimInstance i in queryResults)
    {
        results.Add(i.CimInstanceProperties["Username"].Value.ToString());
        results.Add(i.CimInstanceProperties["BootUpState"].Value.ToString());
        results.Add(i.CimInstanceProperties["Manufacturer"].Value.ToString());
        results.Add(i.CimInstanceProperties["Model"].Value.ToString());
    }
    //returning all the variables
    return results;
});
//adding username, bootupstate, manufacturer and model to their textboxes
GIUS.Text = GI1[0];
GIBS.Text = GI1[1];
GIMF.Text = GI1[2];
GIMD.Text = GI1[3];

Any suggestions or help is appreciated :)

Answer 1

Conventions

• Use meaningful and camel-cased names var GI1 GIUS ..
• Use var when the instance type is the same as the declaring type CimSession session =

Design

I suggest to create a class to store the info you require from the cim interface.

 class CimInfo
 {
     public CimInfo(CimInstance cim)
     {
         UserName = GetProperty(cim, "UserName");
         BootUpState = GetProperty(cim, "BootUpState");
         Manufacturer = GetProperty(cim, "Manufacturer");
         Model = GetProperty(cim, "Model");
     }

     private static string GetProperty(CimInstance cim, string name)
     {
         if (cim == null) throw new ArgumentNullException(nameof(cim));
         return cim.CimInstanceProperties[name].Value.ToString();
     }

     public string UserName { get; }
     public string BootUpState { get; }
     public string Manufacturer { get; }
     public string Model { get; }
 }

The async operations can be merged and rewritten using the new class. Since you only have need of one cim instance, we could also avoid looping all queried results.

Notes:

• Perhaps there is an alternative available for QueryInstances that only returns the first result.
• If CimSession implements IDisposable, use a using block for it.

snippet

var cimInfo = await Task.Run(() =>
  {
      var session = CimSession.Create(computerHostName);
      var queryResults = session.QueryInstances(nameSpace, WQL, 
          "SELECT Username, BootUpState, Manufacturer, Model FROM Win32_ComputerSystem");
      return new CimInfo(queryResults.FirstOrDefault());
  });

And the output could be

TextBoxUserName.Text = cimInfo.UserName;
TextBoxBootUpState.Text = cimInfo.BootUpState;
TextBoxManufacturer.Text = cimInfo.Manufacturer;
TextBoxModel.Text = cimInfo.Model;

Answer 2

//creating cimsession to remote host
CimSession session = await Task.Run(() =>
{
     return CimSession.Create(computerHostName);
});

Task.Run is a red flag. This should be

CimSession session = await CimSession.CreateAsync(computerHostName);

Similarly for every method you call on session which has a ...Async version, you should use the ...Async version.

---

It seems that you're calling this from a GUI thread. That means you need to be careful about where your continuations are executed. I would suggest pulling the async calls out into a method and using .ConfigureAwait(false) on all of them, and then calling that method from the GUI thread with ContinueWith(...) to update the GUI, but since you refuse to provide more context I can't be more specific.

Answer 3

Please note that you have a memory leak. You have to dispose of the CimSession and the CimInstances that are created. You can confirm this with something like dotMemory as I have tested and confirmed. You either need to use a using statement or dispose. You can dispose of the CimInstances after you process them in the foreach loop. Also Microsoft.Management.Infrastructure already implements asynchronous methods they just don't conform to the normal TPL and use the observer pattern which makes it not as obvious on how to use(As I have discovered trying to migrate a library from System.Management). If you are doing this for multiple remote computers using Task.Run is going to use the thread pool for each task and block while it performs that synchronous code which remote CimSessions can take awhile and could lead to thread pool exhaustion. Its hard to see with example how you are actually using code and if that could be an issue. If using Reactive extensions is an option you can use it to make it easier like so.

            var tuple = await CimSession
                .CreateAsync(null)
                .SelectMany(_ => Observable.Using(() => _, session => session.QueryInstancesAsync(@"root\cimv2", "WQL", "SELECT Username, BootUpState, Manufacturer, Model FROM Win32_ComputerSystem")), (session, cimInstance) =>
                {
                    using (cimInstance)
                    {
                        return Tuple
                            .Create(cimInstance.CimInstanceProperties["Username"]
                                    ?.Value?.ToString() ?? string.Empty
                                , cimInstance.CimInstanceProperties["BootUpState"]
                                    ?.Value?.ToString() ?? string.Empty
                                , cimInstance.CimInstanceProperties["Manufacturer"]
                                    ?.Value?.ToString() ?? string.Empty
                                , cimInstance.CimInstanceProperties["Model"]
                                    ?.Value?.ToString() ?? string.Empty);
                    }
                })
                .FirstAsync();

You also could have an exception if the value is null and then call ToString() as you can see below I account for that as well. I am assuming that wql query will only ever return one result otherwise you could remove the .FirstAsync() and put .ToList(). It is returning a tuple value where you can just access every item in tuple but as @dfhwze said probably better to use a class to store your values. Also since using wpf probably better to implement the MVVM pattern and use a view model and bind to those values from the class. If doing that ensure to implement the INotifyPropertyChanged interface on the class to ensure no wpf binding leaks.