0
\$\begingroup\$

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 :)

\$\endgroup\$
4
  • \$\begingroup\$ Which library comes CimSession from? \$\endgroup\$
    – dfhwze
    May 31, 2019 at 8:34
  • \$\begingroup\$ @dfhwze Apologies, I've updated the question :) \$\endgroup\$ May 31, 2019 at 8:37
  • \$\begingroup\$ This is a snippet which is missing important context. We can't give you a proper review unless you provide the full methods (or ideally the full class) and whatever XAML is necessary to show how they're called. (Eg. are we dealing with async event handlers?) \$\endgroup\$ May 31, 2019 at 12:24
  • \$\begingroup\$ Hi @PeterTaylor, I'm not really able to share any more code. I just wanted some assistance with the particular snippet. I really appreciate everyone's help and have marked dfhwze's response as the answer as it was the most helpful \$\endgroup\$ May 31, 2019 at 12:31

3 Answers 3

2
\$\begingroup\$

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;
\$\endgroup\$
1
  • \$\begingroup\$ Thank you for your quick reply :) I actually have multiple tasks that get information from WMI and other sources so I think creating a class for each one would just create unnecessary code. I like the idea though :) \$\endgroup\$ May 31, 2019 at 9:25
3
\$\begingroup\$
//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.

\$\endgroup\$
3
  • \$\begingroup\$ I'm not sure I understand that last part: using ConfigureAwait(false) in the pulled-out method makes sense, but why use ContinueWith (which will need to be directed to the UI thread) rather than await for the GUI update after calling such a method? Has something horrifying about async been discovered that I've not yet come across? \$\endgroup\$ Jun 1, 2019 at 14:13
  • \$\begingroup\$ @VisualMelon, the GUI update has to be directed to the UI thread somehow, and exceptions also want to be reflected in the UI from the UI thread. \$\endgroup\$ Jun 1, 2019 at 14:42
  • 1
    \$\begingroup\$ I can't downvote here, but CimSession.CreateAsync does not return a Task ... it's some bizarre MMI-specific type (as are all the other CIM "async" methods). Sucks but it's true. \$\endgroup\$
    – McGuireV10
    Apr 27, 2023 at 13:57
2
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.